[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12792483#action_12792483 ] Hudson commented on ZOOKEEPER-600: -- Integrated in ZooKeeper-trunk #634 (See [http://hudson.zones.apache.org/hudson/job/ZooKeeper-trunk/634/]) . TODO pondering about allocation behavior in zkpython may be removed (gustavo via mahadev) TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 Attachments: deallocate-vector.patch I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12791901#action_12791901 ] Gustavo Niemeyer commented on ZOOKEEPER-600: I believe it's ready for integration. TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 Attachments: deallocate-vector.patch I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12792069#action_12792069 ] Henry Robinson commented on ZOOKEEPER-600: -- Yes, this looks good to me - I'd like to see a test included, but we have no infrastructure for C-side tests which this would probably need. Can be committed as far as I am concerned. Thanks Gustavo! TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 Attachments: deallocate-vector.patch I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12791681#action_12791681 ] Mahadev konar commented on ZOOKEEPER-600: - henry/gustavo, is this ready to commit? TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 Attachments: deallocate-vector.patch I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12789476#action_12789476 ] Hadoop QA commented on ZOOKEEPER-600: - -1 overall. Here are the results of testing the latest attachment http://issues.apache.org/jira/secure/attachment/12426565/deallocate-vector.patch against trunk revision 888216. +1 @author. The patch does not contain any @author tags. -1 tests included. The patch doesn't appear to include any new or modified tests. Please justify why no tests are needed for this patch. +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-h7.grid.sp2.yahoo.net/22/testReport/ Findbugs warnings: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/artifact/trunk/build/test/findbugs/newPatchFindbugsWarnings.html Console output: http://hudson.zones.apache.org/hudson/job/Zookeeper-Patch-h7.grid.sp2.yahoo.net/22/console This message is automatically generated. TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 Attachments: deallocate-vector.patch I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12784352#action_12784352 ] Henry Robinson commented on ZOOKEEPER-600: -- Patch applies fine for me and tests all pass - looks good, thanks Gustavo! I'll open a JIRA for the other issues. TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 Attachments: deallocate-vector.patch I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12783919#action_12783919 ] Henry Robinson commented on ZOOKEEPER-600: -- Correct - my concern is over whether PyString_FromString copies the C string it is given. If it does not, then a String_vector is allocated by zoo_wgetchildren and never freed. http://docs.python.org/c-api/string.html hints that a copy is made, and therefore the memory needs freeing. This would be relatively easy to check by memsetting all strings in the String_vector to 'A' after the copy, and then checking to see if the Python-side strings are altered. Alternatively, you could check the Python C API source - it should be fairly clear what the answer is. Thanks for picking this up! TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12783931#action_12783931 ] Gustavo Niemeyer commented on ZOOKEEPER-600: Ah, I see. Yes, PyString_FromString will definitely copy the string over, so the strings.data array is leaking if it has data allocated dynamically. In addition to this, PyString_FromString and PyList_New are both allocating memory, and thus they may fail to return a proper object. This isn't being checked currently. TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12783933#action_12783933 ] Gustavo Niemeyer commented on ZOOKEEPER-600: Patrick, thanks for pointing me to the conventions, and sorry for missing it earlier. I'll give it a shot and submit a patch soon. TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.
[jira] Commented: (ZOOKEEPER-600) TODO pondering about allocation behavior in zkpython may be removed
[ https://issues.apache.org/jira/browse/ZOOKEEPER-600?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanelfocusedCommentId=12784056#action_12784056 ] Patrick Hunt commented on ZOOKEEPER-600: no worries, we don't expect first time contributors to know everything. ;-) thanks for the interest. TODO pondering about allocation behavior in zkpython may be removed --- Key: ZOOKEEPER-600 URL: https://issues.apache.org/jira/browse/ZOOKEEPER-600 Project: Zookeeper Issue Type: Bug Components: contrib-bindings Affects Versions: 3.2.1 Reporter: Gustavo Niemeyer Assignee: Gustavo Niemeyer Priority: Trivial Fix For: 3.3.0 I suppose the TODO below is referring to the path variable which is passed in as an output variable to PyArg_ParseTuple right below. The TODO may be removed, since the code is right. Code using PyArg_ParseTuple will borrow the reference from the calling code, since there's a stack behind the call to the enclosing function (pyzoo_get_children in this case) which won't go away until the function returns. Index: src/contrib/zkpython/src/c/zookeeper.c === --- src/contrib/zkpython/src/c/zookeeper.c(revision 885582) +++ src/contrib/zkpython/src/c/zookeeper.c(working copy) @@ -774,8 +774,6 @@ static PyObject *pyzoo_get_children(PyObject *self, PyObject *args) { - // TO DO: Does Python copy the string or the reference? If it's the former - // we should free the String_vector int zkhid; char *path; PyObject *watcherfn = Py_None; -- This message is automatically generated by JIRA. - You can reply to this email to add a comment to the issue online.