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

Jakub Hrozek jhrozek at redhat.com
Tue Jan 8 13:11:47 UTC 2013


On Tue, Jan 08, 2013 at 09:29:46AM +0100, Sumit Bose wrote:
> On Mon, Jan 07, 2013 at 01:22:26PM +0100, Jakub Hrozek wrote:
> > 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.
> 
> Simo, Jakub, thank you very much for the review. The new set of patches
> address all your latest comments.
> 
> bye,
> Sumit

Both mine and Simo's comments were addressed, the code looks good to me
and basic sanity testing went fine.

Ack



More information about the sssd-devel mailing list