[PATCH] s4-drs: Utility functions to deal with GUID

[tridge at samba.org]

Hi Fernando,

This patch is very close now, with just a couple of minor things to
fix before it can be accepted. Good work!

 > +	res = talloc_zero(tmp_ctx, struct ldb_result);
 > +	if (!res) {
 > +		DEBUG(0, (__location__ ": out of memory\n"));
 > +		ret = LDB_ERR_OPERATIONS_ERROR;
 > +		goto done;
 > +	}

In the above (in dsdb_find_parentguid_by_dn()) you allocate a struct
ldb_result and then pass the allocated memory down to
dsdb_search_dn_with_deleted(). This isn't needed as
dsdb_search_dn_with_deleted() already allocates that structure, and it
returns it to the caller via a talloc_steal() onto a caller supplied
memory context.

So the fix is just to remove the above allocation.

 > +	if(parent_dn == NULL){
 > +		DEBUG(4,(__location__ ": Failed to find parent for dn %s\n",
 > +					 ldb_dn_get_linearized(dn)));
 > +		ret = LDB_ERR_NO_SUCH_OBJECT;
 > +		goto done;
 > +	}

a very minor point, but you should try and follow the coding style for
whitespace. You'll notice that Samba code would put spaces in that if
statement, like this

 	if (parent_dn == NULL) {

it makes the code more readable to use the same conventions
throughout. We have a coding style guideline in the file README.Coding
in the root of the git tree.

 > +int dsdb_msg_add_guid(struct ldb_context *ldb,
 > +		struct ldb_message *msg,
 > +		struct GUID *guid,
 > +		char* attr_name)
 > +{

I don't think you need the 'ldb' parameter there (you don't use it in
the function), and the guid and attr_name parameters should be const
as they are not modified by the function. So I think the function
should look like this:

 int dsdb_msg_add_guid(struct ldb_message *msg, const struct GUID *guid, const char *attr_name);

you could alternatively just pass the GUID as a structure, like this;

 int dsdb_msg_add_guid(struct ldb_message *msg, struct GUID guid, const char *attr_name);

either one would be fine.

 > +	ndr_err = ndr_push_struct_blob(&v, NULL, NULL,
 > +				       guid,
 > +				       (ndr_push_flags_fn_t)ndr_push_GUID);

in this call you are passing NULL for the mem_ctx argument to
ndr_push_struct_blob(). That means that when it allocates the memory
for the blob it will be allocated on the NULL context, which leads to
a memory leak.

It is important in Samba to always be aware what memory context you
are using. In this case you will need to allocate a tmp_ctx in 
dsdb_msg_add_guid() and pass that to ndr_push_struct_blob(). 

There is also a bit of a trap in ldb_msg_add_value(). If you look
inside that function you'll see that it does this:

	el->values[el->num_values] = *val;

so it just copies the structure. It does not allocate/copy the data
itself. 

What you want in this case is for the data to end up being owned by
the element in the ldb_message structure that holds the data. The
correct call to do that is ldb_msg_add_steal_value(). If you have a
look at that function you'll see that it is a wrapper around
ldb_msg_add_value(), but it additionally does this:

		talloc_steal(el->values, val->data);

that has the effect of transferring ownership of the data to the
element in the ldb_message.

So if you use a tmp_ctx in ndr_push_struct_blob() and then use
ldb_msg_add_steal_value() instead of ldb_msg_add_value() you will get
what you want, which is the GUID being added to the ldb_message with
the correct memory hierarchy.

It can take a bit of time to get used to the memory hierarchy that
talloc brings to Samba, but it is worth the effort. Just try to keep
in mind who owns the memory that you are allocating.

Cheers, Tridge