important: Wrong logic (to my mind) in schema_set.c
Matthieu Patou
mat+Informatique.Samba at matws.net
Mon Nov 30 07:44:40 MST 2009
Hello,
I've been trying multiple solution but none for the moment, there is no
easy fix removing and readding all the @ATTRIBUTES and @INDEX is not
always working as it sometimes consume the current transaction ...,
using samdb_replace is not ok also because it tries to replace
nonexisting attributes, ldb_modify can also fail because some partition
have the object and some other not ...
I am releasing 6 patches for upgradeprovision:
The for ones should be shipped in alpha9, it's mostly the same as yesterday
0001-s4-load-the-domain-level-of-the-current-provision-an.patch
0002-s4-don-t-forget-to-update-defaultSecurityDescriptor.patch
0003-s4-Handle-the-case-in-secrets.ldb-without-name-attri.patch
0004-s4-Remove-targetdir-as-it-can-cause-some-trouble-and.patch
But it takes in account some remarks of andrew B. regarding the domain
level.
The last two is what I made to allow upgrading an alpha3 provision. As
you can see I just ignore the result of ldb_modify which is just bad but
I didn't find a proper way to do it (it seems that we will have to dig
into nested transaction to solve it).
It's just for the information as I think I will use this patches for
upgrading my alpha3 provision.
It seems that we agree that we have to rework quite badly this
bootstrapping so let's do it after alpha9.
My point of view In order to avoid situation like this we should really
insist that users of S4 use upgradeprovision at each alpha (and soon
beta) do an upgradeprovision to keep with the change so that there is no
heavy transformation like it's the case here.
Matthieu.
On 30/11/2009 12:00, Nadezhda Ivanova wrote:
> Hi guys,
> If I am not mistaken we introduced this logic because of schemaUpdateNow. The problem is that the kludge_acl (and the acl module will eventually) filtered out some attributes fromn the search request. They are interpreted as not included in the schema, so we attempt to re-add them, and we got "attribute already exists error" every time. Not sure if we still need that, but it was impossible to change the schema without this at the time.
>
> Regards,
> Nadya
> ----- Original Message -----
>
>> From: samba-technical-bounces at lists.samba.org<samba-technical-bounces at lists.samba.org>
>> To: Andrew Bartlett<abartlet at samba.org>, Matthieu Patou<mat+Informatique.Samba at matws.net>
>> Cc: samba-technical<samba-technical at lists.samba.org>
>> Sent: Monday, November 30, 2009 10:37:42 AM GMT+0200 Europe;Athens
>> Subject: Re: important: Wrong logic (to my mind) in schema_set.c
>>
>
>>> On 30/11/2009 02:57, Andrew Bartlett wrote:
>>> 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,
>>>
>>>
>>>
>> So by apply this patch
>> diff --git a/source4/dsdb/schema/schema_set.c
>> b/source4/dsdb/schema/schema_set.c
>> index a984c03..46cbede 100644
>> --- a/source4/dsdb/schema/schema_set.c
>> +++ b/source4/dsdb/schema/schema_set.c
>> @@ -137,3 +137,9 @@ 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) {
>> + int r;
>> +
>> for(r=0;r<mod_msg->num_elements;r++) {
>> + fprintf(stdout,"att: %s
>>
>> flag %d\n",mod_msg->elements[r].name,mod_msg->elements[r].flags);
>> + }
>> + ldb_modify(ldb,mod_msg);
>> +
>>
>>
>> I produced the log attached to this email. The lines starting with att
>>
>> are produced by the upper patch. It's clear that ldb_msg_diff wants
>> stuff to be added and removed to the database (and quite to change
>> them).
>>
>> Using the ldb_modify instead of samdb_replace will allow to avoid the
>>
>> _ldb.LdbError: (80, 'schema_load_init: dsdb_set_schema() failed: 32:No
>>
>> such object')
>>
>>
>> But still remain the following messages that are emitted in the
>> function
>> guess_names_from_provision when trying to do
>> samdb = Ldb(paths.samdb, session_info=session_info,
>> credentials=credentials, lp=lp,
>> options=["modules:samba_dsdb"])
>>
>>
>> ltdb: tdb(/home/mat/test/private/users.ldb): tdb_transaction_commit:
>> no
>> transaction
>> A transaction is still active in ldb context [0x8ea8190] on
>> /home/mat/test/private/sam.ldb
>>
>> Also please note that in function check_diff_name when called with
>> isschema to 0
>>
>> we just open the samdb normaly but under the hood there is one more
>> round of modifications in the @ATTRIBUTES and @INDEXES (because just
>> before the schema was modified to add the missing classes and
>> attributes) and when the script start to insert the first missing
>> object
>> I get the following output:
>>
>>
>> Adding object
>> CN=401,CN=DisplaySpecifiers,CN=Configuration,DC=home,DC=matws,DC=net
>>
>> Traceback (most recent call last):
>> File "scripting/bin/upgradeprovision", line 770, in<module>
>> update_samdb(newpaths,paths,creds,session,names)
>> File "scripting/bin/upgradeprovision", line 711, in update_samdb
>> hashSD =
>> check_diff_name(newpaths,paths,creds,session,str(names.rootdn),names,
>> 0)
>> File "scripting/bin/upgradeprovision", line 554, in check_diff_name
>> sam_ldb.add(delta,["relax:0"])
>> _ldb.LdbError: (1, 'ltdb modify without transaction')
>>
>>
>> I worked around this problem by reopening the database before adding
>> missing objects but clearly it shouldn't be needed (to my mind).
>>
>> Matthieu.
>>
More information about the samba-technical
mailing list