[SSSD] [PATCHES] Improve group handling in the PAC responder

Simo Sorce simo at redhat.com
Fri Jan 4 22:15:22 UTC 2013


On Fri, 2013-01-04 at 17:15 +0100, Sumit Bose wrote:
> On Fri, Jan 04, 2013 at 04:31:03PM +0100, Jakub Hrozek wrote:
> > On Fri, Dec 21, 2012 at 12:31:32PM +0100, Sumit Bose wrote:
> > > On Wed, Dec 19, 2012 at 08:10:02AM -0500, Simo Sorce wrote:
> > > > On Tue, 2012-12-18 at 21:43 +0100, Sumit Bose wrote:
> > > > > 
> > > > > I started to change it but didn't like the resulting code, because it
> > > > > turned out to be complex and hard to follow. Since the original struct
> > > > > only user 8 or 12 bytes per group (depending if your are on 32bits or
> > > > > on
> > > > > 64bits) I would prefer to keep the old struct for this round of
> > > > > patches.
> > > > > 
> > > > Sorry can't agree, space is only one of the factors, although completely
> > > > unnecessarily using 2x or 3x number of groups space is ugly enough,
> > > > But the main issues are really in the code this structure
> > > > allows/produces.
> > > > 
> > > > Still a nack for me.
> > > 
> > > The attached patch should be applied on top of the other patches and
> > > changes the struct as you suggested in your first comment.
> > > 
> > > bye,
> > > Sumit
> > 
> > Why not squash the patch into the patch that introduces the structure?
> 
> The structure was introduced in the first patch and then used in some of
> the other patches. I didn't squash the changes into the related patches
> to not invalidate the reviews of the others and to allow a better
> comparison between the old and the new structure.


Ok I reviewd all patches except the last, which actually makes it harder
as I would have to wrap my head again around 3/4 patches I already
reviewed, so it would be really nice if it were folded in.
I'll let Jakub decide what to do, maybe he will review the patch on ti's
own and just ack it :)

I have one comment though, in patch 6 you use the term 'local domain'
and later on you also renamed a variable from domain_sid to
loc_domain_sid. I think this is confusing because we have the LOCAL
domain in sssd and we never used the term 'local domain' to describe a
generic domain. I think you should not call it local at all. The
'domain' is just fine, the subdomains are already differentiated by
calling them 'sub'.

I would like to see 0006 respun to remove the 'local' reference from the
commit, comments and the variable.

Also the first commit line of patch 0007 has 2 typos group -> groups,
attributre -> attribute.

Beyond this I think it's an ACK for the first 12 patches, provided
someone either review the 13th or fold the 13th back into the first.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York




More information about the sssd-devel mailing list