[[inferior events] partial cleanup for copy_py_list

[[inferior events] partial cleanup for copy_py_list

[Tom Tromey]

This fixes some issues in copy_py_list, but not all.

The patch changes it to check PyList_Append.

I think it should probably use an iterator to go through the list.
Otherwise it is susceptible to a bug where another thread changes the
list while we are iterating over it.

Still, ok to push in this form?

Tom

diff --git a/gdb/python/python.c b/gdb/python/python.c
index 7c686d3..34e5bf8 100644
--- a/gdb/python/python.c
+++ b/gdb/python/python.c
@@ -163,10 +163,11 @@ compute_python_string (struct command_line *l)
 }
 
 /* Returns a a copy of the give LIST.
-   Creates a new reference which must be handeld by the caller.  */
+   Creates a new reference which must be handled by the caller.  */
 
 PyObject *
-copy_py_list (PyObject *list){
+copy_py_list (PyObject *list)
+{
   int i;
 
   PyObject *new_list = PyList_New (0);
@@ -174,7 +175,11 @@ copy_py_list (PyObject *list){
     return NULL;
 
   for (i = 0; i < PyList_Size (list); i++)
-    PyList_Append (new_list, PyList_GET_ITEM (list, i));
+    if (PyList_Append (new_list, PyList_GET_ITEM (list, i)) < 0)
+      {
+	Py_DECREF (new_list);
+	return NULL;
+      }
 
   return new_list;
 }

Re: [[inferior events] partial cleanup for copy_py_list

[sami wagiaalla]

On 12/14/2010 04:41 PM, Tom Tromey wrote:
> This fixes some issues in copy_py_list, but not all.
>
> The patch changes it to check PyList_Append.
>
> I think it should probably use an iterator to go through the list.
> Otherwise it is susceptible to a bug where another thread changes the
> list while we are iterating over it.
>

Hmm I did not know we had the option of using a safe iterator. If we do 
then that eliminates the need for copy_py_list in the first place; 
copy_py_list is used to make the call back of listeners safe against one 
listener editing the list during the iteration by removing an element.

> Still, ok to push in this form?
>

Yes. And thanks for the cleanups.

Sami

Re: [[inferior events] partial cleanup for copy_py_list

[Tom Tromey]

Tom> I think it should probably use an iterator to go through the list.
Tom> Otherwise it is susceptible to a bug where another thread changes the
Tom> list while we are iterating over it.

Sami> Hmm I did not know we had the option of using a safe iterator. If we
Sami> do then that eliminates the need for copy_py_list in the first place;
Sami> copy_py_list is used to make the call back of listeners safe against
Sami> one listener editing the list during the iteration by removing an
Sami> element.

TBH, I am not totally sure how things work in this area.

One option would be to use the existing convenience function
PyList_AsTuple, and just trust the Python core to get it right.

Tom