important: Wrong logic (to my mind) in schema_set.c
Andrew Bartlett
abartlet at samba.org
Sun Nov 29 14:30:29 MST 2009
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>
More information about the samba-technical
mailing list