[PATCH] DRS refactors and support for sending links interleaved with objects

Tim Beale timbeale at catalyst.net.nz
Wed Sep 13 23:22:36 UTC 2017


Hi,

Attached are some patches that are the precursor work to supporting
GET_TGT on the server-side. Most of the patches just refactor code and
don't affect functionality. The top patch adds a config option that
allows the linked attributes to be sent interleaved with the source
objects, instead of at the end of replication. This behaviour is turned
off by default.

They've been reviewed by Andrew. Can I get one more review?
git.catalyst.net.nz/gw?p=samba.git;a=shortlog;h=refs/heads/tim-tgt-for-list

Thanks,
Tim
-------------- next part --------------
From 2d73095058006e73d858e8374b512d1b21c0bff4 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Fri, 2 Jun 2017 14:42:34 +1200
Subject: [PATCH 01/13] getncchanges.c: Rename anc_cache to obj_cache

When we add GET_TGT support we will reuse the ancestor cache and it
should work the same way - if we've already sent an object because it
was needed for resolving a child object or a link target, then there's
no point sending it again.

This just renames anc_cache --> obj_cache.

An extra is_get_anc flag has been added to getnc_state - once GET_TGT
support is added, we can't assume GET_ANC based solely on the existence
of the obj_cache.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 41 ++++++++++++++++++++-----------
 1 file changed, 27 insertions(+), 14 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index cc41abb..7acab66 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -47,13 +47,14 @@
 
 /* state of a partially completed getncchanges call */
 struct drsuapi_getncchanges_state {
-	struct db_context *anc_cache;
+	struct db_context *obj_cache;
 	struct GUID *guids;
 	uint32_t num_records;
 	uint32_t num_processed;
 	struct ldb_dn *ncRoot_dn;
 	struct GUID ncRoot_guid;
 	bool is_schema_nc;
+	bool is_get_anc;
 	uint64_t min_usn;
 	uint64_t max_usn;
 	struct drsuapi_DsReplicaHighWaterMark last_hwm;
@@ -1935,7 +1936,12 @@ static void dcesrv_drsuapi_update_highwatermark(const struct ldb_message *msg,
 	hwm->reserved_usn = 0;
 }
 
-static WERROR dcesrv_drsuapi_anc_cache_add(struct db_context *anc_cache,
+/**
+ * Adds an object's GUID to the cache of objects already sent.
+ * This avoids us sending the same object multiple times when
+ * the GetNCChanges request uses a flag like GET_ANC.
+ */
+static WERROR dcesrv_drsuapi_obj_cache_add(struct db_context *obj_cache,
 					   const struct GUID *guid)
 {
 	enum ndr_err_code ndr_err;
@@ -1960,7 +1966,7 @@ static WERROR dcesrv_drsuapi_anc_cache_add(struct db_context *anc_cache,
 		return WERR_DS_DRA_INTERNAL_ERROR;
 	}
 
-	status = dbwrap_store(anc_cache, key, val, TDB_REPLACE);
+	status = dbwrap_store(obj_cache, key, val, TDB_REPLACE);
 	if (!NT_STATUS_IS_OK(status)) {
 		return WERR_DS_DRA_INTERNAL_ERROR;
 	}
@@ -1968,7 +1974,11 @@ static WERROR dcesrv_drsuapi_anc_cache_add(struct db_context *anc_cache,
 	return WERR_OK;
 }
 
-static WERROR dcesrv_drsuapi_anc_cache_exists(struct db_context *anc_cache,
+/**
+ * Checks if the object with the GUID specified already exists in the
+ * object cache, i.e. it's already been sent in a GetNCChanges response.
+ */
+static WERROR dcesrv_drsuapi_obj_cache_exists(struct db_context *obj_cache,
 					      const struct GUID *guid)
 {
 	enum ndr_err_code ndr_err;
@@ -1989,7 +1999,7 @@ static WERROR dcesrv_drsuapi_anc_cache_exists(struct db_context *anc_cache,
 		return WERR_DS_DRA_INTERNAL_ERROR;
 	}
 
-	exists = dbwrap_exists(anc_cache, key);
+	exists = dbwrap_exists(obj_cache, key);
 	if (!exists) {
 		return WERR_OBJECT_NOT_FOUND;
 	}
@@ -2497,10 +2507,11 @@ allowed:
 		if (req10->extended_op != DRSUAPI_EXOP_NONE) {
 			/* Do nothing */
 		} else if (req10->replica_flags & DRSUAPI_DRS_GET_ANC) {
-			getnc_state->anc_cache = db_open_rbt(getnc_state);
-			if (getnc_state->anc_cache == NULL) {
+			getnc_state->obj_cache = db_open_rbt(getnc_state);
+			if (getnc_state->obj_cache == NULL) {
 				return WERR_NOT_ENOUGH_MEMORY;
 			}
+			getnc_state->is_get_anc = true;
 		}
 	}
 
@@ -2650,8 +2661,8 @@ allowed:
 		 * an object, we don't need to do anything more,
 		 * as we've already added the links.
 		 */
-		if (getnc_state->anc_cache != NULL) {
-			werr = dcesrv_drsuapi_anc_cache_exists(getnc_state->anc_cache,
+		if (getnc_state->obj_cache != NULL) {
+			werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
 							       &getnc_state->guids[i]);
 			if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
 				dcesrv_drsuapi_update_highwatermark(msg,
@@ -2707,14 +2718,16 @@ allowed:
 
 		new_objs = obj;
 
-		if (getnc_state->anc_cache != NULL) {
-			werr = dcesrv_drsuapi_anc_cache_add(getnc_state->anc_cache,
+		if (getnc_state->obj_cache != NULL) {
+			werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
 							    &getnc_state->guids[i]);
 			if (!W_ERROR_IS_OK(werr)) {
 				return werr;
 			}
 
-			next_anc_guid = obj->parent_object_guid;
+			if (getnc_state->is_get_anc) {
+				next_anc_guid = obj->parent_object_guid;
+			}
 		}
 
 		while (next_anc_guid != NULL) {
@@ -2723,7 +2736,7 @@ allowed:
 			struct ldb_result *anc_res = NULL;
 			struct ldb_dn *anc_dn = NULL;
 
-			werr = dcesrv_drsuapi_anc_cache_exists(getnc_state->anc_cache,
+			werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
 							       next_anc_guid);
 			if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
 				/*
@@ -2810,7 +2823,7 @@ allowed:
 			 * Regardless of if we actually use it or not,
 			 * we add it to the cache so we don't look at it again
 			 */
-			werr = dcesrv_drsuapi_anc_cache_add(getnc_state->anc_cache,
+			werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
 							    next_anc_guid);
 			if (!W_ERROR_IS_OK(werr)) {
 				return werr;
-- 
2.7.4


From ab92c9808eb85229c037a3d1909214a271ff6119 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 6 Jun 2017 15:03:33 +1200
Subject: [PATCH 02/13] getncchanges.c: Split sorting linked attributes into
 separate function

Longer-term we want to split up the links so that they're sent over
multiple GetNCChanges response messages. So it makes sense to split this
code out into its own function. In the short-term, this removes some of
the complexity from dcesrv_drsuapi_DsGetNCChanges() so that the function
is not quite so big.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 137 ++++++++++++++++++------------
 1 file changed, 84 insertions(+), 53 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 7acab66..9905ad9 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -69,7 +69,7 @@ struct drsuapi_getncchanges_state {
 
 /* We must keep the GUIDs in NDR form for sorting */
 struct la_for_sorting {
-	struct drsuapi_DsReplicaLinkedAttribute *link;
+	const struct drsuapi_DsReplicaLinkedAttribute *link;
 	uint8_t target_guid[16];
         uint8_t source_guid[16];
 };
@@ -2007,6 +2007,82 @@ static WERROR dcesrv_drsuapi_obj_cache_exists(struct db_context *obj_cache,
 	return WERR_OBJECT_NAME_EXISTS;
 }
 
+/**
+ * Copies the la_list specified into a sorted array, ready to be sent in a
+ * GetNCChanges response.
+ */
+static WERROR getncchanges_get_sorted_array(const struct drsuapi_DsReplicaLinkedAttribute *la_list,
+					    const uint32_t link_count,
+					    struct ldb_context *sam_ctx,
+					    TALLOC_CTX *mem_ctx,
+					    const struct dsdb_schema *schema,
+					    struct la_for_sorting **ret_array)
+{
+	int j;
+	struct la_for_sorting *guid_array;
+	WERROR werr = WERR_OK;
+
+	*ret_array = NULL;
+	guid_array = talloc_array(mem_ctx, struct la_for_sorting, link_count);
+	if (guid_array == NULL) {
+		DEBUG(0, ("Out of memory allocating %u linked attributes for sorting", link_count));
+		return WERR_NOT_ENOUGH_MEMORY;
+	}
+
+	for (j = 0; j < link_count; j++) {
+
+		/* we need to get the target GUIDs to compare */
+		struct dsdb_dn *dn;
+		const struct drsuapi_DsReplicaLinkedAttribute *la = &la_list[j];
+		const struct dsdb_attribute *schema_attrib;
+		const struct ldb_val *target_guid;
+		DATA_BLOB source_guid;
+		TALLOC_CTX *frame = talloc_stackframe();
+		NTSTATUS status;
+
+		schema_attrib = dsdb_attribute_by_attributeID_id(schema, la->attid);
+
+		werr = dsdb_dn_la_from_blob(sam_ctx, schema_attrib, schema, frame, la->value.blob, &dn);
+		if (!W_ERROR_IS_OK(werr)) {
+			DEBUG(0,(__location__ ": Bad la blob in sort\n"));
+			TALLOC_FREE(frame);
+			return werr;
+		}
+
+		/* Extract the target GUID in NDR form */
+		target_guid = ldb_dn_get_extended_component(dn->dn, "GUID");
+		if (target_guid == NULL
+				|| target_guid->length != sizeof(guid_array[0].target_guid)) {
+			status = NT_STATUS_OBJECT_NAME_NOT_FOUND;
+		} else {
+			/* Repack the source GUID as NDR for sorting */
+			status = GUID_to_ndr_blob(&la->identifier->guid,
+						  frame,
+						  &source_guid);
+		}
+
+		if (!NT_STATUS_IS_OK(status)
+				|| source_guid.length != sizeof(guid_array[0].source_guid)) {
+			DEBUG(0,(__location__ ": Bad la guid in sort\n"));
+			TALLOC_FREE(frame);
+			return ntstatus_to_werror(status);
+		}
+
+		guid_array[j].link = &la_list[j];
+		memcpy(guid_array[j].target_guid, target_guid->data,
+		       sizeof(guid_array[j].target_guid));
+		memcpy(guid_array[j].source_guid, source_guid.data,
+		       sizeof(guid_array[j].source_guid));
+		TALLOC_FREE(frame);
+	}
+
+	LDB_TYPESAFE_QSORT(guid_array, link_count, NULL, linked_attribute_compare);
+
+	*ret_array = guid_array;
+
+	return werr;
+}
+
 /* 
   drsuapi_DsGetNCChanges
 
@@ -2925,59 +3001,14 @@ allowed:
 	} else {
 		/* sort the whole array the first time */
 		if (getnc_state->la_sorted == NULL) {
-			int j;
-			struct la_for_sorting *guid_array = talloc_array(getnc_state, struct la_for_sorting, getnc_state->la_count);
-			if (guid_array == NULL) {
-				DEBUG(0, ("Out of memory allocating %u linked attributes for sorting", getnc_state->la_count));
-				return WERR_NOT_ENOUGH_MEMORY;
-			}
-			for (j = 0; j < getnc_state->la_count; j++) {
-				/* we need to get the target GUIDs to compare */
-				struct dsdb_dn *dn;
-				const struct drsuapi_DsReplicaLinkedAttribute *la = &getnc_state->la_list[j];
-				const struct dsdb_attribute *schema_attrib;
-				const struct ldb_val *target_guid;
-				DATA_BLOB source_guid;
-				TALLOC_CTX *frame = talloc_stackframe();
-
-				schema_attrib = dsdb_attribute_by_attributeID_id(schema, la->attid);
-
-				werr = dsdb_dn_la_from_blob(sam_ctx, schema_attrib, schema, frame, la->value.blob, &dn);
-				if (!W_ERROR_IS_OK(werr)) {
-					DEBUG(0,(__location__ ": Bad la blob in sort\n"));
-					TALLOC_FREE(frame);
-					return werr;
-				}
-
-				/* Extract the target GUID in NDR form */
-				target_guid = ldb_dn_get_extended_component(dn->dn, "GUID");
-				if (target_guid == NULL
-				    || target_guid->length != sizeof(guid_array[0].target_guid)) {
-					status = NT_STATUS_OBJECT_NAME_NOT_FOUND;
-				} else {
-					/* Repack the source GUID as NDR for sorting */
-					status = GUID_to_ndr_blob(&la->identifier->guid,
-								  frame,
-								  &source_guid);
-				}
-
-				if (!NT_STATUS_IS_OK(status)
-				    || source_guid.length != sizeof(guid_array[0].source_guid)) {
-					DEBUG(0,(__location__ ": Bad la guid in sort\n"));
-					TALLOC_FREE(frame);
-					return ntstatus_to_werror(status);
-				}
-
-				guid_array[j].link = &getnc_state->la_list[j];
-				memcpy(guid_array[j].target_guid, target_guid->data,
-				       sizeof(guid_array[j].target_guid));
-				memcpy(guid_array[j].source_guid, source_guid.data,
-				       sizeof(guid_array[j].source_guid));
-				TALLOC_FREE(frame);
+			werr = getncchanges_get_sorted_array(getnc_state->la_list,
+							     getnc_state->la_count,
+							     sam_ctx, getnc_state,
+							     schema,
+							     &getnc_state->la_sorted);
+			if (!W_ERROR_IS_OK(werr)) {
+				return werr;
 			}
-
-			LDB_TYPESAFE_QSORT(guid_array, getnc_state->la_count, NULL, linked_attribute_compare);
-			getnc_state->la_sorted = guid_array;
 		}
 
 		link_count = getnc_state->la_count - getnc_state->la_idx;
-- 
2.7.4


From cb07e6a9b6b7a7a7c512383642c7b82654ac1976 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 15:34:04 +1200
Subject: [PATCH 03/13] getncchanges.c: Split GET_ANC block out into its own
 function

When we add GET_TGT support, it's going to need to reuse all this code
(i.e. to add any ancestors of the link target). This also trims down
the rather large dcesrv_drsuapi_DsGetNCChanges() function a bit.

Note also fixed a compiler warning in the WERR_DS_DRA_INCONSISTENT_DIT
error block which may have caused issues previously (statement was
terminated by a ',' rather than a ';').

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 294 +++++++++++++++++-------------
 1 file changed, 170 insertions(+), 124 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 9905ad9..7646a0d 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2083,7 +2083,165 @@ static WERROR getncchanges_get_sorted_array(const struct drsuapi_DsReplicaLinked
 	return werr;
 }
 
-/* 
+
+/**
+ * Adds any ancestor/parent objects of the child_obj specified.
+ * This is needed when the GET_ANC flag is specified in the request.
+ * @param new_objs if parents are added, this gets updated to point to a chain
+ * of parent objects (with the parents first and the child last)
+ */
+static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemEx *child_obj,
+					 struct ldb_dn *child_dn,
+					 TALLOC_CTX *mem_ctx,
+					 struct ldb_context *sam_ctx,
+					 struct drsuapi_getncchanges_state *getnc_state,
+					 struct dsdb_schema *schema,
+					 DATA_BLOB *session_key,
+					 struct drsuapi_DsGetNCChangesRequest10 *req10,
+					 uint32_t *local_pas,
+					 struct ldb_dn *machine_dn,
+					 struct drsuapi_DsReplicaObjectListItemEx **new_objs)
+{
+	int ret;
+	const struct GUID *next_anc_guid = NULL;
+	WERROR werr = WERR_OK;
+	static const char * const msg_attrs[] = {
+					    "*",
+					    "nTSecurityDescriptor",
+					    "parentGUID",
+					    "replPropertyMetaData",
+					    DSDB_SECRET_ATTRIBUTES,
+					    NULL };
+
+	next_anc_guid = child_obj->parent_object_guid;
+
+	while (next_anc_guid != NULL) {
+		struct drsuapi_DsReplicaObjectListItemEx *anc_obj = NULL;
+		struct ldb_message *anc_msg = NULL;
+		struct ldb_result *anc_res = NULL;
+		struct ldb_dn *anc_dn = NULL;
+
+		/*
+		 * Don't send an object twice. (If we've sent the object, then
+		 * we've also sent all its parents as well)
+		 */
+		werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
+						       next_anc_guid);
+		if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
+			return WERR_OK;
+		}
+		if (W_ERROR_IS_OK(werr)) {
+			return WERR_INTERNAL_ERROR;
+		}
+		if (!W_ERROR_EQUAL(werr, WERR_OBJECT_NOT_FOUND)) {
+			return werr;
+		}
+
+		anc_obj = talloc_zero(mem_ctx,
+				      struct drsuapi_DsReplicaObjectListItemEx);
+		if (anc_obj == NULL) {
+			return WERR_NOT_ENOUGH_MEMORY;
+		}
+
+		anc_dn = ldb_dn_new_fmt(anc_obj, sam_ctx, "<GUID=%s>",
+					GUID_string(anc_obj, next_anc_guid));
+		if (anc_dn == NULL) {
+			return WERR_NOT_ENOUGH_MEMORY;
+		}
+
+		ret = drsuapi_search_with_extended_dn(sam_ctx, anc_obj,
+						      &anc_res, anc_dn,
+						      LDB_SCOPE_BASE,
+						      msg_attrs, NULL);
+		if (ret != LDB_SUCCESS) {
+			const char *anc_str = NULL;
+			const char *obj_str = NULL;
+
+			anc_str = ldb_dn_get_extended_linearized(anc_obj,
+								 anc_dn,
+								 1);
+			obj_str = ldb_dn_get_extended_linearized(anc_obj,
+								 child_dn,
+								 1);
+
+			DBG_ERR("getncchanges: failed to fetch ANC "
+				"DN %s for DN %s - %s\n",
+				anc_str, obj_str, ldb_errstring(sam_ctx));
+			return WERR_DS_DRA_INCONSISTENT_DIT;
+		}
+
+		anc_msg = anc_res->msgs[0];
+
+		werr = get_nc_changes_build_object(anc_obj, anc_msg,
+						   sam_ctx,
+						   getnc_state->ncRoot_dn,
+						   getnc_state->is_schema_nc,
+						   schema, session_key,
+						   getnc_state->min_usn,
+						   req10->replica_flags,
+						   req10->partial_attribute_set,
+						   req10->uptodateness_vector,
+						   req10->extended_op,
+						   false, /* force_object_return */
+						   local_pas,
+						   machine_dn,
+						   next_anc_guid);
+		if (!W_ERROR_IS_OK(werr)) {
+			return werr;
+		}
+
+		werr = get_nc_changes_add_links(sam_ctx, getnc_state,
+						getnc_state->ncRoot_dn,
+						getnc_state->is_schema_nc,
+						schema, getnc_state->min_usn,
+						req10->replica_flags,
+						anc_msg,
+						&getnc_state->la_list,
+						&getnc_state->la_count,
+						req10->uptodateness_vector);
+		if (!W_ERROR_IS_OK(werr)) {
+			return werr;
+		}
+
+		/*
+		 * Regardless of whether we actually use it or not,
+		 * we add it to the cache so we don't look at it again
+		 */
+		werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
+						    next_anc_guid);
+		if (!W_ERROR_IS_OK(werr)) {
+			return werr;
+		}
+
+		/*
+		 * Any ancestors which are below the highwatermark
+		 * or uptodateness_vector shouldn't be added,
+		 * but we still look further up the
+		 * tree for ones which have been changed recently.
+		 */
+		if (anc_obj->meta_data_ctr != NULL) {
+
+			/*
+			 * prepend the parent to the list so that the client-side
+			 * adds the parent object before it adds the children
+			 */
+			anc_obj->next_object = *new_objs;
+			*new_objs = anc_obj;
+		}
+
+		anc_msg = NULL;
+		TALLOC_FREE(anc_res);
+		TALLOC_FREE(anc_dn);
+
+		/*
+		 * We may need to resolve more parents...
+		 */
+		next_anc_guid = anc_obj->parent_object_guid;
+	}
+	return werr;
+}
+
+/*
   drsuapi_DsGetNCChanges
 
   see MS-DRSR 4.1.10.5.2 for basic logic of this function
@@ -2691,7 +2849,6 @@ allowed:
 					    NULL };
 		struct ldb_result *msg_res;
 		struct ldb_dn *msg_dn;
-		const struct GUID *next_anc_guid = NULL;
 
 		obj = talloc_zero(mem_ctx, struct drsuapi_DsReplicaObjectListItemEx);
 		W_ERROR_HAVE_NO_MEMORY(obj);
@@ -2800,133 +2957,22 @@ allowed:
 			if (!W_ERROR_IS_OK(werr)) {
 				return werr;
 			}
-
-			if (getnc_state->is_get_anc) {
-				next_anc_guid = obj->parent_object_guid;
-			}
 		}
 
-		while (next_anc_guid != NULL) {
-			struct drsuapi_DsReplicaObjectListItemEx *anc_obj = NULL;
-			struct ldb_message *anc_msg = NULL;
-			struct ldb_result *anc_res = NULL;
-			struct ldb_dn *anc_dn = NULL;
-
-			werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
-							       next_anc_guid);
-			if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
-				/*
-				 * We don't need to send it twice.
-				 */
-				break;
-			}
-			if (W_ERROR_IS_OK(werr)) {
-				return WERR_INTERNAL_ERROR;
-			}
-			if (!W_ERROR_EQUAL(werr, WERR_OBJECT_NOT_FOUND)) {
-				return werr;
-			}
-			werr = WERR_OK;
-
-			anc_obj = talloc_zero(mem_ctx,
-					struct drsuapi_DsReplicaObjectListItemEx);
-			if (anc_obj == NULL) {
-				return WERR_NOT_ENOUGH_MEMORY;
-			}
-
-			anc_dn = ldb_dn_new_fmt(anc_obj, sam_ctx, "<GUID=%s>",
-					GUID_string(anc_obj, next_anc_guid));
-			if (anc_dn == NULL) {
-				return WERR_NOT_ENOUGH_MEMORY;
-			}
-
-			ret = drsuapi_search_with_extended_dn(sam_ctx, anc_obj,
-							      &anc_res, anc_dn,
-							      LDB_SCOPE_BASE,
-							      msg_attrs, NULL);
-			if (ret != LDB_SUCCESS) {
-				const char *anc_str = NULL;
-				const char *obj_str = NULL;
-
-				anc_str = ldb_dn_get_extended_linearized(anc_obj,
-									 anc_dn,
-									 1);
-				obj_str = ldb_dn_get_extended_linearized(anc_obj,
-									 msg->dn,
-									 1),
-
-				DBG_ERR("getncchanges: failed to fetch ANC "
-					"DN %s for DN %s - %s\n",
-					anc_str, obj_str,
-					ldb_errstring(sam_ctx));
-				return WERR_DS_DRA_INCONSISTENT_DIT;
-			}
-
-			anc_msg = anc_res->msgs[0];
-
-			werr = get_nc_changes_build_object(anc_obj, anc_msg,
-							   sam_ctx,
-							   getnc_state->ncRoot_dn,
-							   getnc_state->is_schema_nc,
-							   schema, &session_key,
-							   getnc_state->min_usn,
-							   req10->replica_flags,
-							   req10->partial_attribute_set,
-							   req10->uptodateness_vector,
-							   req10->extended_op,
-							   false, /* force_object_return */
-							   local_pas,
-							   machine_dn,
-							   next_anc_guid);
-			if (!W_ERROR_IS_OK(werr)) {
-				return werr;
-			}
-
-			werr = get_nc_changes_add_links(sam_ctx, getnc_state,
-							getnc_state->ncRoot_dn,
-							getnc_state->is_schema_nc,
-							schema, getnc_state->min_usn,
-							req10->replica_flags,
-							anc_msg,
-							&getnc_state->la_list,
-							&getnc_state->la_count,
-							req10->uptodateness_vector);
-			if (!W_ERROR_IS_OK(werr)) {
-				return werr;
-			}
-
-			/*
-			 * Regardless of if we actually use it or not,
-			 * we add it to the cache so we don't look at it again
-			 */
-			werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
-							    next_anc_guid);
+		/*
+		 * For GET_ANC, prepend any parents that the client needs
+		 * to know about before it can add this object
+		 */
+		if (getnc_state->is_get_anc) {
+			werr = getncchanges_add_ancestors(obj, msg->dn, mem_ctx,
+							  sam_ctx, getnc_state,
+							  schema, &session_key,
+							  req10, local_pas,
+							  machine_dn,
+							  &new_objs);
 			if (!W_ERROR_IS_OK(werr)) {
 				return werr;
 			}
-
-			/*
-			 * Any ancestors which are below the highwatermark
-			 * or uptodateness_vector shouldn't be added,
-			 * but we still look further up the
-			 * tree for ones which have been changed recently.
-			 */
-			if (anc_obj->meta_data_ctr != NULL) {
-				/*
-				 * prepend it to the list
-				 */
-				anc_obj->next_object = new_objs;
-				new_objs = anc_obj;
-			}
-
-			anc_msg = NULL;
-			TALLOC_FREE(anc_res);
-			TALLOC_FREE(anc_dn);
-
-			/*
-			 * We may need to resolve more...
-			 */
-			next_anc_guid = anc_obj->parent_object_guid;
 		}
 
 		*currentObject = new_objs;
-- 
2.7.4


From 945af76c230688df96f3685f3cc99d87da474ef6 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 15:45:39 +1200
Subject: [PATCH 04/13] getncchanges.c: Add ancestor links when the object
 normally gets sent

Currently we add links each time we send an object, but we don't
actually send these links until the end of the replication cycle.

In subsequent patches we want the links to be sent in the same chunk as
their source object, ideally in as close to USN order as possible.
Processing ancestors complicates this a bit, as the ancestor will have a
higher USN than what we're currently up to, and so potentially will the
ancestor's links.

This patch moves where the ancestor's links get added to the
getnc_state->la_list. The ancestor's links now get added when the object
would normally get sent based purely on its USN (we update the highwater
mark at this point too).

This should not affect functionality, i.e. because we send all the links
at the end, it should make no difference at what point they get added to
the list.

This duplicates a tiny bit of code, but this will be cleaned up in the
next patch.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 7646a0d..61a98ee 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2190,19 +2190,6 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
 			return werr;
 		}
 
-		werr = get_nc_changes_add_links(sam_ctx, getnc_state,
-						getnc_state->ncRoot_dn,
-						getnc_state->is_schema_nc,
-						schema, getnc_state->min_usn,
-						req10->replica_flags,
-						anc_msg,
-						&getnc_state->la_list,
-						&getnc_state->la_count,
-						req10->uptodateness_vector);
-		if (!W_ERROR_IS_OK(werr)) {
-			return werr;
-		}
-
 		/*
 		 * Regardless of whether we actually use it or not,
 		 * we add it to the cache so we don't look at it again
@@ -2891,8 +2878,7 @@ allowed:
 
 		/*
 		 * If it has already been added as an ancestor of
-		 * an object, we don't need to do anything more,
-		 * as we've already added the links.
+		 * an object, we don't need to do anything more
 		 */
 		if (getnc_state->obj_cache != NULL) {
 			werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
@@ -2901,6 +2887,21 @@ allowed:
 				dcesrv_drsuapi_update_highwatermark(msg,
 						getnc_state->max_usn,
 						&r->out.ctr->ctr6.new_highwatermark);
+
+				werr = get_nc_changes_add_links(sam_ctx, getnc_state,
+								getnc_state->ncRoot_dn,
+								getnc_state->is_schema_nc,
+								schema, getnc_state->min_usn,
+								req10->replica_flags,
+								msg,
+								&getnc_state->la_list,
+								&getnc_state->la_count,
+								req10->uptodateness_vector);
+
+				if (!W_ERROR_IS_OK(werr)) {
+					return werr;
+				}
+
 				/* no attributes to send */
 				talloc_free(obj);
 				continue;
-- 
2.7.4


From 187fa6895487d9127c3b02bc867e6c8b9ab18531 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 16:00:57 +1200
Subject: [PATCH 05/13] getncchanges.c: Refactor how we add ancestor links

If the current object had already been sent as an ancestor, we were
duplicating the code that added its links and updated the HWM mark.
We want these to occur when we reach the place where the object's USN
naturally occurs.

Instead of duplicating this code, we can just skip the call to
get_nc_changes_build_object() if the object has already been sent.
There is already an existing 'nothing to send'/continue case after we've
updated the highwater mark.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 70 +++++++++++++------------------
 1 file changed, 29 insertions(+), 41 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 61a98ee..aa60926 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2836,6 +2836,7 @@ allowed:
 					    NULL };
 		struct ldb_result *msg_res;
 		struct ldb_dn *msg_dn;
+		bool obj_already_sent = false;
 
 		obj = talloc_zero(mem_ctx, struct drsuapi_DsReplicaObjectListItemEx);
 		W_ERROR_HAVE_NO_MEMORY(obj);
@@ -2877,54 +2878,41 @@ allowed:
 		msg = msg_res->msgs[0];
 
 		/*
-		 * If it has already been added as an ancestor of
-		 * an object, we don't need to do anything more
+		 * Check if we've already sent the object as an ancestor of
+		 * another object. If so, we don't need to send it again
 		 */
 		if (getnc_state->obj_cache != NULL) {
 			werr = dcesrv_drsuapi_obj_cache_exists(getnc_state->obj_cache,
 							       &getnc_state->guids[i]);
 			if (W_ERROR_EQUAL(werr, WERR_OBJECT_NAME_EXISTS)) {
-				dcesrv_drsuapi_update_highwatermark(msg,
-						getnc_state->max_usn,
-						&r->out.ctr->ctr6.new_highwatermark);
-
-				werr = get_nc_changes_add_links(sam_ctx, getnc_state,
-								getnc_state->ncRoot_dn,
-								getnc_state->is_schema_nc,
-								schema, getnc_state->min_usn,
-								req10->replica_flags,
-								msg,
-								&getnc_state->la_list,
-								&getnc_state->la_count,
-								req10->uptodateness_vector);
-
-				if (!W_ERROR_IS_OK(werr)) {
-					return werr;
-				}
-
-				/* no attributes to send */
-				talloc_free(obj);
-				continue;
+				obj_already_sent = true;
 			}
 		}
 
-		max_wait_reached = (time(NULL) - start > max_wait);
-
-		werr = get_nc_changes_build_object(obj, msg,
-						   sam_ctx, getnc_state->ncRoot_dn,
-						   getnc_state->is_schema_nc,
-						   schema, &session_key, getnc_state->min_usn,
-						   req10->replica_flags,
-						   req10->partial_attribute_set,
-						   req10->uptodateness_vector,
-						   req10->extended_op,
-						   max_wait_reached,
-						   local_pas, machine_dn,
-						   &getnc_state->guids[i]);
-		if (!W_ERROR_IS_OK(werr)) {
-			return werr;
+		if (!obj_already_sent) {
+			max_wait_reached = (time(NULL) - start > max_wait);
+
+			werr = get_nc_changes_build_object(obj, msg,
+							   sam_ctx, getnc_state->ncRoot_dn,
+							   getnc_state->is_schema_nc,
+							   schema, &session_key, getnc_state->min_usn,
+							   req10->replica_flags,
+							   req10->partial_attribute_set,
+							   req10->uptodateness_vector,
+							   req10->extended_op,
+							   max_wait_reached,
+							   local_pas, machine_dn,
+							   &getnc_state->guids[i]);
+			if (!W_ERROR_IS_OK(werr)) {
+				return werr;
+			}
 		}
 
+		/*
+		 * We've reached the USN where this object naturally occurs.
+		 * Regardless of whether we've already sent the object (as an
+		 * ancestor), we add its links and update the HWM at this point
+		 */
 		werr = get_nc_changes_add_links(sam_ctx, getnc_state,
 						getnc_state->ncRoot_dn,
 						getnc_state->is_schema_nc,
@@ -2942,11 +2930,11 @@ allowed:
 					getnc_state->max_usn,
 					&r->out.ctr->ctr6.new_highwatermark);
 
-		if (obj->meta_data_ctr == NULL) {
+		if (obj_already_sent || obj->meta_data_ctr == NULL) {
 			DEBUG(8,(__location__ ": getncchanges skipping send of object %s\n",
 				 ldb_dn_get_linearized(msg->dn)));
-			/* no attributes to send */
-			talloc_free(obj);
+			/* nothing to send */
+			TALLOC_FREE(obj);
 			continue;
 		}
 
-- 
2.7.4


From 471e21ec55d3899b728907eb53e1a6e4893de2c1 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 16:17:10 +1200
Subject: [PATCH 06/13] getncchanges.c: Refactor how objects get added to the
 response

Adding GET_TGT support is going to make things more complicated, and I
think we are going to struggle to do this without refactoring things a
bit.

This patch adds a helper struct to store state related to a single
GetNCChanges chunk. I plan to add to this with things like max_links,
max_objects, etc, which will cutdown on the number of variables/
parameters we pass around.

I found the double-pointer logic where we add objects to the response
confusing - hopefully this refactor simplifies things slightly, and it
allows us to reuse the code for the GET_TGT case.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 67 +++++++++++++++++++++++++------
 1 file changed, 55 insertions(+), 12 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index aa60926..922dfa8 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -45,7 +45,10 @@
 #undef DBGC_CLASS
 #define DBGC_CLASS            DBGC_DRS_REPL
 
-/* state of a partially completed getncchanges call */
+/*
+ * state of a partially-completed replication cycle. This state persists
+ * over multiple calls to dcesrv_drsuapi_DsGetNCChanges()
+ */
 struct drsuapi_getncchanges_state {
 	struct db_context *obj_cache;
 	struct GUID *guids;
@@ -71,7 +74,18 @@ struct drsuapi_getncchanges_state {
 struct la_for_sorting {
 	const struct drsuapi_DsReplicaLinkedAttribute *link;
 	uint8_t target_guid[16];
-        uint8_t source_guid[16];
+	uint8_t source_guid[16];
+};
+
+/*
+ * stores the state for a chunk of replication data. This state information
+ * only exists for a single call to dcesrv_drsuapi_DsGetNCChanges()
+ */
+struct getncchanges_repl_chunk {
+	struct drsuapi_DsGetNCChangesCtr6 *ctr6;
+
+	/* the last object written to the response */
+	struct drsuapi_DsReplicaObjectListItemEx *last_object;
 };
 
 static int drsuapi_DsReplicaHighWaterMark_cmp(const struct drsuapi_DsReplicaHighWaterMark *h1,
@@ -2228,6 +2242,37 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
 	return werr;
 }
 
+/**
+ * Adds a list of new objects into the getNCChanges response message
+ */
+static void getncchanges_add_objs_to_resp(struct getncchanges_repl_chunk *repl_chunk,
+					  struct drsuapi_DsReplicaObjectListItemEx *obj_list)
+{
+	struct drsuapi_DsReplicaObjectListItemEx *obj;
+
+	/*
+	 * We track the last object added to the response message, so just add
+	 * the new object-list onto the end
+	 */
+	if (repl_chunk->last_object == NULL) {
+		repl_chunk->ctr6->first_object = obj_list;
+	} else {
+		repl_chunk->last_object->next_object = obj_list;
+	}
+
+	for (obj = obj_list; obj != NULL; obj = obj->next_object) {
+		repl_chunk->ctr6->object_count += 1;
+
+		/*
+		 * Remember the last object in the response - we'll use this to
+		 * link the next object(s) processed onto the existing list
+		 */
+		if (obj->next_object == NULL) {
+			repl_chunk->last_object = obj;
+		}
+	}
+}
+
 /*
   drsuapi_DsGetNCChanges
 
@@ -2241,7 +2286,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 	uint32_t i, k;
 	struct dsdb_schema *schema;
 	struct drsuapi_DsReplicaOIDMapping_Ctr *ctr;
-	struct drsuapi_DsReplicaObjectListItemEx **currentObject;
+	struct getncchanges_repl_chunk repl_chunk = { 0 };
 	NTSTATUS status;
 	DATA_BLOB session_key;
 	WERROR werr;
@@ -2768,7 +2813,8 @@ allowed:
 	r->out.ctr->ctr6.old_highwatermark = req10->highwatermark;
 	r->out.ctr->ctr6.new_highwatermark = req10->highwatermark;
 
-	currentObject = &r->out.ctr->ctr6.first_object;
+	repl_chunk.ctr6 = &r->out.ctr->ctr6;
+	repl_chunk.last_object = NULL;
 
 	max_objects = lpcfg_parm_int(dce_call->conn->dce_ctx->lp_ctx, NULL, "drs", "max object sync", 1000);
 	/*
@@ -2964,14 +3010,11 @@ allowed:
 			}
 		}
 
-		*currentObject = new_objs;
-		while (new_objs != NULL) {
-			r->out.ctr->ctr6.object_count += 1;
-			if (new_objs->next_object == NULL) {
-				currentObject = &new_objs->next_object;
-			}
-			new_objs = new_objs->next_object;
-		}
+		/*
+		 * Add the object (and any parents it might have) into the
+		 * response message
+		 */
+		getncchanges_add_objs_to_resp(&repl_chunk, new_objs);
 
 		DEBUG(8,(__location__ ": replicating object %s\n", ldb_dn_get_linearized(msg->dn)));
 
-- 
2.7.4


From 1874f26bcd4886d6259948a81609106890f6197d Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 16:19:54 +1200
Subject: [PATCH 07/13] getncchanges.c: Replace hard-coded numbers with a
 define

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 922dfa8..c8f38ba 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -45,6 +45,8 @@
 #undef DBGC_CLASS
 #define DBGC_CLASS            DBGC_DRS_REPL
 
+#define DRS_GUID_SIZE       16
+
 /*
  * state of a partially-completed replication cycle. This state persists
  * over multiple calls to dcesrv_drsuapi_DsGetNCChanges()
@@ -73,8 +75,8 @@ struct drsuapi_getncchanges_state {
 /* We must keep the GUIDs in NDR form for sorting */
 struct la_for_sorting {
 	const struct drsuapi_DsReplicaLinkedAttribute *link;
-	uint8_t target_guid[16];
-	uint8_t source_guid[16];
+	uint8_t target_guid[DRS_GUID_SIZE];
+	uint8_t source_guid[DRS_GUID_SIZE];
 };
 
 /*
@@ -1959,7 +1961,7 @@ static WERROR dcesrv_drsuapi_obj_cache_add(struct db_context *obj_cache,
 					   const struct GUID *guid)
 {
 	enum ndr_err_code ndr_err;
-	uint8_t guid_buf[16] = { 0, };
+	uint8_t guid_buf[DRS_GUID_SIZE] = { 0, };
 	DATA_BLOB b = {
 		.data = guid_buf,
 		.length = sizeof(guid_buf),
@@ -1996,7 +1998,7 @@ static WERROR dcesrv_drsuapi_obj_cache_exists(struct db_context *obj_cache,
 					      const struct GUID *guid)
 {
 	enum ndr_err_code ndr_err;
-	uint8_t guid_buf[16] = { 0, };
+	uint8_t guid_buf[DRS_GUID_SIZE] = { 0, };
 	DATA_BLOB b = {
 		.data = guid_buf,
 		.length = sizeof(guid_buf),
-- 
2.7.4


From 1a10593427872deb301dd08382fdc3dc15890ff1 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 16:21:47 +1200
Subject: [PATCH 08/13] getncchanges.c: Remove a really old TODO

This TODO was added in 2009 (before Samba supported linked_attributes
in getNCChanges())

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index c8f38ba..ccc766d 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2330,7 +2330,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 	invocation_id = *(samdb_ntds_invocation_id(sam_ctx));
 
 	*r->out.level_out = 6;
-	/* TODO: linked attributes*/
+
 	r->out.ctr->ctr6.linked_attributes_count = 0;
 	r->out.ctr->ctr6.linked_attributes = discard_const_p(struct drsuapi_DsReplicaLinkedAttribute, &no_linked_attr);
 
-- 
2.7.4


From f4d733a71ca38e81c3576afb817030dd7ffdf6a7 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 16:29:17 +1200
Subject: [PATCH 09/13] getncchanges.c: Remove unused ncRoot_dn parameter

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index ccc766d..2ded753 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -457,7 +457,6 @@ static WERROR get_nc_changes_filter_attrs(struct drsuapi_DsReplicaObjectListItem
 static WERROR get_nc_changes_build_object(struct drsuapi_DsReplicaObjectListItemEx *obj,
 					  const struct ldb_message *msg,
 					  struct ldb_context *sam_ctx,
-					  struct ldb_dn *ncRoot_dn,
 					  bool   is_schema_nc,
 					  struct dsdb_schema *schema,
 					  DATA_BLOB *session_key,
@@ -859,7 +858,6 @@ static WERROR get_nc_changes_add_la(TALLOC_CTX *mem_ctx,
  */
 static WERROR get_nc_changes_add_links(struct ldb_context *sam_ctx,
 				       TALLOC_CTX *mem_ctx,
-				       struct ldb_dn *ncRoot_dn,
 				       bool is_schema_nc,
 				       struct dsdb_schema *schema,
 				       uint64_t highest_usn,
@@ -2190,7 +2188,6 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
 
 		werr = get_nc_changes_build_object(anc_obj, anc_msg,
 						   sam_ctx,
-						   getnc_state->ncRoot_dn,
 						   getnc_state->is_schema_nc,
 						   schema, session_key,
 						   getnc_state->min_usn,
@@ -2941,8 +2938,7 @@ allowed:
 			max_wait_reached = (time(NULL) - start > max_wait);
 
 			werr = get_nc_changes_build_object(obj, msg,
-							   sam_ctx, getnc_state->ncRoot_dn,
-							   getnc_state->is_schema_nc,
+							   sam_ctx, getnc_state->is_schema_nc,
 							   schema, &session_key, getnc_state->min_usn,
 							   req10->replica_flags,
 							   req10->partial_attribute_set,
@@ -2962,7 +2958,6 @@ allowed:
 		 * ancestor), we add its links and update the HWM at this point
 		 */
 		werr = get_nc_changes_add_links(sam_ctx, getnc_state,
-						getnc_state->ncRoot_dn,
 						getnc_state->is_schema_nc,
 						schema, getnc_state->min_usn,
 						req10->replica_flags,
-- 
2.7.4


From 78b69cce27dc663f48b328137ad86b5404dc157b Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 16:50:38 +1200
Subject: [PATCH 10/13] getncchanges.c: Reduce the parameters to
 get_nc_changes_build_object()

Fifteen parameters seems a bit excessive. Instead, pass it the structs
containing the information it cares about.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 33 ++++++++++++++-----------------
 1 file changed, 15 insertions(+), 18 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 2ded753..161453e 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -457,14 +457,10 @@ static WERROR get_nc_changes_filter_attrs(struct drsuapi_DsReplicaObjectListItem
 static WERROR get_nc_changes_build_object(struct drsuapi_DsReplicaObjectListItemEx *obj,
 					  const struct ldb_message *msg,
 					  struct ldb_context *sam_ctx,
-					  bool   is_schema_nc,
+					  struct drsuapi_getncchanges_state *getnc_state,
 					  struct dsdb_schema *schema,
 					  DATA_BLOB *session_key,
-					  uint64_t highest_usn,
-					  uint32_t replica_flags,
-					  struct drsuapi_DsPartialAttributeSet *partial_attribute_set,
-					  struct drsuapi_DsReplicaCursorCtrEx *uptodateness_vector,
-					  enum drsuapi_DsExtendedOperation extended_op,
+					  struct drsuapi_DsGetNCChangesRequest10 *req10,
 					  bool force_object_return,
 					  uint32_t *local_pas,
 					  struct ldb_dn *machine_dn,
@@ -485,6 +481,14 @@ static WERROR get_nc_changes_build_object(struct drsuapi_DsReplicaObjectListItem
 	struct ldb_result *res = NULL;
 	WERROR werr;
 	int ret;
+	uint32_t replica_flags = req10->replica_flags;
+	struct drsuapi_DsPartialAttributeSet *partial_attribute_set =
+			req10->partial_attribute_set;
+	struct drsuapi_DsReplicaCursorCtrEx *uptodateness_vector =
+			req10->uptodateness_vector;
+	enum drsuapi_DsExtendedOperation extended_op = req10->extended_op;
+	bool is_schema_nc = getnc_state->is_schema_nc;
+	uint64_t highest_usn = getnc_state->min_usn;
 
 	/* make dsdb sytanx context for conversions */
 	dsdb_syntax_ctx_init(&syntax_ctx, sam_ctx, schema);
@@ -2188,13 +2192,9 @@ static WERROR getncchanges_add_ancestors(struct drsuapi_DsReplicaObjectListItemE
 
 		werr = get_nc_changes_build_object(anc_obj, anc_msg,
 						   sam_ctx,
-						   getnc_state->is_schema_nc,
+						   getnc_state,
 						   schema, session_key,
-						   getnc_state->min_usn,
-						   req10->replica_flags,
-						   req10->partial_attribute_set,
-						   req10->uptodateness_vector,
-						   req10->extended_op,
+						   req10,
 						   false, /* force_object_return */
 						   local_pas,
 						   machine_dn,
@@ -2938,12 +2938,9 @@ allowed:
 			max_wait_reached = (time(NULL) - start > max_wait);
 
 			werr = get_nc_changes_build_object(obj, msg,
-							   sam_ctx, getnc_state->is_schema_nc,
-							   schema, &session_key, getnc_state->min_usn,
-							   req10->replica_flags,
-							   req10->partial_attribute_set,
-							   req10->uptodateness_vector,
-							   req10->extended_op,
+							   sam_ctx, getnc_state,
+							   schema, &session_key,
+							   req10,
 							   max_wait_reached,
 							   local_pas, machine_dn,
 							   &getnc_state->guids[i]);
-- 
2.7.4


From 61d97500e434642caa7f3540be390537cebd500a Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 17:18:32 +1200
Subject: [PATCH 11/13] getncchanges.c: Split out code to get an object for a
 response

Basically, everytime we try to add an object to the response, we want
to:
- Build it (i.e. pack it into an RPC message format)
- Add it to our object-cache if we're keeping one
- Add any ancestors needed for the client to resolve it (if GET_ANC)

GET_TGT is going to use the exact same code, so split this out into a
separate function, rather than duplicating it.

The GET_ANC case also uses almost identical code, but it differs in a
couple of minor aspects. I've left this as is for now, as I'm not sure
if this is by accident or by design.

Because all the memory was talloc'd off the 'obj' variable, we now need
to replace it with a tmp TALLOC_CTX.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 149 ++++++++++++++++++++----------
 1 file changed, 99 insertions(+), 50 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index 161453e..f018005 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -2272,7 +2272,79 @@ static void getncchanges_add_objs_to_resp(struct getncchanges_repl_chunk *repl_c
 	}
 }
 
-/*
+/**
+ * Gets the object to send, packed into an RPC struct ready to send. This also
+ * adds the object to the object cache, and adds any ancestors (if needed).
+ * @param msg - DB search result for the object to add
+ * @param guid - GUID of the object to add
+ * @param ret_obj_list - returns the object ready to be sent (in a list, along
+ * with any ancestors that might be needed). NULL if nothing to send.
+ */
+static WERROR getncchanges_get_obj_to_send(const struct ldb_message *msg,
+					   TALLOC_CTX *mem_ctx,
+					   struct ldb_context *sam_ctx,
+					   struct drsuapi_getncchanges_state *getnc_state,
+					   struct dsdb_schema *schema,
+					   DATA_BLOB *session_key,
+					   struct drsuapi_DsGetNCChangesRequest10 *req10,
+					   bool force_object_return,
+					   uint32_t *local_pas,
+					   struct ldb_dn *machine_dn,
+					   const struct GUID *guid,
+					   struct drsuapi_DsReplicaObjectListItemEx **ret_obj_list)
+{
+	struct drsuapi_DsReplicaObjectListItemEx *obj;
+	WERROR werr;
+
+	*ret_obj_list = NULL;
+
+	obj = talloc_zero(mem_ctx, struct drsuapi_DsReplicaObjectListItemEx);
+	W_ERROR_HAVE_NO_MEMORY(obj);
+
+	werr = get_nc_changes_build_object(obj, msg, sam_ctx, getnc_state,
+					   schema, session_key, req10,
+					   force_object_return,
+					   local_pas, machine_dn, guid);
+	if (!W_ERROR_IS_OK(werr)) {
+		return werr;
+	}
+
+	/*
+	 * The object may get filtered out by the UTDV's USN and not actually
+	 * sent, in which case there's nothing more to do here
+	 */
+	if (obj->meta_data_ctr == NULL) {
+		TALLOC_FREE(obj);
+		return WERR_OK;
+	}
+
+	if (getnc_state->obj_cache != NULL) {
+		werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
+						    guid);
+		if (!W_ERROR_IS_OK(werr)) {
+			return werr;
+		}
+	}
+
+	*ret_obj_list = obj;
+
+	/*
+	 * If required, also add any ancestors that the client may need to know
+	 * about before it can resolve this object. These get prepended to the
+	 * ret_obj_list so the client adds them first.
+	 */
+	if (getnc_state->is_get_anc) {
+		werr = getncchanges_add_ancestors(obj, msg->dn, mem_ctx,
+						  sam_ctx, getnc_state,
+						  schema, session_key,
+						  req10, local_pas,
+						  machine_dn, ret_obj_list);
+	}
+
+	return werr;
+}
+
+/* 
   drsuapi_DsGetNCChanges
 
   see MS-DRSR 4.1.10.5.2 for basic logic of this function
@@ -2870,7 +2942,6 @@ allowed:
 		     && !max_wait_reached;
 	    i++) {
 		struct drsuapi_DsReplicaObjectListItemEx *new_objs = NULL;
-		struct drsuapi_DsReplicaObjectListItemEx *obj;
 		struct ldb_message *msg;
 		static const char * const msg_attrs[] = {
 					    "*",
@@ -2882,14 +2953,14 @@ allowed:
 		struct ldb_result *msg_res;
 		struct ldb_dn *msg_dn;
 		bool obj_already_sent = false;
+		TALLOC_CTX *tmp_ctx;
 
-		obj = talloc_zero(mem_ctx, struct drsuapi_DsReplicaObjectListItemEx);
-		W_ERROR_HAVE_NO_MEMORY(obj);
+		tmp_ctx = talloc_new(mem_ctx);
 
-		msg_dn = ldb_dn_new_fmt(obj, sam_ctx, "<GUID=%s>", GUID_string(obj, &getnc_state->guids[i]));
+		msg_dn = ldb_dn_new_fmt(tmp_ctx, sam_ctx, "<GUID=%s>",
+					GUID_string(tmp_ctx, &getnc_state->guids[i]));
 		W_ERROR_HAVE_NO_MEMORY(msg_dn);
 
-
 		/*
 		 * by re-searching here we avoid having a lot of full
 		 * records in memory between calls to getncchanges.
@@ -2898,15 +2969,16 @@ allowed:
 		 * (tombstone expunge) between the first and second
 		 * check.
 		 */
-		ret = drsuapi_search_with_extended_dn(sam_ctx, obj, &msg_res,
+		ret = drsuapi_search_with_extended_dn(sam_ctx, tmp_ctx, &msg_res,
 						      msg_dn,
 						      LDB_SCOPE_BASE, msg_attrs, NULL);
 		if (ret != LDB_SUCCESS) {
 			if (ret != LDB_ERR_NO_SUCH_OBJECT) {
 				DEBUG(1,("getncchanges: failed to fetch DN %s - %s\n",
-					 ldb_dn_get_extended_linearized(obj, msg_dn, 1), ldb_errstring(sam_ctx)));
+					 ldb_dn_get_extended_linearized(tmp_ctx, msg_dn, 1),
+					 ldb_errstring(sam_ctx)));
 			}
-			talloc_free(obj);
+			TALLOC_FREE(tmp_ctx);
 			continue;
 		}
 
@@ -2914,9 +2986,9 @@ allowed:
 			DEBUG(1,("getncchanges: got LDB_SUCCESS but failed"
 				 "to get any results in fetch of DN "
 				 "%s (race with tombstone expunge?)\n",
-				 ldb_dn_get_extended_linearized(obj,
+				 ldb_dn_get_extended_linearized(tmp_ctx,
 								msg_dn, 1)));
-			talloc_free(obj);
+			TALLOC_FREE(tmp_ctx);
 			continue;
 		}
 
@@ -2937,13 +3009,17 @@ allowed:
 		if (!obj_already_sent) {
 			max_wait_reached = (time(NULL) - start > max_wait);
 
-			werr = get_nc_changes_build_object(obj, msg,
-							   sam_ctx, getnc_state,
-							   schema, &session_key,
-							   req10,
-							   max_wait_reached,
-							   local_pas, machine_dn,
-							   &getnc_state->guids[i]);
+			/*
+			 * Construct an object, ready to send (this will include
+			 * the object's ancestors as well, if needed)
+			 */
+			werr = getncchanges_get_obj_to_send(msg, mem_ctx, sam_ctx,
+							    getnc_state, schema,
+							    &session_key, req10,
+							    max_wait_reached,
+							    local_pas, machine_dn,
+							    &getnc_state->guids[i],
+							    &new_objs);
 			if (!W_ERROR_IS_OK(werr)) {
 				return werr;
 			}
@@ -2970,43 +3046,17 @@ allowed:
 					getnc_state->max_usn,
 					&r->out.ctr->ctr6.new_highwatermark);
 
-		if (obj_already_sent || obj->meta_data_ctr == NULL) {
+		if (new_objs == NULL) {
 			DEBUG(8,(__location__ ": getncchanges skipping send of object %s\n",
 				 ldb_dn_get_linearized(msg->dn)));
 			/* nothing to send */
-			TALLOC_FREE(obj);
+			TALLOC_FREE(tmp_ctx);
 			continue;
 		}
 
-		new_objs = obj;
-
-		if (getnc_state->obj_cache != NULL) {
-			werr = dcesrv_drsuapi_obj_cache_add(getnc_state->obj_cache,
-							    &getnc_state->guids[i]);
-			if (!W_ERROR_IS_OK(werr)) {
-				return werr;
-			}
-		}
-
-		/*
-		 * For GET_ANC, prepend any parents that the client needs
-		 * to know about before it can add this object
-		 */
-		if (getnc_state->is_get_anc) {
-			werr = getncchanges_add_ancestors(obj, msg->dn, mem_ctx,
-							  sam_ctx, getnc_state,
-							  schema, &session_key,
-							  req10, local_pas,
-							  machine_dn,
-							  &new_objs);
-			if (!W_ERROR_IS_OK(werr)) {
-				return werr;
-			}
-		}
-
 		/*
-		 * Add the object (and any parents it might have) into the
-		 * response message
+		 * Add the object (and, if GET_ANC, any parents it may
+		 * have) into the current chunk of replication data
 		 */
 		getncchanges_add_objs_to_resp(&repl_chunk, new_objs);
 
@@ -3015,8 +3065,7 @@ allowed:
 		talloc_free(getnc_state->last_dn);
 		getnc_state->last_dn = talloc_move(getnc_state, &msg->dn);
 
-		talloc_free(msg_res);
-		talloc_free(msg_dn);
+		TALLOC_FREE(tmp_ctx);
 	}
 
 	getnc_state->num_processed = i;
-- 
2.7.4


From e81cd32efd155be34275d6fde855babc2422ac58 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Tue, 22 Aug 2017 17:32:32 +1200
Subject: [PATCH 12/13] getnchanges.c: Avoid unnecessary continue

There's not really much after the continue that we're skipping now. We
can just flip the logic and avoid the continue.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 source4/rpc_server/drsuapi/getncchanges.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index f018005..eab1794 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -3046,24 +3046,21 @@ allowed:
 					getnc_state->max_usn,
 					&r->out.ctr->ctr6.new_highwatermark);
 
-		if (new_objs == NULL) {
-			DEBUG(8,(__location__ ": getncchanges skipping send of object %s\n",
-				 ldb_dn_get_linearized(msg->dn)));
-			/* nothing to send */
-			TALLOC_FREE(tmp_ctx);
-			continue;
-		}
+		if (new_objs != NULL) {
 
-		/*
-		 * Add the object (and, if GET_ANC, any parents it may
-		 * have) into the current chunk of replication data
-		 */
-		getncchanges_add_objs_to_resp(&repl_chunk, new_objs);
+			/*
+			 * Add the object (and, if GET_ANC, any parents it may
+			 * have) into the current chunk of replication data
+			 */
+			getncchanges_add_objs_to_resp(&repl_chunk, new_objs);
 
-		DEBUG(8,(__location__ ": replicating object %s\n", ldb_dn_get_linearized(msg->dn)));
+			talloc_free(getnc_state->last_dn);
+			getnc_state->last_dn = talloc_move(getnc_state, &msg->dn);
+		}
 
-		talloc_free(getnc_state->last_dn);
-		getnc_state->last_dn = talloc_move(getnc_state, &msg->dn);
+		DEBUG(8,(__location__ ": %s object %s\n",
+			 new_objs ? "replicating" : "skipping send of",
+			 ldb_dn_get_linearized(msg->dn)));
 
 		TALLOC_FREE(tmp_ctx);
 	}
-- 
2.7.4


From 037cc5ed12bcde09f5a6805f3dfb08d12d1a5382 Mon Sep 17 00:00:00 2001
From: Tim Beale <timbeale at catalyst.net.nz>
Date: Wed, 7 Jun 2017 10:46:47 +1200
Subject: [PATCH 13/13] getncchanges.c: Send linked attributes in each chunk

Instead of sending all the linked attributes at the end, add a
configurable option to send the links in each replication chunk.

The benefits of this approach are:
- it can reduce memory overhead, as we don't have to keep all the links
in memory over the entire replication cycle.
- the client should never end up knowing about objects but not their
links. (Although we're not sure that this has actually resulted in
replication problems, i.e. missing links).

Note that until we support GET_TGT, this approach can mean we now send
a link where the client doesn't know about the target object, causing
the client to siliently drop that linked attribute. Hence, this option
is switched off by default.

Implementation-wise, this code works fairly the same as before. Instead
of sorting the entire getnc_state->la_sorted array at the end and then
splitting it up over chunks, we now split the links up over chunks and
then sort them when we copy them into the message. This should be OK, as
I believe the MS-DRSR Doc says the links in the message should be sorted
(rather than sorting *all* the links overall). Windows behaviour seems
to chunk the links based on USN and then sort them.

getnc_state->la_idx now tracks which links in getnc_state->la_list[]
have already been sent (instead of tracking getnc_state->la_sorted).
This means the la_sorted array no longer needs to be stored in
getnc_state and we can free the array's memory once we've copied the
links into the message. Unfortunately, the link_given/link_total debug
no longer reports the correct information, so I've moved these into
getncchanges_state struct (and now free the struct a bit later so it's
safe to reference in the debug).

The vampire_dc testenv has been updated to use this new behaviour.

Signed-off-by: Tim Beale <timbeale at catalyst.net.nz>
Reviewed-by: Andrew Bartlett <abartlet at samba.org>
---
 selftest/target/Samba4.pm                 |  6 +-
 source4/rpc_server/drsuapi/getncchanges.c | 94 ++++++++++++++++++++++---------
 2 files changed, 72 insertions(+), 28 deletions(-)

diff --git a/selftest/target/Samba4.pm b/selftest/target/Samba4.pm
index 39a64ae..f0f7042 100755
--- a/selftest/target/Samba4.pm
+++ b/selftest/target/Samba4.pm
@@ -1288,9 +1288,12 @@ sub provision_vampire_dc($$$)
 	my ($self, $prefix, $dcvars, $fl) = @_;
 	print "PROVISIONING VAMPIRE DC @ FL $fl...\n";
 	my $name = "localvampiredc";
+	my $extra_conf = "";
 
 	if ($fl == "2000") {
-	    $name = "vampire2000dc";
+		$name = "vampire2000dc";
+	} else {
+		$extra_conf = "drs: immediate link sync = yes";
 	}
 
 	# We do this so that we don't run the provision.  That's the job of 'net vampire'.
@@ -1310,6 +1313,7 @@ sub provision_vampire_dc($$$)
 	server max protocol = SMB2
 
         ntlm auth = mschapv2-and-ntlmv2-only
+	$extra_conf
 
 [sysvol]
 	path = $ctx->{statedir}/sysvol
diff --git a/source4/rpc_server/drsuapi/getncchanges.c b/source4/rpc_server/drsuapi/getncchanges.c
index eab1794..dba92a4 100644
--- a/source4/rpc_server/drsuapi/getncchanges.c
+++ b/source4/rpc_server/drsuapi/getncchanges.c
@@ -68,8 +68,11 @@ struct drsuapi_getncchanges_state {
 	struct drsuapi_DsReplicaCursor2CtrEx *final_udv;
 	struct drsuapi_DsReplicaLinkedAttribute *la_list;
 	uint32_t la_count;
-	struct la_for_sorting *la_sorted;
 	uint32_t la_idx;
+
+	/* these are just used for debugging the replication's progress */
+	uint32_t links_given;
+	uint32_t total_links;
 };
 
 /* We must keep the GUIDs in NDR form for sorting */
@@ -2369,8 +2372,6 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 	uint32_t max_objects;
 	uint32_t max_links;
 	uint32_t link_count = 0;
-	uint32_t link_total = 0;
-	uint32_t link_given = 0;
 	struct ldb_dn *search_dn = NULL;
 	bool am_rodc;
 	enum security_user_level security_level;
@@ -2389,6 +2390,7 @@ WERROR dcesrv_drsuapi_DsGetNCChanges(struct dcesrv_call_state *dce_call, TALLOC_
 	bool full = true;
 	uint32_t *local_pas = NULL;
 	struct ldb_dn *machine_dn = NULL; /* Only used for REPL SECRET EXOP */
+	bool immediate_link_sync;
 
 	DCESRV_PULL_HANDLE_WERR(h, r->in.bind_handle, DRSUAPI_BIND_HANDLE);
 	b_state = h->data;
@@ -2906,6 +2908,9 @@ allowed:
 	 */
 	max_links = lpcfg_parm_int(dce_call->conn->dce_ctx->lp_ctx, NULL, "drs", "max link sync", 1500);
 
+	immediate_link_sync = lpcfg_parm_bool(dce_call->conn->dce_ctx->lp_ctx, NULL,
+					      "drs", "immediate link sync", false);
+
 	/*
 	 * Maximum time that we can spend in a getncchanges
 	 * in order to avoid timeout of the other part.
@@ -2954,6 +2959,7 @@ allowed:
 		struct ldb_dn *msg_dn;
 		bool obj_already_sent = false;
 		TALLOC_CTX *tmp_ctx;
+		uint32_t old_la_index;
 
 		tmp_ctx = talloc_new(mem_ctx);
 
@@ -3025,6 +3031,8 @@ allowed:
 			}
 		}
 
+		old_la_index = getnc_state->la_count;
+
 		/*
 		 * We've reached the USN where this object naturally occurs.
 		 * Regardless of whether we've already sent the object (as an
@@ -3062,11 +3070,17 @@ allowed:
 			 new_objs ? "replicating" : "skipping send of",
 			 ldb_dn_get_linearized(msg->dn)));
 
+		getnc_state->total_links += (getnc_state->la_count - old_la_index);
+
 		TALLOC_FREE(tmp_ctx);
 	}
 
 	getnc_state->num_processed = i;
 
+	if (i < getnc_state->num_records) {
+		r->out.ctr->ctr6.more_data = true;
+	}
+
 	/* the client can us to call UpdateRefs on its behalf to
 	   re-establish monitoring of the NC */
 	if ((req10->replica_flags & (DRSUAPI_DRS_ADD_REF | DRSUAPI_DRS_REF_GCSPN)) &&
@@ -3112,25 +3126,32 @@ allowed:
 		max_links -= r->out.ctr->ctr6.object_count;
 	}
 
-	link_total = getnc_state->la_count;
-
-	if (i < getnc_state->num_records) {
-		r->out.ctr->ctr6.more_data = true;
-	} else {
-		/* sort the whole array the first time */
-		if (getnc_state->la_sorted == NULL) {
-			werr = getncchanges_get_sorted_array(getnc_state->la_list,
-							     getnc_state->la_count,
-							     sam_ctx, getnc_state,
-							     schema,
-							     &getnc_state->la_sorted);
-			if (!W_ERROR_IS_OK(werr)) {
-				return werr;
-			}
-		}
-
+	/*
+	 * Work out how many links we can send in this chunk. The default is to
+	 * send all the links last, but there is a config option to send them
+	 * immediately, in the same chunk as their source object
+	 */
+	if (!r->out.ctr->ctr6.more_data || immediate_link_sync) {
 		link_count = getnc_state->la_count - getnc_state->la_idx;
 		link_count = MIN(max_links, link_count);
+	}
+
+	/* If we've got linked attributes to send, add them now */
+	if (link_count > 0) {
+		struct la_for_sorting *la_sorted;
+
+		/*
+		 * Grab a chunk of linked attributes off the list and put them
+		 * in sorted array, ready to send
+		 */
+		werr = getncchanges_get_sorted_array(&getnc_state->la_list[getnc_state->la_idx],
+						     link_count,
+						     sam_ctx, getnc_state,
+						     schema,
+						     &la_sorted);
+		if (!W_ERROR_IS_OK(werr)) {
+			return werr;
+		}
 
 		r->out.ctr->ctr6.linked_attributes_count = link_count;
 		r->out.ctr->ctr6.linked_attributes = talloc_array(r->out.ctr, struct drsuapi_DsReplicaLinkedAttribute, link_count);
@@ -3140,16 +3161,29 @@ allowed:
 		}
 
 		for (k = 0; k < link_count; k++) {
-			r->out.ctr->ctr6.linked_attributes[k]
-				= *getnc_state->la_sorted[getnc_state->la_idx + k].link;
+			r->out.ctr->ctr6.linked_attributes[k] = *la_sorted[k].link;
 		}
 
 		getnc_state->la_idx += link_count;
-		link_given = getnc_state->la_idx;
+		getnc_state->links_given += link_count;
 
 		if (getnc_state->la_idx < getnc_state->la_count) {
 			r->out.ctr->ctr6.more_data = true;
+		} else {
+
+			/*
+			 * We've now sent all the links seen so far, so we can
+			 * reset la_list back to an empty list again. Note that
+			 * the steal means the linked attribute memory gets
+			 * freed after this RPC message is sent on the wire.
+			 */
+			talloc_steal(mem_ctx, getnc_state->la_list);
+			getnc_state->la_list = NULL;
+			getnc_state->la_idx = 0;
+			getnc_state->la_count = 0;
 		}
+
+		TALLOC_FREE(la_sorted);
 	}
 
 	if (req10->replica_flags & DRSUAPI_DRS_GET_NC_SIZE) {
@@ -3165,17 +3199,23 @@ allowed:
 		 * of links we found so far during the cycle.
 		 */
 		r->out.ctr->ctr6.nc_object_count = getnc_state->num_records;
-		r->out.ctr->ctr6.nc_linked_attributes_count = getnc_state->la_count;
+		r->out.ctr->ctr6.nc_linked_attributes_count = getnc_state->total_links;
 	}
 
 	if (!r->out.ctr->ctr6.more_data) {
-		talloc_steal(mem_ctx, getnc_state->la_list);
 
+		/* this is the last response in the replication cycle */
 		r->out.ctr->ctr6.new_highwatermark = getnc_state->final_hwm;
 		r->out.ctr->ctr6.uptodateness_vector = talloc_move(mem_ctx,
 							&getnc_state->final_udv);
 
-		talloc_free(getnc_state);
+		/*
+		 * Free the state info stored for the replication cycle. Note
+		 * that the RPC message we're sending contains links stored in
+		 * getnc_state. mem_ctx is local to this RPC call, so the memory
+		 * will get freed after the RPC message is sent on the wire.
+		 */
+		talloc_steal(mem_ctx, getnc_state);
 		b_state->getncchanges_state = NULL;
 	} else {
 		ret = drsuapi_DsReplicaHighWaterMark_cmp(&r->out.ctr->ctr6.old_highwatermark,
@@ -3211,7 +3251,7 @@ allowed:
 	       r->out.ctr->ctr6.object_count,
 	       i, r->out.ctr->ctr6.more_data?getnc_state->num_records:i,
 	       r->out.ctr->ctr6.linked_attributes_count,
-	       link_given, link_total,
+	       getnc_state->links_given, getnc_state->total_links,
 	       dom_sid_string(mem_ctx, user_sid)));
 
 #if 0
-- 
2.7.4



More information about the samba-technical mailing list