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

[Matthieu Patou]

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 ?
> 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.

Matthieu.