[PATCH 1/4] Enable domain groups to be added to builtin groups at domain join time

[Tim Prouty]

[Volker Lendecke]

Hi, Tim!

Two comments on this one: First, these functions would
probably better belong to the passdb/ subdirectory instead
of auth/. Second: What's the point of add_sid_to_builtin()?
Isn't this just a wrapper around pdb_add_aliasmem()? I could
see the justification if you passed just the rid of the
alias that you want to add stuff to, but the way the
function is written seems a bit redundant.

Volker

[Tim Prouty]

Hi Volker!

Thanks for the feedback!

On Jul 30, 2008, at 6:24 AM, Volker Lendecke wrote:

> Two comments on this one: First, these functions would
> probably better belong to the passdb/ subdirectory instead
> of auth/.

I made them static since their only callers are inside of this file,  
but if you think they might be useful to other parts of samba, they  
could be made non-static and moved to a better home. Do you have any  
preference where in passdb they should go?  Maybe passdb/util_builtin.c?

> Second: What's the point of add_sid_to_builtin()?
> Isn't this just a wrapper around pdb_add_aliasmem()? I could
> see the justification if you passed just the rid of the
> alias that you want to add stuff to, but the way the
> function is written seems a bit redundant.

The main point is to avoid adding duplicate NT_STATUS_MEMBER_IN_ALIAS  
checks after each pdb_add_aliasmem.  Keeping this logic in a common  
function also allows the debug messages to stay in one place.  If this  
sounds OK to you, I actually have a fifth patch that removes the now  
redundant logging from create_buitlin_users and  
create_builtin_administrators.

-Tim

[Jeremy Allison]

On Wed, Jul 30, 2008 at 09:32:24AM -0700, Tim Prouty wrote:

> Hi Volker!
>
> Thanks for the feedback!
>
> On Jul 30, 2008, at 6:24 AM, Volker Lendecke wrote:
>
> >Two comments on this one: First, these functions would
> >probably better belong to the passdb/ subdirectory instead
> >of auth/.
>
> I made them static since their only callers are inside of this file,  
> but if you think they might be useful to other parts of samba, they  
> could be made non-static and moved to a better home. Do you have any  
> preference where in passdb they should go?  Maybe passdb/util_builtin.c?
>
> >Second: What's the point of add_sid_to_builtin()?
> >Isn't this just a wrapper around pdb_add_aliasmem()? I could
> >see the justification if you passed just the rid of the
> >alias that you want to add stuff to, but the way the
> >function is written seems a bit redundant.
>
> The main point is to avoid adding duplicate NT_STATUS_MEMBER_IN_ALIAS  
> checks after each pdb_add_aliasmem.  Keeping this logic in a common  
> function also allows the debug messages to stay in one place.  If this  
> sounds OK to you, I actually have a fifth patch that removes the now  
> redundant logging from create_buitlin_users and  
> create_builtin_administrators.

Ok, after discussions with Volker I've pushed them as-is into
the 3.3. tree.

If you want to send along the fifth patch that would
be welcome too !

Thanks,

Jeremy.

[Tim Prouty]

On Jul 30, 2008, at 2:07 PM, Jeremy Allison wrote:

> Ok, after discussions with Volker I've pushed them as-is into
> the 3.3. tree.

Thanks!

> If you want to send along the fifth patch that would
> be welcome too !


Here it is :).

[Jeremy Allison]

On Wed, Jul 30, 2008 at 02:56:32PM -0700, Tim Prouty wrote:

> On Jul 30, 2008, at 2:07 PM, Jeremy Allison wrote:
>
> >Ok, after discussions with Volker I've pushed them as-is into
> >the 3.3. tree.
>
> Thanks!
>
> >If you want to send along the fifth patch that would
> >be welcome too !
>
>
> Here it is :).

Pushed - thanks !

Jeremy.