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