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

Michal Židek mzidek at redhat.com
Wed Mar 20 13:08:29 UTC 2013


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. I'll do it after I fix ticket #1741
(sss_cache doesn't support subdomains).

Michal



More information about the sssd-devel mailing list