Todo List: Working on ParentGUID Fix

[Fernando J V da Silva]

Hi there!

I'm starting working on the ParentGUID Fix issue mentioned on Samba Todo
List (http://wiki.samba.org/index.php/Samba4_DRS_TODO_List#Parentguid_fix) .
I've been following a set of recommendations pointed out by Tridge.

This is going to be my first code contribution for samba ... so, sorry for
any basic question ...

Following are some of Tridge's recommendations and doubts I have about it
and about the related code for this task.

> 2) in dsdb/samdb/ldb_modules/operational.c, change
> operational_search() to look for the attribute "parentGUID"

If I understood it right, in operational_search() it looks for  attributes
that are searchable but stored with different names and hidden attributes,
right?
If so, should I include the "parentGUID" into the list of hidden attributes,
creating a new element on "search_sub" for parentGUID and a function that
"construct" the parentGUID dynamically? I've done a few experiments related
to this and it seems to work ... (I created such element and a function to
return a dummy parentGUID) but would it be a way to start coding a
right/suitable solution?

I suppose that, for fixing this issue, the parentGUID for existing objects
also shouldn't be returned when it isn't mentioned on search attributes,
right? So I should do something to avoid it to be shown without the
parentGUID search parameter ... But I still didn't found where exactly in
the code the attributes are gotten from the database (i.e. in which function
the parentGUID and others attributes are read from the database currently?).
Could somebody give me some tip?


Best Regards,


--
Fernando J V da Silva
M Sc Computer Science Student
Institute of Computing, State University of Campinas
[phone]

Hi Fernando,

 > If so, should I include the "parentGUID" into the list of hidden attributes,
 > creating a new element on "search_sub" for parentGUID and a function that
 > "construct" the parentGUID dynamically? I've done a few experiments related
 > to this and it seems to work ... (I created such element and a function to
 > return a dummy parentGUID) but would it be a way to start coding a
 > right/suitable solution?

yes, that will work. The "replace" value for parentGUID in
search_sub[] will be NULL, but that is OK.

This approach has the advantage of simplicity, although it doesn't
follow the normal pattern of doing searches in modules in an
asynchronous manner. Async coding in ldb modules is quite hard, so I
suggest you first get it all working in a simple synchronous manner,
which means a constructor function that builds the parent DN and
searches for its objectGUID.

The code in rpc_server/drsuapi/getncchanges.c in
get_nc_changes_build_object() that calculates the parent_dn then calls
dsdb_find_guid_by_dn() is close to what you want.

 > I suppose that, for fixing this issue, the parentGUID for existing objects
 > also shouldn't be returned when it isn't mentioned on search attributes,
 > right?

you should just remove the code in objectclass.c that adds the
parentGUID to objects when they are added. That code is incorrect, and
we should never have stored the parentGUID in the database in the
first place.

That will probably be the trickiest part of what you are working
on. The parentguid code in objectclass.c is async, so removing it
requires some quite careful work.

 > So I should do something to avoid it to be shown without the
 > parentGUID search parameter

you could do that, and that would help existing deployments of Samba4,
but it is not critical. Once you remove the code in objectclass.c that
adds parentGUID to the database then a new provision will leave it
clean.

 > ... But I still didn't found where exactly in the code the
 > attributes are gotten from the database (i.e. in which function the
 > parentGUID and others attributes are read from the database
 > currently?).  Could somebody give me some tip?

It currently shows up because parentGUID is stored as a "normal"
attribute in the database, and all "normal" attributes match either a
NULL attribute list or a wildcard (ie. "*") attribute.

The function that handles this for the tdb backend is
ltdb_filter_attrs(), in lib/ldb/ldb_tdb/ldb_search.c. You don't need
to modify any of that for what you are doing however, the trick is to
ensure that parentGUID is not stored in the database in the first
place (at least for new objects).

Cheers, Tridge

[Fernando J V da Silva]

Hi Tridge! Thanks for reply!

So I'm going to do the synchronous manner firstly! Then the async mode
afterwards. =)

Best Regards,

--
Fernando J V da Silva
M Sc Computer Science Student
Institute of Computing, State University of Campinas
[phone]

[Fernando J V da Silva]

Hi Tridge!

This approach has the advantage of simplicity, although it doesn't
> follow the normal pattern of doing searches in modules in an
> asynchronous manner. Async coding in ldb modules is quite hard, so I
> suggest you first get it all working in a simple synchronous manner,
> which means a constructor function that builds the parent DN and
> searches for its objectGUID.
>
>
I'm starting thinking about the Async coding... I debuged a few code through
modules requests but I still have a few doubts about how it works ... Is
there any documentation about the modules and/or any similar existing code
that I could follow as an example for the Async implementation?

When operational_search() is called, it builds a search request with
operation_callback as the callback function... That function is only called
after the search is done, right? I couldn't find where the reply message
passed to this function is filled (I suppose there would be the place where
I should fill with the parentGUID, would it be right?), could you give me
some tip?



--
Fernando J V da Silva
M Sc Computer Science Student
Institute of Computing, State University of Campinas
[phone]

Hi Fernando,

It is best to get it all working correctly before trying to make it
async. See my previous reply for what still needs to be fixed.

 > I'm starting thinking about the Async coding... I debuged a few code through
 > modules requests but I still have a few doubts about how it works ... Is
 > there any documentation about the modules and/or any similar existing code
 > that I could follow as an example for the Async implementation?

There is no documentation, but there are plenty of examples in the
dsdb/samdb/ldb_modules/ directory. I should warn you that it is quite
tricky to get right. Andrew and I are still discussing whether we
should just avoid using the async design for new code in dsdb ldb
modules, as it is so hard to code, and makes the code very difficult
to read.

If you want to do it anyway, then have a look at the code you removed
in objectclass.c. That code added the parentGUID in a rename in an
async fashion.

 > When operational_search() is called, it builds a search request with
 > operation_callback as the callback function... That function is only called
 > after the search is done, right?

that's right.

 > I couldn't find where the reply message passed to this function is
 > filled (I suppose there would be the place where I should fill with
 > the parentGUID, would it be right?), could you give me some tip?

have a close look at objectclass_rename_callback(). See that it gets a
result in ac->search_res->message?

I'd suggest you add a lot of DEBUG() messages to an existing piece of
async code (such as the example above) and then trace through how it
works. It can be a bit mind-bending, but if you look at it for a while
then I hope it will start to make sense.

btw, the reason we put this async code in is that ldb can use a remote
ldap database as a backend. In that case an ldb call might have to
wait a few seconds for a remote ldap server to complete an
operation. By doing the operation in an async fashion it allows other
processing in Samba to continue while the ldap server is
responding. This is particularly important when Samba is running in
single process mode (ie. with -M single).

It isn't a common use case however, and it could be argued that we
should just use a new event context for ldb, and always treat it as a
"local database" in the s4 dsdb ldb modules. In that case we would
assume that the backend is "sufficiently responsive" and we would not
need all this async code.

We're still thinking about this, so if you find the async code too
hard, then you should know that you are not along in thinking this,
and perhaps we will just make future changes to dsdb modules be sync,
like you did in your initial patch.

Cheers, Tridge