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.
--
Simo Sorce * Red Hat, Inc * New York
_______________________________________________
sssd-devel mailing list
sssd-devel@lists.fedorahosted.org
https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
