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?