Hi!
Looking at the code -- In idmap_tdb.c:337 you jump to line
400, where you call tdb_chainunlock. At this point
tdb_chainlock has not yet been called, so the chainunlock
would potentially remove a lock that had been set elsewhere.
Is that correct?
Volker
PGP SIGNATURE
Version: GnuPG v1.4.2 (GNU/Linux)
/KIxvNSF1J5eGUrfdxkHHnU=
=o217
PGP SIGNATURE

Tue, Jul 04, 2006 at 11:05:07PM +0200, Volker Lendecke wrote:
Hi!

Looking at the code -- In idmap_tdb.c:337 you jump to line
400, where you call tdb_chainunlock. At this point
tdb_chainlock has not yet been called, so the chainunlock
would potentially remove a lock that had been set elsewhere.
Is that correct?

I (finally) took a look at this code (the idmap). It's *horrible*.

The interface design needs severely cleaning up.

We shouldn't be overloading one int paramter as a type (ID_XXX)
as well as a query specifier (ID_QUERYNLY|ID_CACHENLY).

This *must* be 2 separate parameters in order to begin to make
sense - and both type must be enum types to catch errors here.

This code currently is a disaster waiting to happen. I'm going
to clean it up.

Jeremy.

Tue, Jul 04, 2006 at 04:36:01PM -0700, Jeremy Allison wrote:
I (finally) took a look at this code (the idmap). It's *horrible*.

This code currently is a disaster waiting to happen. I'm going
to clean it up.

More insanity. IDTHER is #defined but never used anywhere.
What is ID_EMPTY for ? Where is it used ? Tell me why we are
mapping to anything other than user or group ?

ID_QUERYNLY|ID_CACHENLY seem to be flags controlling
backend actions - can they not be taken care of by the
backends themselves ?

This code needs completely rewriting. That's probably
too much to do on the 4th of July but I'm taking a
chainsaw to this mess.

Jeremy.

Tue, Jul 04, 2006 at 05:02:02PM -0700, Jeremy Allison wrote:

This code needs completely rewriting. That's probably
too much to do on the 4th of July but I'm taking a
chainsaw to this mess.

FYI: I'm nearly finished with the rewrite (the fireworks
don't start until 9:30pm :-). Simo or Volker, please don't
check in any changes until you see my commit.

Jeremy

Tue, Jul 04, 2006 at 07:26:23PM -0700, Jeremy Allison wrote:
Tue, Jul 04, 2006 at 05:02:02PM -0700, Jeremy Allison wrote:

This code needs completely rewriting. That's probably
too much to do on the 4th of July but I'm taking a
chainsaw to this mess.

FYI: I'm nearly finished with the rewrite (the fireworks
don't start until 9:30pm :-). Simo or Volker, please don't
check in any changes until you see my commit.

I checked in the rewrite. It might not work but it's
better than what we had.

Lets move forward with this

Jeremy.

Wed, Jul 05, 2006 at 12:13:19AM -0400, simo wrote:

well you couldn't really wait just a day?

No. That code was too ugly to live

I had some uncommitted work on this, now I will have to redo and retest
everything from scratch :-(

Sorry, but you shouldn't be adding new code to this mess.

We need to fix the mess first, then you can add new code.

Jeremy.

PGP SIGNED MESSAGE
Hash: SHA1

Jeremy,

I checked in the rewrite. It might not work but it's
better than what we had.

If it doesn't work, it's worse than what we had. I wish
you wouldn't rewrite stuff without testing it. When any of
us do this, it only encourages others (inside and outside
the team) to do the same. I trust you to finish
it up but I also have worked with you enough to be scared
by comments like this one. The fact that gd had to fix
the builds of idmap_rid and idmap_ad is really a bad sign.

I'm not running trunk until I know you have finished
testing your code and I will vote against cutting trunk
over to SAMBA_3_0 until that point.

Sorry to be rude, but I'm a little more than annoyed
at your response on the list. You never do anything
with winbindd and then decide to rewrite the idmap
interface in one day without talking to anyone. I told
Volker I would review Simo's work after the 4th. Everything
was under control.

cheers, jerry
PGP SIGNATURE
Version: GnuPG v1.4.2 (GNU/Linux)
Comment: Using GnuPG with SUSE - http://enigmail.mozdev.org

5lAvXKtknMgs1YgbRLFNDxY=
=AK79
PGP SIGNATURE

Wed, Jul 05, 2006 at 08:17:35AM -0500, Gerald (Jerry) Carter wrote:

If it doesn't work, it's worse than what we had. I wish
you wouldn't rewrite stuff without testing it. When any of
us do this, it only encourages others (inside and outside
the team) to do the same. I trust you to finish
it up but I also have worked with you enough to be scared
by comments like this one.

It is better than what was there. Trust me on that :-).
The code that was there was confused, badly designed
and had several unknown dependencies on in/out parameters
that were not understood by the upper layers. That is
explicit now.

I checked into HEAD specifically because I needed to
fix this without disturbing 3.x. This needs to be possible,
otherwise it's not possible to make the architectural
changes needed to fix major problems. This is something
Volker wanted to do but had not yet gotten to. I helped
out, I made the time (on my holiday day too :-).

The fact that gd had to fix
the builds of idmap_rid and idmap_ad is really a bad sign.

I assumed they were being build by default - if they
were I would have caught those errors in the rewrite. That
wasn't a big deal.

I'm not running trunk until I know you have finished
testing your code and I will vote against cutting trunk
over to SAMBA_3_0 until that point.

I agree. I want to fix this before moving it over.

Sorry to be rude, but I'm a little more than annoyed
at your response on the list. You never do anything
with winbindd and then decide to rewrite the idmap
interface in one day without talking to anyone. I told
Volker I would review Simo's work after the 4th. Everything
was under control.

I do lots of things with winbindd, and work really
hard to make sure I have at least an architectural overview of
everything that's in Samba - this is my job.

Jeremy.