Refactor of Binary DN code

[Andrew Bartlett]

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.

I feel this is a cleaner separation than we had previously, and should
fix some reported issues with the OpenLDAP backend.  (We need to be able
to deal with the linear form of these DNs as a whole, and as the
'normal' part differently in different portions of the code).

It also adds functions to allow these DNs to be searched for correctly,
as the DN is now able to be casefolded independently of the binary or
string prefix.

The next step is to write specific tests for this new code, and to
validate that I've not broken 'net vampire' and replication (for which
this code was added in the first place).

Andrew Bartlett
--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Andrew Bartlett]

On Thu, 2009-11-05 at 18:02 +1100, Andrew Bartlett wrote:

> 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.
>
> I feel this is a cleaner separation than we had previously, and should
> fix some reported issues with the OpenLDAP backend.  (We need to be able
> to deal with the linear form of these DNs as a whole, and as the
> 'normal' part differently in different portions of the code).
>
> It also adds functions to allow these DNs to be searched for correctly,
> as the DN is now able to be casefolded independently of the binary or
> string prefix.
>
> The next step is to write specific tests for this new code, and to
> validate that I've not broken 'net vampire' and replication (for which
> this code was added in the first place).

I've pushed an updated set of changes to my branch.

The problem I have, aside from a small segfault mdw will patch shortly
in his ValidatePassword code, is that dcpromo on a new second Windows
2008 DC fails with:

"The replication system has encountered an internal error"

while replicating cn=configuration

How do I debug something like this?

(otherwise, all of 'make test' passes fine)

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Stefan (metze) Metzmacher]

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! Here're sme minor comments:

Can you use a helper variable for dn_type here.

+       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;
+       }


Can you split variable assigments from the declaration and add error
checks here:

+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)) {


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.

metze

[Stefan (metze) Metzmacher]

Andrew Bartlett schrieb:

> On Thu, 2009-11-05 at 18:02 +1100, Andrew Bartlett wrote:
>> 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.
>>
>> I feel this is a cleaner separation than we had previously, and should
>> fix some reported issues with the OpenLDAP backend.  (We need to be able
>> to deal with the linear form of these DNs as a whole, and as the
>> 'normal' part differently in different portions of the code).
>>
>> It also adds functions to allow these DNs to be searched for correctly,
>> as the DN is now able to be casefolded independently of the binary or
>> string prefix.
>>
>> The next step is to write specific tests for this new code, and to
>> validate that I've not broken 'net vampire' and replication (for which
>> this code was added in the first place).
>
> I've pushed an updated set of changes to my branch.
>
> The problem I have, aside from a small segfault mdw will patch shortly
> in his ValidatePassword code, is that dcpromo on a new second Windows
> 2008 DC fails with:
>
> "The replication system has encountered an internal error"
>
> while replicating cn=configuration

Does it work without your changes?

metze

[Andrew Bartlett]

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.

[Andrew Bartlett]

On Fri, 2009-11-06 at 14:44 +0100, Stefan (metze) Metzmacher wrote:

> Andrew Bartlett schrieb:
> > On Thu, 2009-11-05 at 18:02 +1100, Andrew Bartlett wrote:
> >> 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.
> >>
> >> I feel this is a cleaner separation than we had previously, and should
> >> fix some reported issues with the OpenLDAP backend.  (We need to be able
> >> to deal with the linear form of these DNs as a whole, and as the
> >> 'normal' part differently in different portions of the code).
> >>
> >> It also adds functions to allow these DNs to be searched for correctly,
> >> as the DN is now able to be casefolded independently of the binary or
> >> string prefix.
> >>
> >> The next step is to write specific tests for this new code, and to
> >> validate that I've not broken 'net vampire' and replication (for which
> >> this code was added in the first place).
> >
> > I've pushed an updated set of changes to my branch.
> >
> > The problem I have, aside from a small segfault mdw will patch shortly
> > in his ValidatePassword code, is that dcpromo on a new second Windows
> > 2008 DC fails with:
> >
> > "The replication system has encountered an internal error"
> >
> > while replicating cn=configuration
>
> Does it work without your changes?

No, but for a different reason.  Without my changes it loops at 1606 of
1625 changes in cn=configuration (which is also the same as the last
time I tested replication).

So, how do I debug what it doesn't like about my patch?  

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Andrew Bartlett]

On Sat, 2009-11-07 at 13:38 +1100, Andrew Bartlett wrote:

> On Fri, 2009-11-06 at 14:44 +0100, Stefan (metze) Metzmacher wrote:
> > Andrew Bartlett schrieb:
> > > On Thu, 2009-11-05 at 18:02 +1100, Andrew Bartlett wrote:
> > >> 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.
> > >>
> > >> I feel this is a cleaner separation than we had previously, and should
> > >> fix some reported issues with the OpenLDAP backend.  (We need to be able
> > >> to deal with the linear form of these DNs as a whole, and as the
> > >> 'normal' part differently in different portions of the code).
> > >>
> > >> It also adds functions to allow these DNs to be searched for correctly,
> > >> as the DN is now able to be casefolded independently of the binary or
> > >> string prefix.
> > >>
> > >> The next step is to write specific tests for this new code, and to
> > >> validate that I've not broken 'net vampire' and replication (for which
> > >> this code was added in the first place).
> > >
> > > I've pushed an updated set of changes to my branch.
> > >
> > > The problem I have, aside from a small segfault mdw will patch shortly
> > > in his ValidatePassword code, is that dcpromo on a new second Windows
> > > 2008 DC fails with:
> > >
> > > "The replication system has encountered an internal error"
> > >
> > > while replicating cn=configuration
> >
> > Does it work without your changes?
>
> No, but for a different reason.  Without my changes it loops at 1606 of
> 1625 changes in cn=configuration (which is also the same as the last
> time I tested replication).
>
> So, how do I debug what it doesn't like about my patch?  

I've fixed what I thought was the cause (a bug in the schema_syntax
changes), and written unit tests for many of the syntax handlers.
However, despite fixing that, and make test passing, it still fails the
dcpromo.

I've updated the dsdb-dn branch, and would appreciate any further
review.  (My environment for reproducing the dcpromo has just broken, so
I'll get back to painful debugging once I get this rebuilt).

Thanks,

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Andrew Bartlett]

On Tue, 2009-11-10 at 22:38 +1100, Andrew Bartlett wrote:

> On Sat, 2009-11-07 at 13:38 +1100, Andrew Bartlett wrote:
> > On Fri, 2009-11-06 at 14:44 +0100, Stefan (metze) Metzmacher wrote:
> > > Andrew Bartlett schrieb:
> > > > On Thu, 2009-11-05 at 18:02 +1100, Andrew Bartlett wrote:
> > > >> 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.
> > > >>
> > > >> I feel this is a cleaner separation than we had previously, and should
> > > >> fix some reported issues with the OpenLDAP backend.  (We need to be able
> > > >> to deal with the linear form of these DNs as a whole, and as the
> > > >> 'normal' part differently in different portions of the code).
> > > >>
> > > >> It also adds functions to allow these DNs to be searched for correctly,
> > > >> as the DN is now able to be casefolded independently of the binary or
> > > >> string prefix.
> > > >>
> > > >> The next step is to write specific tests for this new code, and to
> > > >> validate that I've not broken 'net vampire' and replication (for which
> > > >> this code was added in the first place).
> > > >
> > > > I've pushed an updated set of changes to my branch.
> > > >
> > > > The problem I have, aside from a small segfault mdw will patch shortly
> > > > in his ValidatePassword code, is that dcpromo on a new second Windows
> > > > 2008 DC fails with:
> > > >
> > > > "The replication system has encountered an internal error"
> > > >
> > > > while replicating cn=configuration
> > >
> > > Does it work without your changes?
> >
> > No, but for a different reason.  Without my changes it loops at 1606 of
> > 1625 changes in cn=configuration (which is also the same as the last
> > time I tested replication).
> >
> > So, how do I debug what it doesn't like about my patch?  
>
> I've fixed what I thought was the cause (a bug in the schema_syntax
> changes), and written unit tests for many of the syntax handlers.
> However, despite fixing that, and make test passing, it still fails the
> dcpromo.

In addition to my unit tests, the dsdb-dn branch now also contains a
revived RPC-DSSYNC test that loads the schema from the remote server and
uses that to pull all the attributes into the LDAP format.  We then
verify that the pull was correct by comparison against the LDAP server
on the remote host.

However, currently the test is too slow to enable by default (it does
pass against Windows and Samba), partly because it tries to verify the
attribute IDs for every object.  

However, as far as I can see in _drs_util_verify_attids(), the only
verification is done by eye - not by software.  Could we remove this
part of the test, now we actually use the remote schema?

Thanks,

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Kamen Mazdrashki-2]

On Wed, Nov 11, 2009 at 14:07, Andrew Bartlett <abartlet@...> wrote:
>
> In addition to my unit tests, the dsdb-dn branch now also contains a
> revived RPC-DSSYNC test that loads the schema from the remote server and
> uses that to pull all the attributes into the LDAP format.  We then
> verify that the pull was correct by comparison against the LDAP server
> on the remote host.

Sounds great!

>
> However, currently the test is too slow to enable by default (it does
> pass against Windows and Samba), partly because it tries to verify the
> attribute IDs for every object.

I've committed a patch for that (I wanted to enable DSSYNC test in 'make test',
and it was really slow)
http://repo.or.cz/w/Samba/kamenim.git/commit/9966595ac1a185b87f47ebd8ce3f651755060d34

>
> However, as far as I can see in _drs_util_verify_attids(), the only
> verification is done by eye - not by software.  Could we remove this
> part of the test, now we actually use the remote schema?
>
Main purpose of _drs_util_verify_attids() is to verify decoded OID
is real one - i.e. decoding ATTID based on drsuapi_prefixMap leads
to an OID that actually exists.
So please, don't remove it.
The cache i've implemented lowers execution time significantly.

CU,
Kamen Mazdrashki
kamen.mazdrashki@...
http://repo.or.cz/w/Samba/kamenim.git
-------------------------------------
CISCO SYSTEMS BULGARIA EOOD
http://www.cisco.com/global/BG/

[Kamen Mazdrashki]

Hi Andrew,

I've merged it with your work on using LDB interface
(actually this is what I wanted to do next, so thanks).
Here is the branch and patch:
http://repo.or.cz/w/Samba/kamenim.git/shortlog/refs/heads/drsuapi-wip
http://repo.or.cz/w/Samba/kamenim.git/commitdiff/d07abebc6309628a60316eef246338ac09d88c3d


BR,
Kamen Mazdrashki
kamen.mazdrashki@...
http://repo.or.cz/w/Samba/kamenim.git
-------------------------------------
CISCO SYSTEMS BULGARIA EOOD
http://www.cisco.com/global/BG/

> -----Original Message-----
> From: samba-technical-bounces@... [mailto:samba-technical-
> bounces@...] On Behalf Of Kamen Mazdrashki
> Sent: Wednesday, November 11, 2009 3:04 PM
> To: Andrew Bartlett
> Cc: Stefan (metze) Metzmacher; 'tridge@...'; 'samba-
> technical@...'
> Subject: Re: Improved RPC-DSSYNC
>
> On Wed, Nov 11, 2009 at 14:07, Andrew Bartlett <abartlet@...>
> wrote:
> >
> > In addition to my unit tests, the dsdb-dn branch now also contains a
> > revived RPC-DSSYNC test that loads the schema from the remote server
> and
> > uses that to pull all the attributes into the LDAP format.  We then
> > verify that the pull was correct by comparison against the LDAP
> server
> > on the remote host.
>
> Sounds great!
>
> >
> > However, currently the test is too slow to enable by default (it does
> > pass against Windows and Samba), partly because it tries to verify
> the
> > attribute IDs for every object.
>
> I've committed a patch for that (I wanted to enable DSSYNC test in
> 'make test',
> and it was really slow)
> http://repo.or.cz/w/Samba/kamenim.git/commit/9966595ac1a185b87f47ebd8ce
> 3f651755060d34
>
> >
> > However, as far as I can see in _drs_util_verify_attids(), the only
> > verification is done by eye - not by software.  Could we remove this
> > part of the test, now we actually use the remote schema?
> >
> Main purpose of _drs_util_verify_attids() is to verify decoded OID
> is real one - i.e. decoding ATTID based on drsuapi_prefixMap leads
> to an OID that actually exists.
> So please, don't remove it.
> The cache i've implemented lowers execution time significantly.
>
> CU,
> Kamen Mazdrashki
> kamen.mazdrashki@...
> http://repo.or.cz/w/Samba/kamenim.git
> -------------------------------------
> CISCO SYSTEMS BULGARIA EOOD
> http://www.cisco.com/global/BG/

[Andrew Bartlett]

On Wed, 2009-11-11 at 15:04 +0200, Kamen Mazdrashki wrote:

> On Wed, Nov 11, 2009 at 14:07, Andrew Bartlett <abartlet@...> wrote:
> >
> > In addition to my unit tests, the dsdb-dn branch now also contains a
> > revived RPC-DSSYNC test that loads the schema from the remote server and
> > uses that to pull all the attributes into the LDAP format.  We then
> > verify that the pull was correct by comparison against the LDAP server
> > on the remote host.
>
> Sounds great!
>
> >
> > However, currently the test is too slow to enable by default (it does
> > pass against Windows and Samba), partly because it tries to verify the
> > attribute IDs for every object.
>
> I've committed a patch for that (I wanted to enable DSSYNC test in 'make test',
> and it was really slow)
> http://repo.or.cz/w/Samba/kamenim.git/commit/9966595ac1a185b87f47ebd8ce3f651755060d34
>
> >
> > However, as far as I can see in _drs_util_verify_attids(), the only
> > verification is done by eye - not by software.  Could we remove this
> > part of the test, now we actually use the remote schema?
> >
> Main purpose of _drs_util_verify_attids() is to verify decoded OID
> is real one - i.e. decoding ATTID based on drsuapi_prefixMap leads
> to an OID that actually exists.

Doesn't loading the schema already check that?  (If not, should we not
do the checks there?).

> So please, don't remove it.
> The cache i've implemented lowers execution time significantly.

And this cache is simply a duplicate of the schema we already load.  If
we must keep this code, then at least let us use the schema, which does
a binary, rather than a linear, search of the possible names.

But I still can't see how it validates anything extra.  

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.

[Kamen Mazdrashki]

On Wed, Nov 11, 2009 at 22:53, Andrew Bartlett <abartlet@...> wrote:
> On Wed, 2009-11-11 at 15:04 +0200, Kamen Mazdrashki wrote:
>> Main purpose of _drs_util_verify_attids() is to verify decoded OID
>> is real one - i.e. decoding ATTID based on drsuapi_prefixMap leads
>> to an OID that actually exists.
>
> Doesn't loading the schema already check that?  (If not, should we not
> do the checks there?).
>
No. _drs_util_verify_attids() is to verify ATTIDs got from GetNCChanges
are properly decoded using the prefixMap we got from GetNCChanges.
Which helped me to find out w2k8-r2 does not make same ATTIDs as
described in MD-DRSR.pdf.

>> So please, don't remove it.
>> The cache i've implemented lowers execution time significantly.
>
> And this cache is simply a duplicate of the schema we already load.  If
> we must keep this code, then at least let us use the schema, which does
> a binary, rather than a linear, search of the possible names.
>
Yes, you are right.
By the time I was implementing this tiny cache I wasn't aware you are
implementing schema loading - hence we collide :).

> But I still can't see how it validates anything extra.
>
As I noted above - it verifies ATTIDs received against the drsuapi_prefixMap
we receive.
The whole ideia was that DSSYNC test to use alternative implementation
for "drsuapi_prefixMap  + ATTID => OID" encoding/decoding so that we
can catch regressions in dsdb implementation.

BR,
Kamen Mazdrashki
kamen.mazdrashki@...
http://repo.or.cz/w/Samba/kamenim.git
-------------------------------------
CISCO SYSTEMS BULGARIA EOOD
http://www.cisco.com/global/BG/

[Andrew Bartlett]

On Wed, 2009-11-11 at 23:16 +0200, Kamen Mazdrashki wrote:

> On Wed, Nov 11, 2009 at 22:53, Andrew Bartlett <abartlet@...> wrote:
> > On Wed, 2009-11-11 at 15:04 +0200, Kamen Mazdrashki wrote:
> >> Main purpose of _drs_util_verify_attids() is to verify decoded OID
> >> is real one - i.e. decoding ATTID based on drsuapi_prefixMap leads
> >> to an OID that actually exists.
> >
> > Doesn't loading the schema already check that?  (If not, should we not
> > do the checks there?).
> >
> No. _drs_util_verify_attids() is to verify ATTIDs got from GetNCChanges
> are properly decoded using the prefixMap we got from GetNCChanges.
> Which helped me to find out w2k8-r2 does not make same ATTIDs as
> described in MD-DRSR.pdf.
>
> >> So please, don't remove it.
> >> The cache i've implemented lowers execution time significantly.
> >
> > And this cache is simply a duplicate of the schema we already load.  If
> > we must keep this code, then at least let us use the schema, which does
> > a binary, rather than a linear, search of the possible names.
> >
> Yes, you are right.
> By the time I was implementing this tiny cache I wasn't aware you are
> implementing schema loading - hence we collide :).
>
> > But I still can't see how it validates anything extra.
> >
> As I noted above - it verifies ATTIDs received against the drsuapi_prefixMap
> we receive.
> The whole ideia was that DSSYNC test to use alternative implementation
> for "drsuapi_prefixMap  + ATTID => OID" encoding/decoding so that we
> can catch regressions in dsdb implementation.

If you want to do that, then write up a test that uses expected values
for the 'fixed' part of the table.  That will give us a real test that
might actually fail on errors.  In the meantime, I do propose to remove
this, as I can't see what extra value it gives.

That is: by doing a OID -> attr ID load during the schema load, and a
attrid -> attribute lookup during the syntax conversion, and a
comparison on the converted values (by looking up LDAP with a string
attribute name), I think we have a high degree of confidence that we
have the right mapping.

Can you have a look over my dsdb-dn branch and see if you agree?  

Thanks,

Andrew Bartlett

--
Andrew Bartlett                                http://samba.org/~abartlet/
Authentication Developer, Samba Team           http://samba.org
Samba Developer, Cisco Inc.