[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