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 = (msg->dn);
+var = (ldb_dn_linearize(msg, msg->dn));
the 'name' argument to () isn't the contents, its just a
label, so something like:
var = ("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, TALLC_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

PGP SIGNED MESSAGE
Hash: SHA1

tridge (AT) samba (DOT) 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 = (msg->dn);
+var = (ldb_dn_linearize(msg, msg->dn));

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

var = ("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, TALLC_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.orgwww.samba.org
PGP SIGNATURE
Version: GnuPG v1.2.3-nr1 (Windows XP)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

MwieaI8QjfW6WBYoeQB+oHs=
=EeLA
PGP SIGNATURE