[email protected] wrote on Thu, 03 Sep 2009 16:31 -0400:
> But Pete, the problem is encode_string does not write 8 bytes of 0, it
> writes 4 bytes of 0. If the buffer is 0's to start with what is here
> works fine, but if there is something else in the field you get garbage.
>
> Seems like a prudent move to simply zero out all 8 bytes instead of just
> 4, wouldn't you say?
Thanks for explaining. I'm looking at the wrong end of the problem,
maybe.
On the encode side, we have
#define encode_string(pptr,pbuf) do { \
u_int32_t len = 0; \
if (*pbuf) \
len = strlen(*pbuf); \
*(u_int32_t *) *(pptr) = htobmi32(len); \
if (len) { \
memcpy(*(pptr)+4, *pbuf, len+1); \
*(pptr) += roundup8(4 + len + 1); \
} else { \
*(u_int32_t *) (*(pptr)+4) = 0; \
*(pptr) += 8; \
} \
} while (0)
For a NULL pbuf, or a non-NULL pbuf that starts with'\0',
we get:
len = 0
pptr[0..3] = 0
(take the else clause)
pptr[4..7] = 0
(pptr offset up by 8)
That generates 8 bytes okay.
For any other pbuf with len > 0, say 1 for "a\0", we get:
len = 1
pptr[0..3] = htonl(1)
(take the if clause)
pptr[4..7] = 'a' '\0' 'x' 'x' /* 'x' == garbage */
(pptr offset up by 8)
Is it those two bytes of 'x' that you don't like? That's why
we have the valgrind ifdef. What am I missing?
Michael's initial patch was:
131c131,134
< memcpy(pbuf, *(pptr) + 4, len + 1); \
---
> > if( len ) \
> > memcpy(pbuf, *(pptr) + 4, len + 1); \
> > else \
> > memcpy(pbuf, *(pptr), len + 1); \
Unfortunately without the useful "-u -p" args to diff, but eyeing
the source shows he's patching on the decode side.
-- Pete
> Pete Wyckoff wrote:
>> [email protected] wrote on Thu, 03 Sep 2009 13:44 -0400:
>>> Pete Wyckoff wrote:
>>>> [email protected] wrote on Thu, 03 Sep 2009 11:51 -0400:
>>>>> In looking at some issues I was having with the encoding of PVFS_dirent
>>>>> structs in requests I saw an inconsistency in how here_strings are
>>>>> encoded and decoded. encode_string memcpys strings starting at *(pptr)+4
>>>>> unless it's length 0 in which case it sets *(pptr) to 0. However,
>>>>> decode_here_string always copys from *(pptr) + 4. So, if d_name is an
>>>>> empty string when encoded d_name gets 1 byte of *(pptr)+4 instead of 0
>>>>> on decoding.
>>>>>
>>>>> The fix is just to handle decoding like encoding. Is there a reason for
>>>>> always copying to *(pptr)+4 in decode_here_string? Is this something
>>>>> that should be changed?
>>>> encode_string always ships at least 8 bytes. For a null string, that's 8
>>>> bytes of zeroes. Decoding a null "here" string will use one of
>>>> those zero bytes to set pbuf[0] = '\0'. I figured it would be nice
>>>> to make sure the string was set to NULL.
>>>>
>>> I see where 8 bytes are always shipped in encode_string but I'm not
>>> seeing where 8 bytes of '\0' get encoded for a NULL string or if the
>>> length of the string is 0.
>>>
>>> However, adding an 8 byte memset worth of '\0' to *(pptr) in the else
>>> (length 0) branch of encode_string also resolves the problem I was
>>> seeing. So, your point may make for a better solution.
>>
>> uint8_t *wiredata;
>> char mystr[20];
>> decode_here_string(&wiredata, mystr);
>>
>> wiredata -> 0 0 0 0 0 0 0 0
>> mystr unititialized
>>
>> 131 memcpy(pbuf, *(pptr) + 4, len + 1); \
>>
>> memcpy(pbuf, <bunch of zeroes>, 1)
>> equiv to
>>
>> pbuf[0] = '\0';
>> or
>> mystr[0] = '\0';
>>
>> right? It's a _here_ string, you don't want to destroy the pointer,
>> you want to put a zero into it to nullify the string.
>>
>> The comment does say "odd variation". I don't remember where this
>> even gets used.
>>
>> -- Pete
>> _______________________________________________
>> Pvfs2-developers mailing list
>> [email protected]
>> http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
>
_______________________________________________
Pvfs2-developers mailing list
[email protected]
http://www.beowulf-underground.org/mailman/listinfo/pvfs2-developers
