big ldb patch

tridge at samba.org tridge at samba.org
Thu Aug 18 01:38:42 GMT 2005


Simo,

ok, you've convinced me that pushing ldb_dn more widely is a good
thing. Just to show you that I did read the patch, here are some
(fairly minor!) comments :-)

btw, I've CCd the list, as others may be interested. My apologise for
the lack of context ....

 > -		domain_dn = samdb_result_string(msgs_domain[0], "nCName", NULL);
 > +		domain_dn = ldb_dn_explode(mem_ctx, samdb_result_string(msgs_domain[0], "nCName", NULL));

I think it would be better to create a samdb_result_dn() call rather
than inlining it like this. It could then do some error checking :-)

I see quite a few calls like the above, obviously all of them should
use samdb_result_dn().

 > -	var = mprObject(msg->dn);
 > +	var = mprObject(ldb_dn_linearize(msg, msg->dn));

the 'name' argument to mprObject() isn't the contents, its just a
label, so something like:

 var = mprObject("dn")

would be better (I know this isn't your bug, just something I noticed
due to your patch).

 > -NTSTATUS samdb_allocate_next_id(struct ldb_context *sam_ldb, TALLOC_CTX *mem_ctx, const char *dn, const char *attr,
 > -				uint32_t *id)
 > -{

maybe you could do the cleanup of this code as a separate commit? It
looks like you are just removing dead code, but it seems unrelated to
the ldb dn changes.

 > -		msg->dn = talloc_asprintf(mem_ctx, "CN=%s,CN=%s,%s",
 > -					  cn_name, container, state->base_dn[database]);
 > +		msg->dn = ldb_dn_build_child(mem_ctx,
 > +					     "CN", cn_name,
 > +					     ldb_dn_build_child(mem_ctx,
 > +								"CN", container,
 > +								state->base_dn[database]));

hmm, thats ugly ...

Maybe change ldb_dn_build_child() be varargs, so you can add multiple
levels at once? Like this:

 msg->dn = ldb_dn_build_child(mem_ctx, state->base_dn[database],
			      "CN", container, "CN", cn_name, NULL);

If you are not familiar with this sort of varargs() trick then have a
look at security_descriptor_create(), which does something very
similar.

Cheers, Tridge


More information about the samba-technical mailing list