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

[Matthieu Patou]

Hello,

I finally rested the whole thing reverting to ldb_modify is to my mind 
the good solution.

I attached a patch for this and 2 patch for updateprovision.

The whole serie allow update of quite old provision (ie. alpha3).

Matthieu

On 30/11/2009 01:07, 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 ?
>> 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.