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

Jakub Hrozek jhrozek at redhat.com
Wed Mar 20 12:53:44 UTC 2013


On Thu, Oct 11, 2012 at 11:07:55PM +0200, Jakub Hrozek wrote:
> 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.

I'm sorry this patch got forgotten. Can you rebase it to the current
master?



More information about the sssd-devel mailing list