[PATCH] s4-drs: Synchronous Implementation of generated ParentGUID - ToDo List ParentGUID fix

[Andrew Tridgell]

Hi Fernando,

 > I'm sending another patch, trying to solve the issues found on the last
 > one...

It looks like the new patch you sent is a delta against your earlier
patch. The usual approach is to only send a delta patch if your
earlier patch had already been accepted into the tree. If none of your
patch has been accepted yet, then send the full patch.

You should also break up this patch into multiple logical pieces
(which you can send as separate emails). I'd suggest 3 patches:

 1) a patch that adds the new utility functions to dsdb/common/util.c

 2) a patch that implements parentGUID in operational.c

 3) a patch that removes the parentGUID code from objectclass.c

This makes it easier to review the patches, and makes for a better
long term record of your work. It also makes it easier to revert
patches if need be, and to accept the patches piece by piece. So for
example I could accept the patch to util.c before the other two are
ready.

You may wish to learn some new git commands to make this easier. Some
particular commands you might like to learn are "git rebase -i", 
"git add --patch" and gitk. There are tutorials for these commands
available if you do a search for them.

I'd also suggest you get yourself a public git repo, perhaps using
repo.or.cz. Then try to neaten up the set of patches in your
repository to have the set of patches you wish to submit.

One common approach is to have multiple branches in your
repository. One branch would hold the patches that you want to be
considered for inclusion in Samba (ie. patches that you think are
ready), and another branch would have your "work in progress" (often
called a 'wip' branch).

If you like I can run through some common git commands for you during
our tutorial session next week.

 > On a try to solve it, I used "" instead of NULL. 

no, please don't use this solution. Much better to pass the correct
list of attributes down the module stack.

 > It doesn't seem to be a nice solution, but if I don't replace for
 > anything, it will return the stored parentGUID for objects that
 > have it...

your change to objectclass.c means that in future we won't have a
parentGUID in those objects. I don't think we want to pass a bad
search attribute down to other modules just to cope with that.

So I think the original code is the best solution:

                for (i=0;i<ARRAY_SIZE(search_sub);i++) {
-                       if (ldb_attr_cmp(ac->attrs[a], search_sub[i].attr) == 0 &&
-                           search_sub[i].replace) {
+                       if (ldb_attr_cmp(ac->attrs[a], search_sub[i].attr) == 0) {
+
                                if (!search_attrs) {

you removed the test in the loop which avoided changing the
search_attrs[] array when search_sub[i].replace was NULL. Doesn't that
do exactly what we want?

To handle existing databases nicely I think you'll need a modification
in operational_search_post_process() to remove the attribute if it's
not wanted. Perhaps you could add a flags field to search_sub[] that
marks the attribute as one that should be removed from search results?

 > I made a few tests and it seems that this search for the parent is necessary
 > to validate de DN at objectclass_do_rename(), but I could remove the
 > objectGUID attribute on this search.

ok

 > I'm going to take a look on the tests scripts to see if I can write
 > something more!

thanks!

now some comments on the patch itself ...

 > +/*
 > +  use a DN to find it's parentGUID
 > + */
 > +int dsdb_find_parentguid_by_dn(struct ldb_context *ldb,
 > +			struct ldb_dn *dn, struct GUID **parent_guid)
 > +{

why does this take a struct GUID ** ? That would seem to imply that it
returns allocated memory, but if you want to return allocated memory
then you should pass in a memory context to put it on. Currently you
allocate it as a child of the ldb, which could lead to memory leaks.

I think it would be better if this function took a "struct GUID *" and
left it up to the caller to allocate the GUID (perhaps just as a stack
variable).

 > +/*
 > +  Generate the parentGUID and adds it to the message
 > + */
 > +int dsdb_msg_add_parentguid(struct ldb_context *ldb, struct ldb_message *msg)

I don't think this is a good function for util.c. Things that go into
util.c should be more generic.

Instead I'd suggest you have this function:

  int dsdb_msg_add_guid(struct ldb_context *ldb, struct ldb_message *msg,
      			struct GUID *guid);

and then call both dsdb_find_parentguid_by_dn() and
dsdb_msg_add_guid() from operational.c.

You might also like to look for other places in the code that
currently add a GUID manually to a message and see if you can make
them instead use your new dsdb_msg_add_guid() function.

For example, look in repl_meta_data.c, in replmd_add(). You'll find a
bit of code that is adding a GUID to a message. That would be much
neater with your function. I think you'll find some other places too
(for example, in objectguid_add() in objectguid.c).

 > +{
 > +	struct GUID *parent_guid;
 > +	int ret;
 > +	enum ndr_err_code ndr_err;
 > +	struct ldb_val v;
 > +
 > +	parent_guid = talloc(msg, struct GUID);
 > +	ret = dsdb_find_parentguid_by_dn(ldb, msg->dn, &parent_guid);
 > +
 > +	if(ret != LDB_SUCCESS){
 > +		return ret;
 > +	}

In the above you have a memory leak if the function fails (you
allocated parent_guid but didn't free it. 


 > +	ret = ldb_msg_add_value(msg, "parentGUID", &v, NULL);
 > +	if (ret != LDB_SUCCESS) {
 > +		DEBUG(4,(__location__ ": Failed to get parentGUID for dn %s\n",
 > +					 ldb_dn_get_linearized(msg->dn)));
 > +		return ret;
 > +	}

be careful when you cut&paste that you get the message right. The
above debug message makes no sense for this error condition.

 >  	ret = ldb_build_search_req(&search_req, ldb,
 > -				   ac, parent_dn, LDB_SCOPE_BASE,
 > -				   "(objectClass=*)",
 > -				   attrs, NULL, 
 > -				   ac, get_search_callback,
 > -				   req);
 > +					   ac, parent_dn, LDB_SCOPE_BASE,
 > +					   "(objectClass=*)",
 > +					   NULL, NULL,
 > +					   ac, get_search_callback,
 > +					   req);
 > +

In the above you changed the code from looking for just the
"objectGUID" attribute to looking for the NULL attribute list. When
you search for the NULL attribute list in ldb it means that you want
all attributes, not no attributes. So instead you should pass the
empty list (which means no attributes).

Cheers, Tridge