svn commit: samba r17372 - in branches/SOC/mkhl/ldb-map/modules: .

[Martin Kühl]

On 8/2/06, abartlet at samba.org <abartlet at samba.org> wrote:

I am fine with most of this changeset, however...

> --- branches/SOC/mkhl/ldb-map/modules/ldb_map.c 2006-08-02 01:25:05 UTC (rev 17371)
> +++ branches/SOC/mkhl/ldb-map/modules/ldb_map.c 2006-08-02 02:24:00 UTC (rev 17372)
[...]
> @@ -2046,6 +2072,7 @@
>                                 ldb_msg_add(msg, el, 0);
>                 }
>
> +               msg->dn = talloc_steal(msg, ares->message->dn);
>                 talloc_free(ares->message);
>                 ares->message = msg;
>         }

This change caused a strange regression in samba3sam: Add requests
started to fail because the second search in `samldb_get_new_sid'
started yielding bogus results.  It turned out that the name of the
"objectSid" element of the result got corrupted (and read "objectSi@"
instead).  This happened regardless of whether the former message was
freed or not (`ares->message' from the above snippet), but stopped
when I stopped stealing the DN...

I remembered the `ldb_msg_remove_attr' function though and am about to
replace this block with it.

> @@ -2242,9 +2269,8 @@
>
>         /* XXX: ugly kludge
>         ac->local_attrs = local_attrs;
> +       */
>         ac->remote_req->op.search.attrs = remote_attrs;
> -       */
> -       ac->remote_req->op.search.attrs = NULL;
>
>         /* split local from remote tree */
>         ret = partition_tree(module, ac->remote_req, ac,

When checking a search result against the original parse-tree, I
wanted to have all attributes of the record available, which is why I
set the searched attributes to NULL here.
Can you tell me the reason you wanted this change?

> @@ -2672,6 +2698,13 @@
>         ac->remote_req->op.del.dn = map_local_dn(module, ac->remote_req,
>                                                  req->op.del.dn);
>
> +       /* The DN didn't change, so just pretend we were never here */
> +       if (ldb_dn_compare(module->ldb, ac->remote_req->op.del.dn,
> +                          req->op.del.dn) == 0) {
> +               talloc_free(h);
> +               return ldb_next_request(module, req);
> +       }
> +
>         ac->remote_req->context = NULL;
>         ac->remote_req->callback = NULL;
>

This change causes another regression samba3sam, where it is common
that the local and remote DN are equal until the remote one is
rebased.  I think we should test equality of the base DNs instead to
determine whether to run the local requests at all.

> @@ -2774,10 +2807,11 @@
>         if (ldb_dn_is_special(req->op.rename.olddn))
>                 return ldb_next_request(module, req);
>
> -       /* no mapping requested, skip to next module */
> -       if (!check_dn_local(module, req->op.rename.olddn) &&
> -           !check_dn_local(module, req->op.rename.newdn))
> +       /* no mapping requested, (perhaps no DN mapping specified), skip to next module */
> +       if ((!check_dn_local(module, req->op.rename.olddn) &&
> +            !check_dn_local(module, req->op.rename.newdn))) {
>                 return ldb_next_request(module, req);
> +       }
>
>         /* rename into/out of the mapped partition requested, bail out */
>         if (!check_dn_local(module, req->op.rename.olddn) ||

Um, you just wrapped the whole expression the `if' tests in another
pair of parens.  Did you perhaps mean to wrap the subexpressions
instead? :-)

Cheers,
Martin