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