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

Jakub Hrozek jhrozek at redhat.com
Mon Jan 7 12:22:26 UTC 2013


On Mon, Jan 07, 2013 at 10:30:41AM +0100, Jakub Hrozek wrote:
> On Fri, Jan 04, 2013 at 05:15:22PM -0500, Simo Sorce wrote:
> > 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 :)
> 
> Sumit explained it would be difficult to squash the patch into the
> previous set. In general, it would be nicer to have only a single set,
> but given our current constraints, I'm fine as long as the resulting
> code works and looks sane.
> 
> I'll review the patch separately.

The last patch looks OK to me, I only asked Sumit to add some comments
to place that wasn't so obvious in get_gids_from_pac().

I haven't read the previous patches too carefully, but I trust Simo's review.



More information about the sssd-devel mailing list