important: Wrong logic (to my mind) in schema_set.c
Nadezhda Ivanova
nadezhda.ivanova at postpath.com
Mon Nov 30 02:00:05 MST 2009
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