Refactor of Binary DN code
Andrew Bartlett
abartlet at samba.org
Fri Nov 6 18:01:41 MST 2009
On Fri, 2009-11-06 at 14:44 +0100, Stefan (metze) Metzmacher wrote:
> Andrew Bartlett schrieb:
> > I wanted to bring your attention to my GIT branch dsdb-dn
> > http://gitweb.samba.org/?p=abartlet/samba.git/.git;a=shortlog;h=refs/heads/dsdb-dn
> >
> > This creates a new struct dsdb_dn and associated functions, taking over
> > from ldb_dn for the DN+Binary and DN+String code.
>
> It looks good!
Thanks!
> Here're sme minor comments:
>
> Can you use a helper variable for dn_type here.
OK.
> + return dsdb_dn_construct_internal(mem_ctx, dn, extra_part,
> dsdb_dn_oid_to_format(oid), oid);
>
> We may need to allow non even number for DN+STRING...
>
> + if ((blen % 2 != 0)) {
> + DEBUG(10, (__location__ ": blen=%u - not an even
> number\n", (unsigned)blen));
> + goto failed;
> + }
Well caught.
> Can you split variable assigments from the declaration and add error
> checks here:
I put the error checks in dsdb_dn_get_with_prefix() to avoid duplication
and make the code simpler.
> +char *dsdb_dn_get_linearized(TALLOC_CTX *mem_ctx,
> + struct dsdb_dn *dsdb_dn)
> +{
> + const char *postfix = ldb_dn_get_linearized(dsdb_dn->dn);
> + return dsdb_dn_get_with_postfix(mem_ctx, dsdb_dn, postfix);
> +}
> +
> +char *dsdb_dn_get_casefold(TALLOC_CTX *mem_ctx,
> + struct dsdb_dn *dsdb_dn)
> +{
> + const char *postfix = ldb_dn_get_casefold(dsdb_dn->dn);
> + return dsdb_dn_get_with_postfix(mem_ctx, dsdb_dn, postfix);
> +}
> +
> +char *dsdb_dn_get_extended_linearized(TALLOC_CTX *mem_ctx,
> + struct dsdb_dn *dsdb_dn,
> + int mode)
> +{
> + char *postfix = ldb_dn_get_extended_linearized(mem_ctx,
> dsdb_dn->dn, mode);
> + char *ret = dsdb_dn_get_with_postfix(mem_ctx, dsdb_dn, postfix);
> + talloc_free(postfix);
> + return ret;
> +}
>
> Can you use a helper variable here too (maybe use ldb_module_get_ctx()
> once on top of the function):
>
> dsdb_dn = dsdb_dn_parse(msg, ldb_module_get_ctx(ac->module), plain_dn,
> attribute->syntax->ldap_oid);
>
> and here too passing a function as argument is always ugly for usage
> in gdb.
>
> - os->dn = ldb_dn_from_ldb_val(os, ldb_module_get_ctx(ac->module),
> plain_dn);
> - if (!os->dn || !ldb_dn_validate(os->dn)) {
> + os->dsdb_dn = dsdb_dn_parse(os, ldb_module_get_ctx(ac->module),
> plain_dn, oid);
> + if (!os->dsdb_dn || !ldb_dn_validate(os->dsdb_dn->dn)) {
Sure.
> We also need to keep in mind that there're some special comparison rules
> and some controls to change the behavior on searches. Sometimes only the
> dn part matters.
I was unable to reproduce any situation where only the DN part mattered
(see ldap.py tests). Can you give me more detail on how I would
reproduce that?
Andrew Bartlett
--
Andrew Bartlett http://samba.org/~abartlet/
Authentication Developer, Samba Team http://samba.org
Samba Developer, Cisco Inc.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 189 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20091107/7d0a638c/attachment.pgp>
More information about the samba-technical
mailing list