[jira] Commented: (ZOOKEEPER-311) handle small path lengths in zoo_create()
[
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12740291#action_12740291
]
Hudson commented on ZOOKEEPER-311:
--
Integrated in ZooKeeper-trunk #407 (See
[http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/407/])
. handle small path lengths in zoo_create()
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, 3.1.1, 3.2.0
Reporter: Chris Darroch
Assignee: Chris Darroch
Priority: Minor
Fix For: 3.2.1, 3.3.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.
[jira] Commented: (ZOOKEEPER-311) handle small path lengths in zoo_create()
[
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12737148#action_12737148
]
Hadoop QA commented on ZOOKEEPER-311:
-
+1 overall. Here are the results of testing the latest attachment
http://issues.apache.org/jira/secure/attachment/12413380/ZOOKEEPER-311.patch
against trunk revision 798038.
+1 @author. The patch does not contain any @author tags.
+1 tests included. The patch appears to include 3 new or modified tests.
+1 javadoc. The javadoc tool did not generate any warning messages.
+1 javac. The applied patch does not increase the total number of javac
compiler warnings.
+1 findbugs. The patch does not introduce any new Findbugs warnings.
+1 release audit. The applied patch does not increase the total number of
release audit warnings.
+1 core tests. The patch passed core unit tests.
+1 contrib tests. The patch passed contrib unit tests.
Test results:
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/testReport/
Findbugs warnings:
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html
Console output:
http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-vesta.apache.org/160/console
This message is automatically generated.
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, 3.1.1, 3.2.0
Reporter: Chris Darroch
Assignee: Chris Darroch
Priority: Minor
Fix For: 3.2.1, 3.3.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.
[jira] Commented: (ZOOKEEPER-311) handle small path lengths in zoo_create()
[
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12732654#action_12732654
]
Mahadev konar commented on ZOOKEEPER-311:
-
the qa isnt running because the nightly builds are failing. This should be
fixed soon and we can get the patch process back on track.
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, 3.1.1, 3.2.0
Reporter: Chris Darroch
Assignee: Chris Darroch
Priority: Minor
Fix For: 3.2.1
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.
[jira] Commented: (ZOOKEEPER-311) handle small path lengths in zoo_create()
[
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12711316#action_12711316
]
Benjamin Reed commented on ZOOKEEPER-311:
-
i think we should commit this patch now. the only change in behavior is when
the length of the buffer to receive the name is of length 1. it is a silly
corner case, but really the fact that we don't put a null there should be
considered a bug.
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
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.
[jira] Commented: (ZOOKEEPER-311) handle small path lengths in zoo_create()
[
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12697949#action_12697949
]
Benjamin Reed commented on ZOOKEEPER-311:
-
this is really just a documentation bug. is there a reason you are changing
logic?
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
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.
[jira] Commented: (ZOOKEEPER-311) handle small path lengths in zoo_create()
[
https://issues.apache.org/jira/browse/ZOOKEEPER-311?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12697996#action_12697996
]
Chris Darroch commented on ZOOKEEPER-311:
-
Mostly because the existing logic is a little counter-intuitive. I spent quite
a bit of time looking at it while trying to determine it's behaviour in various
cases --- e.g., what happens if my buffer is 1 byte long? Because I was
writing a wrapper library (Net::ZooKeeper) and not a client application, I
didn't know what max path len the user might supply and I wanted to ensure
consistent behaviour.
Typical C functions that take a buffer and fill it (e.g., snprintf()) rarely do
nothing if the buffer is 1 byte long; instead, they fill it with a null
terminator. So that's surprising and counterintuitive.
What I ended up doing in Net::ZooKeeper was preventing users from supplying
max path len values less than 2.
My other concern is really just about future-proofing. Suppose in a future
release ZooKeeper returns null paths from create() calls for some reason ---
because of some new feature (virtual nodes?) or whatever. Anyway, in this
case, the user might reasonably expect a 1-byte buffer to be (marginally)
useful. Or perhaps the code gets copied and re-used in another context where
1-byte buffers make more sense.
The code rewrite, although simple, I think clarifies what's going on in a way
that will help prevent buffer problems in the future --- always a sore point
when coding in C. First we calculate the len of the buffer that would be
required for the full path, including space for the null terminator. If that's
longer than what we've been given, we truncate to what we have. If there's
anything to do at this point (i.e., the supplied buffer had at least one byte),
then we do it.
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
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.
