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