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