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

Jakub Hrozek jhrozek at redhat.com
Wed Mar 20 13:17:21 UTC 2013


On Wed, Mar 20, 2013 at 02:08:29PM +0100, Michal Židek wrote:
> On 03/20/2013 01:53 PM, Jakub Hrozek wrote:
> >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?
> 
> I think it is better to do the https://fedorahosted.org
> /sssd/ticket/1772 first, and then rebase this patch with the
> new macros used.

Good idea.

> I'll do it after I fix ticket #1741
> (sss_cache doesn't support subdomains).

Yes, that one definitely takes precedence, sure. I'm just trying to
clean up the list :)



More information about the sssd-devel mailing list