[PATCH] s4:drs Create connection object (nTDSConnection)

[Crístian Viana]

hi Tridge,

If you do decide to add this helper, then you could send a separate

patch that makes the other places in Samba that add DNs to also use

this function.


I added this function on a separate branch here, I'll update the other
places where this new function should be used. but how am I going to find
every occurrence of adding a DN to an ldb_message? I believe this "git grep"
isn't enough because there may be other DN variable names out there without
"dn" on it. but I'll change as much as I can and I'll send this separate
patch.

2) you need a lot more error checking.


I added some error checking and memory freeing in the code.

3) you should probably add a higher level debug message (maybe level 2) when
> you create a new object or delete one


done.

You should either make the function return void, or put the return code into
> a variable in the caller and print an appropriate message if it fails.


now it returns void. if the function returns an error code, I don't know
what to do with it in the caller function (kccsrv_simple_update). I don't
think I can abort it, and a DEBUG message is already displayed for every
error on kccsrv_create_connection.

5) Why are you adding an invocationId ?


I don't see this attribute on Windows too, but if I don't do it, when adding
the ldb_message I see this error:

Failed to find our own NTDS Settings invocationId in the ldb!

ldb: replmd_add: unable to find invocationId

so that's what I thought I should do. I don't even know if that's the right
ID value, but the function samdb_ntds_invocation_id just seemed to be what I
needed. how do I get rid of this error without setting an invocationID?

you should also now compare the resulting objects with what windows produces
> and look into any other discrepancies. Also make sure you test the case
> when a connection should be deleted.


I'll do this and the issue #1 tomorrow, along with metze's suggestions on
the previous e-mail.

I also noticed that the invocationId is not displaying as a nicely formatted
> GUID in ldbsearch.


I see, it's displaying some random characters. it's probably due to the
wrong way I'm adding it to the ldb_message (as a string).

the current patch is attached. I'm also pushing my changes to the online
repository hosted at git://repo.or.cz/Samba/cd1.git. for now, the code is on
a branch called 'cd1', I'm struggling to move the code to the 'master'
branch, git is still something new to me...

I'll send another patch soon.

cheers,

On Sat, Nov 14, 2009 at 12:16 AM, <tridge at samba.org> wrote:

> Hi Cristian,
>
> Thanks for the patch!
>
> The problem you are getting with the fromServer attribute is because
> you are treating it as a binary blob, not a string. When you're trying
> to work out how to deal with a particular attribute type it can be
> useful to look at how it is dealt with elsewhere in Samba. So for
> example you could run this:
>
>  git grep ldb_msg_add | grep dn
>
> and you'll see lots of places that add a dn to a message. You'll see
> that they treat it as a string.
>
> It is also arguarble that we should add a new helper function in
> ldb_msg.c called ldb_msg_add_dn() which takes a 'struct ldb_dn *'
> directly. It would look something like this:
>
> int ldb_msg_add_dn(struct ldb_message *msg,
>                   const char *attr_name, struct ldb_dn *dn)
> {
>        return ldb_msg_add_string(msg, attr_name,
> ldb_dn_get_linearized(dn));
> }
>
> If you do decide to add this helper, then you could send a separate
> patch that makes the other places in Samba that add DNs to also use
> this function.
>
> After you fix the above in your patch you'll find that it will create
> the object successfully. There are some other issues with the patch
> that you should fix up however.
>
>  1) Each time the kcc runs it re-runs this function. That means it
>  keeps creating new connection objects, whereas we only want one
>  connection object per DC that we are replicating with. We also need
>  to remove connection objects from DCs that we are no longer
>  replicating with (ie. if the nTDSDSA object of that DC is gone). So
>  you'll need to do a LDB_SCOPE_ONELEVEL search under our nTDSDSA
>  object to find all the existing connection objects we have, and then
>  only add missing ones, plus delete ones that should not be there. So
>  you'll probably need one function that works out a list of what
>  connections we should have, then a separate function that applies
>  that, adding/deleting as needed.
>
>  2) you need a lot more error checking. There are lots of functions
>  in kccsrv_create_connection() that could fail, and you need to
>  report an appropriate error if they do fail. This is especially the
>  case for any calls that do searches (such as
>  dsdb_find_dn_by_guid()). If in doubt, please err on the side of
>  checking error returns rather than not. When something fails we need
>  to know what went wrong by looking at the logs.
>
>  3) you should probably add a higher level debug message (maybe level
>  2) when you create a new object or delete one, saying what object
>  you are creating, and what server it is for. The admin can then see
>  what happened by looking at the logs.
>
>  4) your function kccsrv_create_connection() returns an error, but
>  the caller never looks at it. You should either make the function
>  return void, or put the return code into a variable in the caller
>  and print an appropriate message if it fails.
>
>  5) Why are you adding an invocationId ? I don't see one on
>  nTDSConnection objects on windows, and I don't see it as an
>  attribute for this class in the schema. (the invocationId you are
>  adding also looks wrong - GUIDs need to be added as NDR objects).
>
> Once you fix the fromServer problem, you should also now compare the
> resulting objects with what windows produces and look into any
> other discrepancies. Also make sure you test the case when a
> connection should be deleted.
>
> While looking at your patch I also noticed that the invocationId is
> not displaying as a nicely formatted GUID in ldbsearch.
>
>  > By the way, I'm not sure if I should send this patch as a top-level
> e-mail
>  > (like this one) or as a reply to the previous e-mail discussing this
>  > implementation.
>
> It is good to do it as a new mail, with a [PATCH] prefix just like
> you've done. Thanks!
>
> Cheers, Tridge
>



-- 
Crístian Deives dos Santos Viana [aka CD1]
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-create-a-valid-fromServer-attribute-on-an-nTDSConnec.patch
Type: application/octet-stream
Size: 3199 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20091116/4854a9b1/attachment.obj>