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).
Yes, that one definitely takes precedence, sure. I'm just trying to
clean up the list :)