Chris Darroch updated ZOOKEEPER-311:

    Attachment: ZOOKEEPER-311.patch

This revision is a good demonstration of why one should never trust a 
programmer, especially if there's a degree of hubris involved ("It's just a 
small patch, what could go wrong?")  In testing, I found I had an off-by-one 
error when calculating where to put the trailing null.  Sigh.

I agree this shouldn't be committed without some tests.  I'm at virtually 100% 
capacity at the moment with just my paid work so it may take me a while to do 
this; it is my next to-do, though.  If someone else wants to write a few tests 
in the meantime, that would also be great.

> handle small path lengths in zoo_create()
> -----------------------------------------
>                 Key: ZOOKEEPER-311
>                 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-311
>             Project: Zookeeper
>          Issue Type: Improvement
>          Components: c client
>    Affects Versions: 3.0.0, 3.0.1, 3.1.0
>            Reporter: Chris Darroch
>            Assignee: Chris Darroch
>            Priority: Minor
>             Fix For: 4.0.0
>         Attachments: ZOOKEEPER-311.patch, ZOOKEEPER-311.patch, 
> ZOOKEEPER-311.patch
> The synchronous completion for zoo_create() contains the following code:\\
> {noformat}
> if (sc->u.str.str_len > strlen(res.path)) {
>     len = strlen(res.path);
> } else {
>     len = sc->u.str.str_len-1;
> }
> if (len > 0) {
>     memcpy(sc->u.str.str, res.path, len);
>     sc->u.str.str[len] = '\0';
> }
> {noformat}
> In the case where the max_realpath_len argument to zoo_create() is 0, none of 
> this code executes, which is OK.  In the case where max_realpath_len is 1, a 
> user might expect their buffer to be filled with a null terminator, but 
> again, nothing will happen (even if strlen(res.path) is 0, which is unlikely 
> since new node's will have paths longer than "/").
> The name of the argument to zoo_create() is also a little misleading, as is 
> its description ("the maximum length of real path you would want") in 
> zookeeper.h, and the example usage in the Programmer's Guide:
> {noformat}
> int rc = zoo_create(zh,"/xyz","value", 5, &CREATE_ONLY, ZOO_EPHEMERAL, 
> buffer, sizeof(buffer)-1);
> {noformat}
> In fact this value should be the actual length of the buffer, including space 
> for the null terminator.  If the user supplies a max_realpath_len of 10 and a 
> buffer of 11 bytes, and strlen(res.path) is 10, the code will truncate the 
> returned value to 9 bytes and put the null terminator in the second-last 
> byte, leaving the final byte of the buffer unused.
> It would be better, I think, to rename the realpath and max_realpath_len 
> arguments to something like path_buffer and path_buffer_len, akin to 
> zoo_set().  The path_buffer_len would be treated as the full length of the 
> buffer (as the code does now, in fact, but the docs suggest otherwise).
> The code in the synchronous completion could then be changed as per the 
> attached patch.
> Since this would change, slightly, the behaviour or "contract" of the API, I 
> would be inclined to suggest waiting until 4.0.0 to implement this change.

This message is automatically generated by JIRA.
You can reply to this email to add a comment to the issue online.

Reply via email to