[ 
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:all-tabpanel
 ]

Chris Darroch updated ZOOKEEPER-311:
------------------------------------

    Description: 
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.

  was:
The synchronous completion for zoo_create() contains the following code:

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';
}

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:

int rc = zoo_create(zh,"/xyz","value", 5, &CREATE_ONLY, ZOO_EPHEMERAL, buffer, 
sizeof(buffer)-1);

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.


> 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
>            Priority: Minor
>             Fix For: 4.0.0
>
>         Attachments: 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