important: Wrong logic (to my mind) in schema_set.c

[Andrew Bartlett]

On Mon, 2009-11-30 at 01:07 +0300, Matthieu Patou wrote:
> On 30/11/2009 00:30, Andrew Bartlett wrote:
> > On Sun, 2009-11-29 at 17:02 +0300, Matthieu Patou wrote:
> >    
> >> Dear all, I've been trying to upgrade a very old provision (alpha3) I've
> >> been hit by errors like this:
> >>
> >> _ldb.LdbError: (80, 'schema_load_init: dsdb_set_schema() failed: 32:No
> >> such object')
> >> After a careful investigation I am 99% sure that those two changes
> >> (introduced in changeset 6b05a907) are wrong
> >>
> >> @@ -135,7 +135,7 @@ static int dsdb_schema_set_attributes(struct
> >> ldb_context *ldb, struct dsdb_schem
> >>
> >>                   mod_msg = ldb_msg_diff(ldb, res->msgs[0], msg);
> >>                   if (mod_msg->num_elements>  0) {
> >> -                       ret = ldb_modify(ldb, mod_msg);
> >> +                       ret = samdb_replace(ldb, mem_ctx, mod_msg);
> >>                   }
> >>           }
> >>
> >> @@ -163,7 +163,7 @@ static int dsdb_schema_set_attributes(struct
> >> ldb_context *ldb, struct dsdb_schem
> >>
> >>                   mod_msg = ldb_msg_diff(ldb, res_idx->msgs[0], msg_idx);
> >>                   if (mod_msg->num_elements>  0) {
> >> -                       ret = ldb_modify(ldb, mod_msg);
> >> +                       ret = samdb_replace(ldb, mem_ctx, mod_msg);
> >>                   }
> >>
> >> Because the different elements with different flags (add,replace,delete)
> >> and using samdb_replace will override this flag by using just replace.
> >> Obviously if the ldb_diff was telling you that you should add the
> >> attribute 'foo' trying to make ldb modify the 'foo' attribute will not
> >> be very good.
> >>
> >> Also I'm quite puzzled by the use ldb_msg_diff and the order of
> >> parameters. Because res->msgs[0] is an message element obtained from a
> >> search in the LDB database represented by the ldb object.
> >> So to my mind by doing
> >> mod_msg = ldb_msg_diff(ldb, res->msgs[0], msg);
> >> we are comparing res->msgs[0] and msg and using res->msgs[0] as a
> >> reference, so there is no point trying to apply changes in the ldb
> >> object as the attributes have already the value contained in the
> >> different messageElement of the mod_msg object.
> >>
> >> So I think parameters should be swapped it makes just sens from the
> >> point of view of the comparison and the modification of the ldb but for
> >> the overall logic I'm much more reserved.
> >>      
> > I've looked over the code, and while I agree the samdb_replace seems
> > wrong, but before we added it, it just didn't work.  The goal of this
> > routine is to build new @ATTRIBUTE records and @INDEX records, and to
> > then, if they differ from the DB, replace them in a DB.
> >
> >    
> It's not very clear from what @ATTRIBUTE and @INDEX are built can you 
> explain me ?

What we do is walk over the schema, and if the schema elements tell us
that an object is case sensitive, or if it is indexed, then we add it to
those attributes. 

The idea is then that every time we load the schema, we check that this
on-disk record (used by ldb before the schema is loaded, and used to
determine indexing) is in sync with the schema. 

> > The purpose of res->msgs[0] is to give the current DB as a base to the
> > diff function.
> >
> >    
> Yeah I checked that ldb_msg_diff use the last ldbMessage as the ref, I 
> tried to revert to ldb_modify but it didn't make the trick, as it was 
> proposing  to add attribute to @ATTRIBUTE that were already in the database.
> 
> I'll try to remake trace so that we can further discuss it.

Thanks, 

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/20091130/37c83b25/attachment.pgp>