Michael,
Are you using valgrind? I'm curious what motivated this commit?
-sam
On Sep 4, 2009, at 9:27 AM, Michael Moore wrote:
Michael Moore wrote:
Pete Wyckoff wrote:
[email protected] wrote on Fri, 04 Sep 2009 09:29 -0400:
Pete Wyckoff wrote:
[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)
There's the mixup, we're looking at different versions of
encode_string!
From a fresh 2.8.1 and the branch I'm working from I have the
following
encode_string:
#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); \
int pad = roundup8(4 + len + 1) - (4 + len + 1); \
*(pptr) += roundup8(4 + len + 1); \
memset(*(pptr)-pad, 0, pad); \
} else { \
*(u_int32_t *) *(pptr) = 0; \
*(pptr) += 8; \
} \
In the else branch there is no +4 in the *pptr assignment. Which
branch
are you working from? It looks like this was already addressed
somewhere
before?
I go to pvfs.org/fisheye. Looking at cvs vers 1.27 now. I was
quoting the non-valgrind one. Which seems to be different from the
valgrind one you quote above in this important way. My vote goes
toward fixing the valgrind version to fix the off-by-4 bug you just
found. It's old, apparently, but would only hurt if you configured
--with-valgrind.
Good catch. Looking at that mess of #defines is always a challenge.
-- Pete
The above snippet is the non-Valgrind version but both versions
have the
issue in pre-1.27. It looks like the non-valgrind version just got
fixed
by dbonnie in the 1.26->1.27 patch on Wednesday.
Scratch that, it was a patch back in July by nlmills to specifically
address this issue.
Michael
Thanks for your help!
Michael
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
Apologies for the lack of context on the diff but you're right it
was on
the decode side. However, when you mentioned the encode side always
shipped 8 bytes I looked closer at the encode side and saw in this
version only 4 of the 8 bytes were getting set to '\0'.
Michael
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
_______________________________________________
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
