[SSSD] [PATCH] Alignment issues reported by clang.

Jakub Hrozek jhrozek at redhat.com
Thu Oct 11 21:07:55 UTC 2012


On Thu, Oct 11, 2012 at 03:34:03PM -0400, Simo Sorce wrote:
> On Thu, 2012-10-11 at 20:44 +0200, Michal Židek wrote:
> > > Sorry Michael,
> > > haven't really checked as carefully as I want but I think with this
> > > patch you are changing the client protocol by changing one
> > alignemnt.
> > >
> > > You can't do that, changes to the client protocol are not allowed.
> > >
> > > Can you confirm/deny ?
> > >
> > > Simo.
> > >
> > 
> > Hello Simo,
> > 
> > do you mean this change?
> > 
> > -    /* Make sure pr->buffer[i+pad] is 32 bit aligned */
> > +    /* Make sure pr->buffer[i+pad] is aligned to sizeof (char **) */
> >       pad = 0;
> > -    while((i + pad) % 4) {
> > +    while((i + pad) % sizeof(char **)) {
> >           pad++;
> >       }
> > 
> > I have a mistake here, it should have been sizeof(char *) not 
> > sizeof(char**), because we store pointers to char on that location. I 
> > know that this will probably be the same number, but it is still a 
> > mistake (I fixed it in the new patch).
> > 
> > But to your question. I think this does not change the client
> > protocol, 
> > the function only reads the reply (the reply is in the client
> > protocol 
> > format), but stores data into another buffer. As far as I understand
> > the 
> > code, this new buffer has nothing to do with the client protocol (it
> > is 
> > connected with the reply via those mentioned char pointers, but
> > nothing 
> > more), but maybe I am wrong, it is not easy to read piece of code.
> > 
> > Or do you mean a different part of patch affects the client protocol?
> > 
> This is the change I meant, and you are right this affects the internal
> buffer returned to glibc not the actual protocol.
> 
> I guess maybe we should rename some of those variable names to make it
> clearer what buffer are we referring to.
> 
> Thanks for double checking with me, this code is sometimes tricky and
> alignment issues make it trickier.
> 
> All the other alignment fixes look good to me, but the patch is quite
> bit, I wouldn't mind if a second pair of eyes could check and give a
> second ack.
> 
> Simo.

Thanks for checking the client part Simo.

I will review the code tomorrow or Monday. I moved the ticket to a later
release because I think it touches quite a few parts of the SSSD so it's
easy to overlook something. Also the patch is mostly useful to secondary
architectures.



More information about the sssd-devel mailing list