big ldb patch

[Stefan (metze) Metzmacher]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

tridge at samba.org schrieb:
> 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 :-)

very good idea, then we can also add a GUID and dom_sid to ldb_dn, and then we can support
extended dn's later

> 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.

looks good

- --
metze

Stefan Metzmacher <metze at samba.org> www.samba.org
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.2.3-nr1 (Windows XP)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFDA/Okm70gjA5TCD8RAipNAJ9N+VXkr+ci2D5rKwk99j64SKDQfQCfbavK
MwieaI8QjfW6WBYoeQB+oHs=
=EeLA
-----END PGP SIGNATURE-----