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

[Andrew Bartlett]

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.

The purpose of res->msgs[0] is to give the current DB as a base to the
diff function. 

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/796fc56c/attachment.pgp>