[PATCH] Address some -O3 issues on Ubuntu 10.04

Stefan Metzmacher metze at samba.org
Thu Aug 4 21:26:46 UTC 2016


Am 04.08.2016 um 21:35 schrieb Jeremy Allison:
> On Thu, Aug 04, 2016 at 09:29:13PM +0200, Michael Adam wrote:
>> On 2016-08-04 at 08:56 -0700, Jeremy Allison wrote:
>>> On Thu, Aug 04, 2016 at 11:38:33AM +0200, Michael Adam wrote:
>>>> On 2016-08-04 at 00:39 +0200, Michael Adam wrote:
>>>>> On 2016-08-03 at 17:41 +0200, Stefan Metzmacher wrote:
>>>>>> Hi,
>>>>>>
>>>>>> here's an update that also includes andrew's backupkey test fixes.
>>>>>>
>>>>>> All with bug references for the backports.
>>>>>>
>>>>>> Please review and push.
>>>>>>
>>>>>> (I already pushed the two patches from Andrew)
>>>>>
>>>>> Reviewed-by: me and pushed to autobuild.
>>>>
>>>> Re-pushed after failed autobuild.
>>>
>>> Yes, mine and Andreas's autobuild failed also.
>>> I'll re-push one more time. Ralph seemed to
>>> get something through (god knows how :-).
>>
>> Yeah... some time ago. ;-)
>>
>> Metze's last autobuild failed as well.
>>
>> Mine is currently running (at this moment at the drs tests...)
>> and it has metze's last autobuild content included.
>> Andreas' autobuild is in the queue mirroring mine.
>>
>>> I think if metze recent work doesn't fix it the
>>> samba4.*samba_tool* tests need to be skipped
>>> until fixed.
>>
>> This!
> 
> Well after metze's analysis I have one final attempt
> at fixing them (attached). I think patch #1 is a
> genuine bugfix that needs to go in regardless.
> 
> Patch #2 is a band-aid for the error, but something
> like it is needed to stop recursion (which currently
> isn't prevented in the repl code at all).
> 
> If you want to review and try pushing them I won't
> object :-).

I'm currently running some autobuilds with the following
patchset...

metze
-------------- next part --------------
From 5d7603ed81bc7bc53ee47a837403112900f95284 Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 4 Aug 2016 11:09:21 -0700
Subject: [PATCH 1/8] s4: repl: Ensure all error paths in
 dreplsrv_op_pull_source_get_changes_trigger() are protected with tevent
 returns.

Otherwise dreplsrv_op_pull_source_get_changes_trigger() could infinitely recurse.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source4/dsdb/repl/drepl_out_helpers.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c
index bf8a372..29bb9d5 100644
--- a/source4/dsdb/repl/drepl_out_helpers.c
+++ b/source4/dsdb/repl/drepl_out_helpers.c
@@ -446,6 +446,9 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
 		if (!W_ERROR_IS_OK(werr)) {
 			DEBUG(0,(__location__ ": Failed to convert UDV for %s : %s\n",
 				 ldb_dn_get_linearized(partition->dn), win_errstr(werr)));
+			/* The above error can only be OUT_OF_MEMORY. */
+			tevent_req_nterror(req, NT_STATUS_NO_MEMORY);
+			return;
 		}
 	}
 
@@ -470,6 +473,7 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
 		status = dreplsrv_get_gc_partial_attribute_set(service, r, &pas);
 		if (!NT_STATUS_IS_OK(status)) {
 			DEBUG(0,(__location__ ": Failed to construct GC partial attribute set : %s\n", nt_errstr(status)));
+			tevent_req_nterror(req, status);
 			return;
 		}
 		replica_flags &= ~DRSUAPI_DRS_WRIT_REP;
@@ -482,6 +486,7 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
 		status = dreplsrv_get_rodc_partial_attribute_set(service, r, &pas, for_schema);
 		if (!NT_STATUS_IS_OK(status)) {
 			DEBUG(0,(__location__ ": Failed to construct RODC partial attribute set : %s\n", nt_errstr(status)));
+			tevent_req_nterror(req, status);
 			return;
 		}
 		replica_flags &= ~DRSUAPI_DRS_WRIT_REP;
-- 
1.9.1


From 5bd2f3abab0fdded4edf67d08b605e37c52340bc Mon Sep 17 00:00:00 2001
From: Jeremy Allison <jra at samba.org>
Date: Thu, 4 Aug 2016 11:13:50 -0700
Subject: [PATCH 2/8] s4: repl: Add protection to
 dreplsrv_op_pull_source_get_changes_trigger() to prevent recursion.

Signed-off-by: Jeremy Allison <jra at samba.org>
---
 source4/dsdb/repl/drepl_out_helpers.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)

diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c
index 29bb9d5..3f34d76 100644
--- a/source4/dsdb/repl/drepl_out_helpers.c
+++ b/source4/dsdb/repl/drepl_out_helpers.c
@@ -241,6 +241,7 @@ struct dreplsrv_op_pull_source_state {
 	struct tevent_context *ev;
 	struct dreplsrv_out_operation *op;
 	void *ndr_struct_ptr;
+	unsigned int depth;
 };
 
 static void dreplsrv_op_pull_source_connect_done(struct tevent_req *subreq);
@@ -260,6 +261,7 @@ struct tevent_req *dreplsrv_op_pull_source_send(TALLOC_CTX *mem_ctx,
 	}
 	state->ev = ev;
 	state->op = op;
+	state->depth = 0;
 
 	subreq = dreplsrv_out_drsuapi_send(state, ev, op->source_dsa->conn);
 	if (tevent_req_nomem(subreq, req)) {
@@ -421,6 +423,19 @@ static void dreplsrv_op_pull_source_get_changes_trigger(struct tevent_req *req)
 	struct drsuapi_DsReplicaHighWaterMark highwatermark;
 	struct ldb_dn *schema_dn = ldb_get_schema_basedn(service->samdb);
 
+	/*
+	 * Stop this infinitely recursing.
+	 */
+
+	if (state->depth > 2) {
+		DBG_ERR("called with depth %u. Error out to stop recursion.\n",
+			state->depth);
+		tevent_req_nterror(req, NT_STATUS_INTERNAL_ERROR);
+		return;
+	}
+
+	state->depth++;
+
 	r = talloc(state, struct drsuapi_DsGetNCChanges);
 	if (tevent_req_nomem(r, req)) {
 		return;
-- 
1.9.1


From 00b11d3c94729e19224c1d397f96b25a3816c004 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 4 Aug 2016 10:25:32 +0200
Subject: [PATCH 3/8] reproduce-try1.sh

run this in TDB_NO_FSYNC=1 make -j testenv SELFTEST_TESTENV=vampire_dc SCREEN=1 SAMBA_OPTIONS="-d4"
---
 reproduce-try1.sh | 11 +++++++++++
 1 file changed, 11 insertions(+)
 create mode 100644 reproduce-try1.sh

diff --git a/reproduce-try1.sh b/reproduce-try1.sh
new file mode 100644
index 0000000..7fb97c2
--- /dev/null
+++ b/reproduce-try1.sh
@@ -0,0 +1,11 @@
+
+for i in $(seq 1 2); do
+	echo $i;
+	PYTHONPATH=bin/python:source4/torture/drs/python DC1=$DC_SERVER DC2=$VAMPIRE_DC_SERVER source4/scripting/bin/subunitrun repl_schema -U$DOMAIN/$DC_USERNAME%$DC_PASSWORD || {
+		echo "FAIL DC1 DC2: $i"; break;
+	} ;
+	PYTHONPATH=bin/python:source4/torture/drs/python DC2=$DC_SERVER DC1=$VAMPIRE_DC_SERVER source4/scripting/bin/subunitrun repl_schema -U$DOMAIN/$DC_USERNAME%$DC_PASSWORD || {
+		echo "FAIL DC2 DC1: $i"; break;
+	} ;
+	echo "OK: $i";
+done
-- 
1.9.1


From 773d35cbcabe643809dc71c88d2c2443ef4bed9d Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 4 Aug 2016 09:01:04 +0200
Subject: [PATCH 4/8] debug schema problem

---
 source4/dsdb/repl/drepl_out_helpers.c  |  2 +-
 source4/dsdb/repl/replicated_objects.c |  4 ++--
 source4/dsdb/schema/schema_info_attr.c | 12 ++++++++++--
 3 files changed, 13 insertions(+), 5 deletions(-)

diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c
index 3f34d76..d0a822d 100644
--- a/source4/dsdb/repl/drepl_out_helpers.c
+++ b/source4/dsdb/repl/drepl_out_helpers.c
@@ -881,7 +881,7 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req
 			tevent_req_nterror(req, nt_status);
 			return;
 		}
-		DEBUG(4,("Wrong schema when applying reply GetNCChanges, retrying\n"));
+		DEBUG(0,("Wrong schema when applying reply GetNCChanges, retrying\n"));
 
 		dreplsrv_op_pull_source_get_changes_trigger(req);
 		return;
diff --git a/source4/dsdb/repl/replicated_objects.c b/source4/dsdb/repl/replicated_objects.c
index fb8083d..4aa319b 100644
--- a/source4/dsdb/repl/replicated_objects.c
+++ b/source4/dsdb/repl/replicated_objects.c
@@ -635,7 +635,7 @@ WERROR dsdb_replicated_objects_convert(struct ldb_context *ldb,
 		 */
 		status = dsdb_schema_info_cmp(schema, mapping_ctr);
 		if (!W_ERROR_IS_OK(status)) {
-			DEBUG(4,("Can't replicate %s because remote schema has changed since we last replicated the schema\n",
+			DEBUG(0,("Can't replicate %s because remote schema has changed since we last replicated the schema\n",
 				 ldb_dn_get_linearized(partition_dn)));
 			talloc_free(out);
 			return status;
@@ -915,7 +915,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
 		}
 	}
 
-	DEBUG(2,("Replicated %u objects (%u linked attributes) for %s\n",
+	DEBUG(0,("Replicated %u objects (%u linked attributes) for %s\n",
 		 objects->num_objects, objects->linked_attributes_count,
 		 ldb_dn_get_linearized(objects->partition_dn)));
 		 
diff --git a/source4/dsdb/schema/schema_info_attr.c b/source4/dsdb/schema/schema_info_attr.c
index e113033..e8b9fef 100644
--- a/source4/dsdb/schema/schema_info_attr.c
+++ b/source4/dsdb/schema/schema_info_attr.c
@@ -179,6 +179,7 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 	DATA_BLOB blob;
 	char *schema_info_str;
 	struct drsuapi_DsReplicaOIDMapping *mapping;
+	WERROR werr;
 
 	/* we should have at least schemaInfo element */
 	if (ctr->num_mappings < 1) {
@@ -200,9 +201,16 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 	W_ERROR_HAVE_NO_MEMORY(schema_info_str);
 
 	bres = strequal(schema->schema_info, schema_info_str);
+	werr = bres ? WERR_OK : WERR_DS_DRA_SCHEMA_MISMATCH;
+	if (1) {
+		DEBUG(0,("dsdb_schema_info_cmp(%s):\nremote[%s]\nlocal[%s]\nts_last_change[%s], metadata_usn[%llu]\n",
+			win_errstr(werr),
+			schema_info_str, schema->schema_info,
+			timestring(schema_info_str, schema->ts_last_change),
+			(unsigned long long)schema->metadata_usn));
+	}
 	talloc_free(schema_info_str);
-
-	return bres ? WERR_OK : WERR_DS_DRA_SCHEMA_MISMATCH;
+	return werr;
 }
 
 
-- 
1.9.1


From 8293d00c0d338decac89e907d31b31997d062d93 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 4 Aug 2016 10:03:14 +0200
Subject: [PATCH 5/8] struct dsdb_schema_info part 1

---
 source4/dsdb/samdb/ldb_modules/schema_util.c | 18 ++--------
 source4/dsdb/schema/schema.h                 |  2 +-
 source4/dsdb/schema/schema_info_attr.c       | 44 ++++++++++++++++---------
 source4/dsdb/schema/schema_init.c            | 31 ++++++++++--------
 source4/dsdb/schema/schema_prefixmap.c       | 22 +++++++------
 source4/torture/drs/unit/prefixmap_tests.c   | 49 +++++++++++++---------------
 source4/torture/drs/unit/schemainfo_tests.c  | 12 ++++---
 7 files changed, 91 insertions(+), 87 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/schema_util.c b/source4/dsdb/samdb/ldb_modules/schema_util.c
index 7402c04..027487b 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_util.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_util.c
@@ -276,9 +276,7 @@ int dsdb_module_schema_info_update(struct ldb_module *ldb_module,
 {
 	int ret;
 	const struct GUID *invocation_id;
-	DATA_BLOB ndr_blob;
 	struct dsdb_schema_info *schema_info;
-	const char *schema_info_str;
 	WERROR werr;
 	TALLOC_CTX *temp_ctx = talloc_new(schema);
 	if (temp_ctx == NULL) {
@@ -322,21 +320,9 @@ int dsdb_module_schema_info_update(struct ldb_module *ldb_module,
 	}
 
 	/* finally, update schema_info in the cache */
-	werr = dsdb_blob_from_schema_info(schema_info, temp_ctx, &ndr_blob);
-	if (!W_ERROR_IS_OK(werr)) {
-		ldb_asprintf_errstring(ldb_module_get_ctx(ldb_module), "Failed to get schema info");
-		talloc_free(temp_ctx);
-		return ldb_operr(ldb_module_get_ctx(ldb_module));
-	}
-
-	schema_info_str = hex_encode_talloc(schema, ndr_blob.data, ndr_blob.length);
-	if (!schema_info_str) {
-		talloc_free(temp_ctx);
-		return ldb_module_oom(ldb_module);
-	}
 
-	talloc_unlink(schema, discard_const(schema->schema_info));
-	schema->schema_info = schema_info_str;
+	talloc_unlink(schema, schema->schema_info);
+	schema->schema_info = talloc_move(schema, &schema_info);
 
 	talloc_free(temp_ctx);
 	return LDB_SUCCESS;
diff --git a/source4/dsdb/schema/schema.h b/source4/dsdb/schema/schema.h
index 6c3581b..6543eda 100644
--- a/source4/dsdb/schema/schema.h
+++ b/source4/dsdb/schema/schema.h
@@ -212,7 +212,7 @@ struct dsdb_schema {
 	 * this is the content of the schemaInfo attribute of the
 	 * Schema-Partition head object.
 	 */
-	const char *schema_info;
+	struct dsdb_schema_info *schema_info;
 
 	struct dsdb_attribute *attributes;
 	struct dsdb_class *classes;
diff --git a/source4/dsdb/schema/schema_info_attr.c b/source4/dsdb/schema/schema_info_attr.c
index e8b9fef..4c9b35a 100644
--- a/source4/dsdb/schema/schema_info_attr.c
+++ b/source4/dsdb/schema/schema_info_attr.c
@@ -175,11 +175,13 @@ WERROR dsdb_blob_from_schema_info(const struct dsdb_schema_info *schema_info,
 WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 			    const struct drsuapi_DsReplicaOIDMapping_Ctr *ctr)
 {
-	bool bres;
-	DATA_BLOB blob;
-	char *schema_info_str;
-	struct drsuapi_DsReplicaOIDMapping *mapping;
+	TALLOC_CTX *frame = NULL;
+	DATA_BLOB blob = data_blob_null;
+	struct dsdb_schema_info *schema_info = NULL;
+	const struct drsuapi_DsReplicaOIDMapping *mapping = NULL;
 	WERROR werr;
+	struct GUID_txt_buf lguid;
+	struct GUID_txt_buf rguid;
 
 	/* we should have at least schemaInfo element */
 	if (ctr->num_mappings < 1) {
@@ -197,19 +199,29 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 		return WERR_INVALID_PARAMETER;
 	}
 
-	schema_info_str = hex_encode_talloc(NULL, blob.data, blob.length);
-	W_ERROR_HAVE_NO_MEMORY(schema_info_str);
-
-	bres = strequal(schema->schema_info, schema_info_str);
-	werr = bres ? WERR_OK : WERR_DS_DRA_SCHEMA_MISMATCH;
-	if (1) {
-		DEBUG(0,("dsdb_schema_info_cmp(%s):\nremote[%s]\nlocal[%s]\nts_last_change[%s], metadata_usn[%llu]\n",
-			win_errstr(werr),
-			schema_info_str, schema->schema_info,
-			timestring(schema_info_str, schema->ts_last_change),
-			(unsigned long long)schema->metadata_usn));
+	frame = talloc_stackframe();
+	werr = dsdb_schema_info_from_blob(&blob, frame, &schema_info);
+	if (!W_ERROR_IS_OK(werr)) {
+		TALLOC_FREE(frame);
+		return werr;
+	}
+
+	if (schema->schema_info->revision != schema_info->revision) {
+		werr = WERR_DS_DRA_SCHEMA_MISMATCH;
+	} else {
+		werr = WERR_OK;
 	}
-	talloc_free(schema_info_str);
+
+	DEBUG(0,("dsdb_schema_info_cmp(%s):\nremote[%u:%s]\nlocal[%u:%s]\nts_last_change[%s], metadata_usn[%llu]\n",
+		win_errstr(werr),
+		(unsigned)schema_info->revision,
+		GUID_buf_string(&schema_info->invocation_id, &rguid),
+		(unsigned)schema->schema_info->revision,
+		GUID_buf_string(&schema->schema_info->invocation_id, &lguid),
+		timestring(frame, schema->ts_last_change),
+		(unsigned long long)schema->metadata_usn));
+
+	TALLOC_FREE(frame);
 	return werr;
 }
 
diff --git a/source4/dsdb/schema/schema_init.c b/source4/dsdb/schema/schema_init.c
index 16f213d..c9c55ca 100644
--- a/source4/dsdb/schema/schema_init.c
+++ b/source4/dsdb/schema/schema_init.c
@@ -64,7 +64,11 @@ struct dsdb_schema *dsdb_schema_copy_shallow(TALLOC_CTX *mem_ctx,
 		goto failed;
 	}
 
-	schema_copy->schema_info = talloc_strdup(schema_copy, schema->schema_info);
+	schema_copy->schema_info = talloc(schema_copy, struct dsdb_schema_info);
+	if (!schema_copy->schema_info) {
+		goto failed;
+	}
+	*schema_copy->schema_info = *schema->schema_info;
 
 	/* copy classes and attributes*/
 	for (cls = schema->classes; cls; cls = cls->next) {
@@ -107,8 +111,8 @@ WERROR dsdb_load_prefixmap_from_drsuapi(struct dsdb_schema *schema,
 					const struct drsuapi_DsReplicaOIDMapping_Ctr *ctr)
 {
 	WERROR werr;
-	const char *schema_info;
-	struct dsdb_schema_prefixmap *pfm;
+	struct dsdb_schema_info *schema_info = NULL;
+	struct dsdb_schema_prefixmap *pfm = NULL;
 
 	werr = dsdb_schema_pfm_from_drsuapi_pfm(ctr, true, schema, &pfm, &schema_info);
 	W_ERROR_NOT_OK_RETURN(werr);
@@ -117,7 +121,7 @@ WERROR dsdb_load_prefixmap_from_drsuapi(struct dsdb_schema *schema,
 	talloc_free(schema->prefixmap);
 	schema->prefixmap = pfm;
 
-	talloc_free(discard_const(schema->schema_info));
+	talloc_free(schema->schema_info);
 	schema->schema_info = schema_info;
 
 	return WERR_OK;
@@ -169,8 +173,8 @@ WERROR dsdb_load_oid_mappings_ldb(struct dsdb_schema *schema,
 				  const struct ldb_val *schemaInfo)
 {
 	WERROR werr;
-	const char *schema_info;
-	struct dsdb_schema_prefixmap *pfm;
+	struct dsdb_schema_info *schema_info = NULL;
+	struct dsdb_schema_prefixmap *pfm = NULL;
 	TALLOC_CTX *mem_ctx;
 
 	/* verify schemaInfo blob is valid one */
@@ -192,19 +196,18 @@ WERROR dsdb_load_oid_mappings_ldb(struct dsdb_schema *schema,
 	}
 
 	/* decode schema_info */
-	schema_info = hex_encode_talloc(mem_ctx,
-					schemaInfo->data,
-					schemaInfo->length);
-	if (!schema_info) {
+	werr = dsdb_schema_info_from_blob(schemaInfo, mem_ctx, &schema_info);
+	if (!W_ERROR_IS_OK(werr)) {
+		DEBUG(0, (__location__ " dsdb_schema_info_from_blob failed: %s\n", win_errstr(werr)));
 		talloc_free(mem_ctx);
-		return WERR_NOMEM;
+		return werr;
 	}
 
 	/* store prefixMap and schema_info into cached Schema */
 	talloc_free(schema->prefixmap);
 	schema->prefixmap = talloc_steal(schema, pfm);
 
-	talloc_free(discard_const(schema->schema_info));
+	talloc_free(schema->schema_info);
 	schema->schema_info = talloc_steal(schema, schema_info);
 
 	/* clean up locally allocated mem */
@@ -257,8 +260,8 @@ WERROR dsdb_get_oid_mappings_ldb(const struct dsdb_schema *schema,
 	talloc_free(ctr);
 	W_ERROR_NOT_OK_RETURN(status);
 
-	*schemaInfo = strhex_to_data_blob(mem_ctx, schema->schema_info);
-	W_ERROR_HAVE_NO_MEMORY(schemaInfo->data);
+	status = dsdb_blob_from_schema_info(schema->schema_info, mem_ctx, schemaInfo);
+	W_ERROR_NOT_OK_RETURN(status);
 
 	return WERR_OK;
 }
diff --git a/source4/dsdb/schema/schema_prefixmap.c b/source4/dsdb/schema/schema_prefixmap.c
index 270e6be..c5acf9c 100644
--- a/source4/dsdb/schema/schema_prefixmap.c
+++ b/source4/dsdb/schema/schema_prefixmap.c
@@ -510,7 +510,7 @@ WERROR dsdb_schema_pfm_from_drsuapi_pfm(const struct drsuapi_DsReplicaOIDMapping
 					bool have_schema_info,
 					TALLOC_CTX *mem_ctx,
 					struct dsdb_schema_prefixmap **_pfm,
-					const char **_schema_info)
+					struct dsdb_schema_info **_schema_info)
 {
 	WERROR werr;
 	uint32_t i;
@@ -561,12 +561,12 @@ WERROR dsdb_schema_pfm_from_drsuapi_pfm(const struct drsuapi_DsReplicaOIDMapping
 		 *  but set it here for clarity */
 		i = ctr->num_mappings - 1;
 
-		*_schema_info = hex_encode_talloc(mem_ctx,
-						  ctr->mappings[i].oid.binary_oid,
-						  ctr->mappings[i].oid.length);
-		if (!*_schema_info) {
+		blob = data_blob_const(ctr->mappings[i].oid.binary_oid,
+				       ctr->mappings[i].oid.length);
+		werr = dsdb_schema_info_from_blob(&blob, mem_ctx, _schema_info);
+		if (!W_ERROR_IS_OK(werr)) {
 			talloc_free(pfm);
-			return WERR_NOMEM;
+			return werr;
 		}
 	}
 
@@ -585,7 +585,7 @@ WERROR dsdb_schema_pfm_from_drsuapi_pfm(const struct drsuapi_DsReplicaOIDMapping
  * \param _ctr Out pointer to drsuapi_DsReplicaOIDMapping_Ctr prefix map structure
  */
 WERROR dsdb_drsuapi_pfm_from_schema_pfm(const struct dsdb_schema_prefixmap *pfm,
-					const char *schema_info,
+					const struct dsdb_schema_info *schema_info,
 					TALLOC_CTX *mem_ctx,
 					struct drsuapi_DsReplicaOIDMapping_Ctr **_ctr)
 {
@@ -628,14 +628,16 @@ WERROR dsdb_drsuapi_pfm_from_schema_pfm(const struct dsdb_schema_prefixmap *pfm,
 
 	/* make schema_info entry if needed */
 	if (schema_info) {
+		WERROR werr;
+
 		/* by this time, i should have this value,
 		 *  but set it here for clarity */
 		i = ctr->num_mappings - 1;
 
-		blob = strhex_to_data_blob(ctr, schema_info);
-		if (!blob.data) {
+		werr = dsdb_blob_from_schema_info(schema_info, ctr, &blob);
+		if (!W_ERROR_IS_OK(werr)) {
 			talloc_free(ctr);
-			return WERR_NOMEM;
+			return werr;
 		}
 
 		ctr->mappings[i].id_prefix = 0;
diff --git a/source4/torture/drs/unit/prefixmap_tests.c b/source4/torture/drs/unit/prefixmap_tests.c
index 29941eb..969d641 100644
--- a/source4/torture/drs/unit/prefixmap_tests.c
+++ b/source4/torture/drs/unit/prefixmap_tests.c
@@ -26,7 +26,7 @@
 #include "torture/rpc/drsuapi.h"
 #include "torture/drs/proto.h"
 #include "param/param.h"
-
+#include "librpc/ndr/libndr.h"
 
 /**
  * Private data to be shared among all test in Test case
@@ -36,7 +36,6 @@ struct drsut_prefixmap_data {
 	struct dsdb_schema_prefixmap *pfm_full;
 
 	/* default schemaInfo value to test with */
-	const char *schi_default_str;
 	struct dsdb_schema_info *schi_default;
 
 	struct ldb_context *ldb_ctx;
@@ -539,7 +538,8 @@ static bool torture_drs_unit_pfm_oid_from_attid_check_attid(struct torture_conte
 static bool torture_drs_unit_pfm_to_from_drsuapi(struct torture_context *tctx, struct drsut_prefixmap_data *priv)
 {
 	WERROR werr;
-	const char *schema_info;
+	struct dsdb_schema_info *schema_info;
+	DATA_BLOB schema_info_blob;
 	struct dsdb_schema_prefixmap *pfm;
 	struct drsuapi_DsReplicaOIDMapping_Ctr *ctr;
 	TALLOC_CTX *mem_ctx;
@@ -548,19 +548,20 @@ static bool torture_drs_unit_pfm_to_from_drsuapi(struct torture_context *tctx, s
 	torture_assert(tctx, mem_ctx, "Unexpected: Have no memory!");
 
 	/* convert Schema_prefixMap to drsuapi_prefixMap */
-	werr = dsdb_drsuapi_pfm_from_schema_pfm(priv->pfm_full, priv->schi_default_str, mem_ctx, &ctr);
+	werr = dsdb_drsuapi_pfm_from_schema_pfm(priv->pfm_full, priv->schi_default, mem_ctx, &ctr);
 	torture_assert_werr_ok(tctx, werr, "dsdb_drsuapi_pfm_from_schema_pfm() failed");
 	torture_assert(tctx, ctr && ctr->mappings, "drsuapi_prefixMap not constructed correctly");
 	torture_assert_int_equal(tctx, ctr->num_mappings, priv->pfm_full->length + 1,
 				 "drs_mappings count does not match");
 	/* look for schema_info entry - it should be the last one */
-	schema_info = hex_encode_talloc(mem_ctx,
-					ctr->mappings[ctr->num_mappings - 1].oid.binary_oid,
-					ctr->mappings[ctr->num_mappings - 1].oid.length);
-	torture_assert_str_equal(tctx,
-				 schema_info,
-				 priv->schi_default_str,
-				 "schema_info not stored correctly or not last entry");
+	schema_info_blob = data_blob_const(ctr->mappings[ctr->num_mappings - 1].oid.binary_oid,
+					   ctr->mappings[ctr->num_mappings - 1].oid.length);
+	werr = dsdb_schema_info_from_blob(&schema_info_blob, tctx, &schema_info);
+	torture_assert_werr_ok(tctx, werr, "dsdb_schema_info_from_blob failed");
+	torture_assert_int_equal(tctx, schema_info->revision,  priv->schi_default->revision,
+				 "schema_info (revision) not stored correctly or not last entry");
+	torture_assert(tctx, GUID_equal(&schema_info->invocation_id, &priv->schi_default->invocation_id),
+		       "schema_info (invocation_id) not stored correctly or not last entry");
 
 	/* compare schema_prefixMap and drsuapi_prefixMap */
 	werr = dsdb_schema_pfm_contains_drsuapi_pfm(priv->pfm_full, ctr);
@@ -569,7 +570,10 @@ static bool torture_drs_unit_pfm_to_from_drsuapi(struct torture_context *tctx, s
 	/* convert back drsuapi_prefixMap to schema_prefixMap */
 	werr = dsdb_schema_pfm_from_drsuapi_pfm(ctr, true, mem_ctx, &pfm, &schema_info);
 	torture_assert_werr_ok(tctx, werr, "dsdb_schema_pfm_from_drsuapi_pfm() failed");
-	torture_assert_str_equal(tctx, schema_info, priv->schi_default_str, "Fetched schema_info is different");
+	torture_assert_int_equal(tctx, schema_info->revision,  priv->schi_default->revision,
+				 "Fetched schema_info is different (revision)");
+	torture_assert(tctx, GUID_equal(&schema_info->invocation_id, &priv->schi_default->invocation_id),
+		       "Fetched schema_info is different (invocation_id)");
 
 	/* compare against the original */
 	if (!_torture_drs_pfm_compare_same(tctx, priv->pfm_full, pfm, true)) {
@@ -599,7 +603,6 @@ static bool torture_drs_unit_pfm_to_from_drsuapi(struct torture_context *tctx, s
 static bool torture_drs_unit_pfm_to_from_ldb_val(struct torture_context *tctx, struct drsut_prefixmap_data *priv)
 {
 	WERROR werr;
-	const char *schema_info;
 	struct dsdb_schema *schema;
 	struct ldb_val pfm_ldb_val;
 	struct ldb_val schema_info_ldb_val;
@@ -613,7 +616,7 @@ static bool torture_drs_unit_pfm_to_from_ldb_val(struct torture_context *tctx, s
 
 	/* set priv->pfm_full as prefixMap for new schema object */
 	schema->prefixmap = priv->pfm_full;
-	schema->schema_info = priv->schi_default_str;
+	schema->schema_info = priv->schi_default;
 
 	/* convert schema_prefixMap to ldb_val blob */
 	werr = dsdb_get_oid_mappings_ldb(schema, mem_ctx, &pfm_ldb_val, &schema_info_ldb_val);
@@ -622,14 +625,6 @@ static bool torture_drs_unit_pfm_to_from_ldb_val(struct torture_context *tctx, s
 		       "pfm_ldb_val not constructed correctly");
 	torture_assert(tctx, schema_info_ldb_val.data && schema_info_ldb_val.length,
 		       "schema_info_ldb_val not constructed correctly");
-	/* look for schema_info entry - it should be the last one */
-	schema_info = hex_encode_talloc(mem_ctx,
-					schema_info_ldb_val.data,
-					schema_info_ldb_val.length);
-	torture_assert_str_equal(tctx,
-				 schema_info,
-				 priv->schi_default_str,
-				 "schema_info not stored correctly or not last entry");
 
 	/* convert pfm_ldb_val back to schema_prefixMap */
 	schema->prefixmap = NULL;
@@ -641,6 +636,10 @@ static bool torture_drs_unit_pfm_to_from_ldb_val(struct torture_context *tctx, s
 		talloc_free(mem_ctx);
 		return false;
 	}
+	torture_assert_int_equal(tctx, schema->schema_info->revision, priv->schi_default->revision,
+				 "Fetched schema_info is different (revision)");
+	torture_assert(tctx, GUID_equal(&schema->schema_info->invocation_id, &priv->schi_default->invocation_id),
+		       "Fetched schema_info is different (invocation_id)");
 
 	talloc_free(mem_ctx);
 	return true;
@@ -664,7 +663,7 @@ static bool torture_drs_unit_pfm_read_write_ldb(struct torture_context *tctx, st
 	torture_assert(tctx, schema, "Unexpected: failed to allocate schema object");
 	/* set priv->pfm_full as prefixMap for new schema object */
 	schema->prefixmap = priv->pfm_full;
-	schema->schema_info = priv->schi_default_str;
+	schema->schema_info = priv->schi_default;
 
 	/* write prfixMap to ldb */
 	werr = dsdb_write_prefixes_from_schema_to_ldb(mem_ctx, priv->ldb_ctx, schema);
@@ -701,7 +700,7 @@ static bool torture_drs_unit_dsdb_create_prefix_mapping(struct torture_context *
 	schema = dsdb_new_schema(mem_ctx);
 	torture_assert(tctx, schema, "Unexpected: failed to allocate schema object");
 	/* set priv->pfm_full as prefixMap for new schema object */
-	schema->schema_info = priv->schi_default_str;
+	schema->schema_info = priv->schi_default;
 	werr = _drsut_prefixmap_new(_prefixmap_test_new_data, ARRAY_SIZE(_prefixmap_test_new_data),
 				    schema, &schema->prefixmap);
 	torture_assert_werr_ok(tctx, werr, "_drsut_prefixmap_new() failed");
@@ -827,8 +826,6 @@ static bool torture_drs_unit_prefixmap_setup(struct torture_context *tctx, struc
 	werr = dsdb_blob_from_schema_info(priv->schi_default, priv, &blob);
 	torture_assert_werr_ok(tctx, werr, "dsdb_blob_from_schema_info() failed");
 
-	priv->schi_default_str = data_blob_hex_string_upper(priv, &blob);
-
 	/* create temporary LDB and populate with data */
 	if (!torture_drs_unit_ldb_setup(tctx, priv)) {
 		return false;
diff --git a/source4/torture/drs/unit/schemainfo_tests.c b/source4/torture/drs/unit/schemainfo_tests.c
index 2fb5308..3b492b8 100644
--- a/source4/torture/drs/unit/schemainfo_tests.c
+++ b/source4/torture/drs/unit/schemainfo_tests.c
@@ -304,6 +304,7 @@ static bool test_dsdb_schema_info_cmp(struct torture_context *tctx,
 {
 	DATA_BLOB blob;
 	struct drsuapi_DsReplicaOIDMapping_Ctr *ctr;
+	struct dsdb_schema_info schema_info;
 
 	ctr = talloc_zero(priv, struct drsuapi_DsReplicaOIDMapping_Ctr);
 	torture_assert(tctx, ctr, "Not enough memory!");
@@ -354,8 +355,10 @@ static bool test_dsdb_schema_info_cmp(struct torture_context *tctx,
 				  "dsdb_schema_info_cmp(): unexpected result");
 
 	/* test with correct schemaInfo, but invalid ATTID */
-	blob = strhex_to_data_blob(ctr, priv->schema->schema_info);
-	torture_assert(tctx, blob.data, "Not enough memory!");
+	schema_info = *priv->schema->schema_info;
+	torture_assert_werr_ok(tctx,
+		dsdb_blob_from_schema_info(&schema_info, tctx, &blob),
+		"dsdb_blob_from_schema_info() failed");
 	ctr->mappings[0].id_prefix	= 1;
 	ctr->mappings[0].oid.length     = blob.length;
 	ctr->mappings[0].oid.binary_oid = blob.data;
@@ -365,7 +368,6 @@ static bool test_dsdb_schema_info_cmp(struct torture_context *tctx,
 				  "dsdb_schema_info_cmp(): unexpected result");
 
 	/* test with valid schemaInfo */
-	blob = strhex_to_data_blob(ctr, priv->schema->schema_info);
 	ctr->mappings[0].id_prefix	= 0;
 	torture_assert_werr_ok(tctx,
 			       dsdb_schema_info_cmp(priv->schema, ctr),
@@ -595,7 +597,9 @@ static bool torture_drs_unit_schemainfo_setup(struct torture_context *tctx,
 	priv->schema = dsdb_new_schema(priv);
 
 	/* set schema_info in dsdb_schema for testing */
-	priv->schema->schema_info = talloc_strdup(priv->schema, SCHEMA_INFO_DEFAULT_STR);
+	torture_assert(tctx,
+		       _drsut_schemainfo_new(tctx, SCHEMA_INFO_DEFAULT_STR, &priv->schema->schema_info),
+		       "Failed to create schema_info test object");
 
 	/* pre-cache invocationId for samdb_ntds_invocation_id()
 	 * to work with our mock ldb */
-- 
1.9.1


From e073efe334731dfc18afa6fe23d16bac5db9a753 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 4 Aug 2016 11:00:34 +0200
Subject: [PATCH 6/8] struct dsdb_schema_info part 2

---
 source4/dsdb/schema/schema_info_attr.c      | 11 ++++-
 source4/torture/drs/unit/schemainfo_tests.c | 71 ++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 3 deletions(-)

diff --git a/source4/dsdb/schema/schema_info_attr.c b/source4/dsdb/schema/schema_info_attr.c
index 4c9b35a..81be295 100644
--- a/source4/dsdb/schema/schema_info_attr.c
+++ b/source4/dsdb/schema/schema_info_attr.c
@@ -206,8 +206,17 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 		return werr;
 	}
 
-	if (schema->schema_info->revision != schema_info->revision) {
+	if (schema->schema_info->revision > schema_info->revision) {
+		/*
+		 * It's ok if our schema is newer than the remote one
+		 */
+		werr = WERR_OK;
+	} else if (schema->schema_info->revision < schema_info->revision) {
 		werr = WERR_DS_DRA_SCHEMA_MISMATCH;
+	} else if (!GUID_equal(&schema->schema_info->invocation_id,
+		   &schema_info->invocation_id))
+	{
+		werr = WERR_DS_DRA_SCHEMA_CONFLICT;
 	} else {
 		werr = WERR_OK;
 	}
diff --git a/source4/torture/drs/unit/schemainfo_tests.c b/source4/torture/drs/unit/schemainfo_tests.c
index 3b492b8..e5a1c87 100644
--- a/source4/torture/drs/unit/schemainfo_tests.c
+++ b/source4/torture/drs/unit/schemainfo_tests.c
@@ -344,14 +344,14 @@ static bool test_dsdb_schema_info_cmp(struct torture_context *tctx,
 				  WERR_INVALID_PARAMETER,
 				  "dsdb_schema_info_cmp(): unexpected result");
 
-	/* test with valid schemaInfo, but not correct one */
+	/* test with valid schemaInfo, but older one should be ok */
 	blob = strhex_to_data_blob(ctr, "FF0000000000000000000000000000000000000000");
 	torture_assert(tctx, blob.data, "Not enough memory!");
 	ctr->mappings[0].oid.length     = blob.length;
 	ctr->mappings[0].oid.binary_oid = blob.data;
 	torture_assert_werr_equal(tctx,
 				  dsdb_schema_info_cmp(priv->schema, ctr),
-				  WERR_DS_DRA_SCHEMA_MISMATCH,
+				  WERR_OK,
 				  "dsdb_schema_info_cmp(): unexpected result");
 
 	/* test with correct schemaInfo, but invalid ATTID */
@@ -373,6 +373,73 @@ static bool test_dsdb_schema_info_cmp(struct torture_context *tctx,
 			       dsdb_schema_info_cmp(priv->schema, ctr),
 			       "dsdb_schema_info_cmp(): unexpected result");
 
+	/* test with valid schemaInfo, but older revision */
+	schema_info = *priv->schema->schema_info;
+	schema_info.revision -= 1;
+	torture_assert_werr_ok(tctx,
+		dsdb_blob_from_schema_info(&schema_info, tctx, &blob),
+		"dsdb_blob_from_schema_info() failed");
+	ctr->mappings[0].oid.length     = blob.length;
+	ctr->mappings[0].oid.binary_oid = blob.data;
+	torture_assert_werr_equal(tctx,
+				  dsdb_schema_info_cmp(priv->schema, ctr),
+				  WERR_OK,
+				  "dsdb_schema_info_cmp(): unexpected result");
+
+	/* test with valid schemaInfo, but newer revision */
+	schema_info = *priv->schema->schema_info;
+	schema_info.revision += 1;
+	torture_assert_werr_ok(tctx,
+		dsdb_blob_from_schema_info(&schema_info, tctx, &blob),
+		"dsdb_blob_from_schema_info() failed");
+	ctr->mappings[0].oid.length     = blob.length;
+	ctr->mappings[0].oid.binary_oid = blob.data;
+	torture_assert_werr_equal(tctx,
+				  dsdb_schema_info_cmp(priv->schema, ctr),
+				  WERR_DS_DRA_SCHEMA_MISMATCH,
+				  "dsdb_schema_info_cmp(): unexpected result");
+
+	/* test with valid schemaInfo, but newer revision and other invocationId */
+	schema_info = *priv->schema->schema_info;
+	schema_info.revision += 1;
+	schema_info.invocation_id.time_mid += 1;
+	torture_assert_werr_ok(tctx,
+		dsdb_blob_from_schema_info(&schema_info, tctx, &blob),
+		"dsdb_blob_from_schema_info() failed");
+	ctr->mappings[0].oid.length     = blob.length;
+	ctr->mappings[0].oid.binary_oid = blob.data;
+	torture_assert_werr_equal(tctx,
+				  dsdb_schema_info_cmp(priv->schema, ctr),
+				  WERR_DS_DRA_SCHEMA_MISMATCH,
+				  "dsdb_schema_info_cmp(): unexpected result");
+
+	/* test with valid schemaInfo, but older revision and other invocationId */
+	schema_info = *priv->schema->schema_info;
+	schema_info.revision -= 1;
+	schema_info.invocation_id.time_mid += 1;
+	torture_assert_werr_ok(tctx,
+		dsdb_blob_from_schema_info(&schema_info, tctx, &blob),
+		"dsdb_blob_from_schema_info() failed");
+	ctr->mappings[0].oid.length     = blob.length;
+	ctr->mappings[0].oid.binary_oid = blob.data;
+	torture_assert_werr_equal(tctx,
+				  dsdb_schema_info_cmp(priv->schema, ctr),
+				  WERR_OK,
+				  "dsdb_schema_info_cmp(): unexpected result");
+
+	/* test with valid schemaInfo, but same revision and other invocationId */
+	schema_info = *priv->schema->schema_info;
+	schema_info.invocation_id.time_mid += 1;
+	torture_assert_werr_ok(tctx,
+		dsdb_blob_from_schema_info(&schema_info, tctx, &blob),
+		"dsdb_blob_from_schema_info() failed");
+	ctr->mappings[0].oid.length     = blob.length;
+	ctr->mappings[0].oid.binary_oid = blob.data;
+	torture_assert_werr_equal(tctx,
+				  dsdb_schema_info_cmp(priv->schema, ctr),
+				  WERR_DS_DRA_SCHEMA_CONFLICT,
+				  "dsdb_schema_info_cmp(): unexpected result");
+
 	talloc_free(ctr);
 	return true;
 }
-- 
1.9.1


From 08393cb1891d78db3179e07786b16636d435ac94 Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 4 Aug 2016 15:50:52 +0200
Subject: [PATCH 7/8] more debug

---
 source4/dsdb/repl/drepl_out_helpers.c              |  7 ++-
 source4/dsdb/repl/replicated_objects.c             |  3 ++
 .../dsdb/samdb/ldb_modules/partition_metadata.c    |  4 ++
 source4/dsdb/samdb/ldb_modules/schema_load.c       |  4 +-
 source4/dsdb/samdb/ldb_modules/schema_util.c       |  1 +
 source4/dsdb/schema/schema.h                       |  3 +-
 source4/dsdb/schema/schema_info_attr.c             | 52 +++++++++++++++++++---
 source4/dsdb/schema/schema_init.c                  |  5 ++-
 source4/dsdb/schema/schema_set.c                   |  5 +++
 source4/rpc_server/drsuapi/getncchanges.c          |  4 ++
 10 files changed, 77 insertions(+), 11 deletions(-)

diff --git a/source4/dsdb/repl/drepl_out_helpers.c b/source4/dsdb/repl/drepl_out_helpers.c
index d0a822d..fb7841b 100644
--- a/source4/dsdb/repl/drepl_out_helpers.c
+++ b/source4/dsdb/repl/drepl_out_helpers.c
@@ -686,7 +686,7 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req
 	struct dreplsrv_partition *partition = state->op->source_dsa->partition;
 	struct dreplsrv_drsuapi_connection *drsuapi = state->op->source_dsa->conn->drsuapi;
 	struct ldb_dn *schema_dn = ldb_get_schema_basedn(service->samdb);
-	struct dsdb_schema *schema;
+	struct dsdb_schema *schema = NULL;
 	struct dsdb_schema *working_schema = NULL;
 	const struct drsuapi_DsReplicaOIDMapping_Ctr *mapping_ctr;
 	uint32_t object_count;
@@ -733,6 +733,9 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req
 		return;
 	}
 
+	dsdb_schema_info_debug(schema, "before_get_schema", __location__, __func__);
+	NDR_PRINT_DEBUG(drsuapi_DsReplicaOIDMapping_Ctr, discard_const(mapping_ctr));
+
 	schema = dsdb_get_schema(service->samdb, state);
 	if (!schema) {
 		DEBUG(0,(__location__ ": Schema is not loaded yet!\n"));
@@ -740,6 +743,8 @@ static void dreplsrv_op_pull_source_apply_changes_trigger(struct tevent_req *req
 		return;
 	}
 
+	dsdb_schema_info_debug(schema, "schema", __location__, __func__);
+
 	/*
 	 * Decide what working schema to use for object conversion.
 	 * We won't need a working schema for empty replicas sent.
diff --git a/source4/dsdb/repl/replicated_objects.c b/source4/dsdb/repl/replicated_objects.c
index 4aa319b..178642f 100644
--- a/source4/dsdb/repl/replicated_objects.c
+++ b/source4/dsdb/repl/replicated_objects.c
@@ -778,6 +778,7 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
 			TALLOC_FREE(tmp_ctx);
 			return WERR_INTERNAL_ERROR;
 		}
+		dsdb_schema_info_debug(working_schema, "working_schema", __location__, __func__);
 	}
 
 	ret = ldb_extended(ldb, DSDB_EXTENDED_REPLICATED_OBJECTS_OID, objects, &ext_res);
@@ -913,6 +914,8 @@ WERROR dsdb_replicated_objects_commit(struct ldb_context *ldb,
 		} else if (used_global_schema) {
 			dsdb_make_schema_global(ldb, new_schema);
 		}
+		dsdb_schema_info_debug(working_schema, "working_schema", __location__, __func__);
+		dsdb_schema_info_debug(new_schema, "new_schema", __location__, __func__);
 	}
 
 	DEBUG(0,("Replicated %u objects (%u linked attributes) for %s\n",
diff --git a/source4/dsdb/samdb/ldb_modules/partition_metadata.c b/source4/dsdb/samdb/ldb_modules/partition_metadata.c
index 3c5180b..e6c4682 100644
--- a/source4/dsdb/samdb/ldb_modules/partition_metadata.c
+++ b/source4/dsdb/samdb/ldb_modules/partition_metadata.c
@@ -165,6 +165,10 @@ int partition_metadata_inc_schema_sequence(struct ldb_module *module)
 		return ret;
 	}
 
+	DEBUG(0, ("%s:%s: PID[%d] PPID[%d] update %s from %llu to %llu\n",
+		  __location__, __func__, getpid(), getppid(), DSDB_METADATA_SCHEMA_SEQ_NUM,
+		  (unsigned long long)value,
+		  (unsigned long long)value+1));
 	value++;
 	ret = partition_metadata_set_uint64(module, DSDB_METADATA_SCHEMA_SEQ_NUM, value, false);
 	if (ret == LDB_ERR_OPERATIONS_ERROR) {
diff --git a/source4/dsdb/samdb/ldb_modules/schema_load.c b/source4/dsdb/samdb/ldb_modules/schema_load.c
index 6f15a3b..37be0d8 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_load.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_load.c
@@ -221,7 +221,7 @@ static struct dsdb_schema *dsdb_schema_refresh(struct ldb_module *module, struct
 				TALLOC_FREE(mem_ctx);
 				return schema;
 			} else {
-				DEBUG(3, ("Schema refresh needed %lld != %lld\n",
+				DEBUG(0, ("Schema refresh needed %lld != %lld\n",
 					  (unsigned long long)schema->metadata_usn,
 					  (unsigned long long)schema_seq_num));
 			}
@@ -340,6 +340,8 @@ static int dsdb_schema_from_db(struct ldb_module *module,
 
 	(*schema)->metadata_usn = schema_seq_num;
 
+	dsdb_schema_info_debug((*schema), "loaded_schema", __location__, __func__);
+
 	talloc_steal(mem_ctx, *schema);
 
 failed:
diff --git a/source4/dsdb/samdb/ldb_modules/schema_util.c b/source4/dsdb/samdb/ldb_modules/schema_util.c
index 027487b..0b6f8d6 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_util.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_util.c
@@ -323,6 +323,7 @@ int dsdb_module_schema_info_update(struct ldb_module *ldb_module,
 
 	talloc_unlink(schema, schema->schema_info);
 	schema->schema_info = talloc_move(schema, &schema_info);
+	schema->last_update = timeval_current();
 
 	talloc_free(temp_ctx);
 	return LDB_SUCCESS;
diff --git a/source4/dsdb/schema/schema.h b/source4/dsdb/schema/schema.h
index 6543eda..955e350 100644
--- a/source4/dsdb/schema/schema.h
+++ b/source4/dsdb/schema/schema.h
@@ -246,8 +246,9 @@ struct dsdb_schema {
 	} fsmo;
 
 	/* Was this schema loaded from ldb (if so, then we will reload it when we detect a change in ldb) */
-	bool refresh_in_progress;
 	time_t ts_last_change;
+	struct timeval last_load;
+	struct timeval last_update;
 	/* This 'opaque' is stored in the metadata and is used to check if the currently
 	 * loaded schema needs a reload because another process has signaled that it has been
 	 * requested to reload the schema (either due through DRS or via the schemaUpdateNow).
diff --git a/source4/dsdb/schema/schema_info_attr.c b/source4/dsdb/schema/schema_info_attr.c
index 81be295..61c62e9 100644
--- a/source4/dsdb/schema/schema_info_attr.c
+++ b/source4/dsdb/schema/schema_info_attr.c
@@ -27,6 +27,7 @@
 #include "librpc/gen_ndr/ndr_drsuapi.h"
 #include "librpc/gen_ndr/ndr_drsblobs.h"
 #include "param/param.h"
+#include "lib/util/time_basic.h"
 
 
 /**
@@ -164,6 +165,43 @@ WERROR dsdb_blob_from_schema_info(const struct dsdb_schema_info *schema_info,
 	return WERR_OK;
 }
 
+void dsdb_schema_info_debug(const struct dsdb_schema *schema, const char *name, const char *location, const char *fn)
+{
+	struct GUID_txt_buf lguid;
+	struct timeval tv_last_change = { .tv_sec = 0, };
+	struct timeval_buf tb_last_change;
+	struct timeval_buf tb_last_load;
+	struct timeval_buf tb_last_update;
+	struct timeval tv_current = timeval_current();
+	struct timeval_buf tb_current;
+
+	if (schema == NULL) {
+		DEBUG(0,("%s:%s:%s:%p PID[%d] PPID[%d] NULL\n",
+			location, fn, name, schema, getpid(), getppid()));
+		return;
+	}
+
+	if (schema->schema_info == NULL) {
+		DEBUG(0,("%s:%s:%s:%p PID[%d] PPID[%d] NULL schema_info!!!!\n",
+			location, fn, name, schema, getpid(), getppid()));
+		return;
+	}
+
+	tv_last_change.tv_sec = schema->ts_last_change;
+
+	DEBUG(0,("%s:%s:%s:%p PID[%d] PPID[%d] revision%u invocation:%s metadata_usn[%llu] change[%s] update[%s] load[%s] now[%s] master[%s] updates allowed[%s]\n",
+		location, fn, name, schema, getpid(), getppid(),
+		(unsigned)schema->schema_info->revision,
+		GUID_buf_string(&schema->schema_info->invocation_id, &lguid),
+		(unsigned long long)schema->metadata_usn,
+		timeval_str_buf(&tv_last_change, false, true, &tb_last_change),
+		timeval_str_buf(&schema->last_load, false, true, &tb_last_load),
+		timeval_str_buf(&schema->last_update, false, true, &tb_last_update),
+		timeval_str_buf(&tv_current, false, true, &tb_current),
+		(schema->fsmo.we_are_master?"yes":"no"),
+		(schema->fsmo.update_allowed?"yes":"no")));
+}
+
 /**
  * Compares schemaInfo signatures in dsdb_schema and prefixMap.
  * NOTE: At present function compares schemaInfo values
@@ -183,6 +221,7 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 	struct GUID_txt_buf lguid;
 	struct GUID_txt_buf rguid;
 
+
 	/* we should have at least schemaInfo element */
 	if (ctr->num_mappings < 1) {
 		return WERR_INVALID_PARAMETER;
@@ -201,8 +240,8 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 
 	frame = talloc_stackframe();
 	werr = dsdb_schema_info_from_blob(&blob, frame, &schema_info);
+	TALLOC_FREE(frame);
 	if (!W_ERROR_IS_OK(werr)) {
-		TALLOC_FREE(frame);
 		return werr;
 	}
 
@@ -221,16 +260,15 @@ WERROR dsdb_schema_info_cmp(const struct dsdb_schema *schema,
 		werr = WERR_OK;
 	}
 
-	DEBUG(0,("dsdb_schema_info_cmp(%s):\nremote[%u:%s]\nlocal[%u:%s]\nts_last_change[%s], metadata_usn[%llu]\n",
-		win_errstr(werr),
+	dsdb_schema_info_debug(schema, "local_schema", __location__, __func__);
+	DEBUG(0,("PID[%d] PPID[%d] dsdb_schema_info_cmp(%s):\nremote[%u:%s]\nlocal[%u:%s]\n",
+		getpid(), getppid(), win_errstr(werr),
 		(unsigned)schema_info->revision,
 		GUID_buf_string(&schema_info->invocation_id, &rguid),
 		(unsigned)schema->schema_info->revision,
-		GUID_buf_string(&schema->schema_info->invocation_id, &lguid),
-		timestring(frame, schema->ts_last_change),
-		(unsigned long long)schema->metadata_usn));
+		GUID_buf_string(&schema->schema_info->invocation_id, &lguid)));
+	NDR_PRINT_DEBUG(drsuapi_DsReplicaOIDMapping_Ctr, discard_const(ctr));
 
-	TALLOC_FREE(frame);
 	return werr;
 }
 
diff --git a/source4/dsdb/schema/schema_init.c b/source4/dsdb/schema/schema_init.c
index c9c55ca..06db205 100644
--- a/source4/dsdb/schema/schema_init.c
+++ b/source4/dsdb/schema/schema_init.c
@@ -985,7 +985,10 @@ int dsdb_schema_from_ldb_results(TALLOC_CTX *mem_ctx, struct ldb_context *ldb,
 		schema->fsmo.we_are_master = false;
 	}
 
-	DEBUG(5, ("schema_fsmo_init: we are master[%s] updates allowed[%s]\n",
+	schema->last_load = timeval_current();
+
+	dsdb_schema_info_debug(schema, "reloaded_schema", __location__, __func__);
+	DEBUG(0, ("schema_fsmo_init: we are master[%s] updates allowed[%s]\n",
 		  (schema->fsmo.we_are_master?"yes":"no"),
 		  (schema->fsmo.update_allowed?"yes":"no")));
 
diff --git a/source4/dsdb/schema/schema_set.c b/source4/dsdb/schema/schema_set.c
index 1b29c4d..96f0cd1 100644
--- a/source4/dsdb/schema/schema_set.c
+++ b/source4/dsdb/schema/schema_set.c
@@ -498,9 +498,11 @@ int dsdb_set_schema(struct ldb_context *ldb, struct dsdb_schema *schema)
 	 * none, NULL is harmless here.
 	 */
 	if (old_schema != schema) {
+		dsdb_schema_info_debug(old_schema, "old_schema", __location__, __func__);
 		talloc_unlink(ldb, old_schema);
 	}
 
+	dsdb_schema_info_debug(schema, "schema", __location__, __func__);
 	return ret;
 }
 
@@ -556,6 +558,7 @@ int dsdb_reference_schema(struct ldb_context *ldb, struct dsdb_schema *schema,
 		return ret;
 	}
 
+	dsdb_schema_info_debug(schema, "schema", __location__, __func__);
 	return LDB_SUCCESS;
 }
 
@@ -673,12 +676,14 @@ void dsdb_make_schema_global(struct ldb_context *ldb, struct dsdb_schema *schema
 	}
 
 	if (global_schema) {
+		dsdb_schema_info_debug(global_schema, "old_global_schema", __location__, __func__);
 		talloc_unlink(talloc_autofree_context(), global_schema);
 	}
 
 	/* we want the schema to be around permanently */
 	talloc_reparent(ldb, talloc_autofree_context(), schema);
 	global_schema = schema;
+	dsdb_schema_info_debug(global_schema, "global_schema", __location__, __func__);
 
 	/* This calls the talloc_reference() of the global schema back onto the ldb */
 	dsdb_set_global_schema(ldb);
diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index f002836..4ab47343 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -1990,6 +1990,7 @@ allowed:
 		DEBUG(0,("No schema in sam_ctx\n"));
 		return WERR_DS_DRA_INTERNAL_ERROR;
 	}
+	dsdb_schema_info_debug(schema, "schema", __location__, __func__);
 
 	r->out.ctr->ctr6.naming_context = talloc(mem_ctx, struct drsuapi_DsReplicaObjectIdentifier);
 	*r->out.ctr->ctr6.naming_context = *ncRoot;
@@ -2007,6 +2008,9 @@ allowed:
 	dsdb_get_oid_mappings_drsuapi(schema, true, mem_ctx, &ctr);
 	r->out.ctr->ctr6.mapping_ctr = *ctr;
 
+	dsdb_schema_info_debug(schema, "dsdb_get_oid_mappings_drsuapi schema", __location__, __func__);
+	NDR_PRINT_DEBUG(drsuapi_DsReplicaOIDMapping_Ctr, ctr);
+
 	r->out.ctr->ctr6.source_dsa_guid = *(samdb_ntds_objectGUID(sam_ctx));
 	r->out.ctr->ctr6.source_dsa_invocation_id = *(samdb_ntds_invocation_id(sam_ctx));
 
-- 
1.9.1


From 4d6c3c2317ee2500ae9f09663995ff6d1ca3f8be Mon Sep 17 00:00:00 2001
From: Stefan Metzmacher <metze at samba.org>
Date: Thu, 4 Aug 2016 23:04:32 +0200
Subject: [PATCH 8/8] don't update schema->schema_info without a reload!

---
 source4/dsdb/samdb/ldb_modules/schema_util.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/source4/dsdb/samdb/ldb_modules/schema_util.c b/source4/dsdb/samdb/ldb_modules/schema_util.c
index 0b6f8d6..84959fc 100644
--- a/source4/dsdb/samdb/ldb_modules/schema_util.c
+++ b/source4/dsdb/samdb/ldb_modules/schema_util.c
@@ -321,8 +321,11 @@ int dsdb_module_schema_info_update(struct ldb_module *ldb_module,
 
 	/* finally, update schema_info in the cache */
 
-	talloc_unlink(schema, schema->schema_info);
-	schema->schema_info = talloc_move(schema, &schema_info);
+	/*
+	 * We don't update the schema->schema_info!
+	 * as that would not represent the other information
+	 * in schema->*
+	 */
 	schema->last_update = timeval_current();
 
 	talloc_free(temp_ctx);
-- 
1.9.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20160804/580d119c/signature.sig>


More information about the samba-technical mailing list