GNOME Bugzilla – Bug 683150
Assignment of pointer fields to None does not set them to NULL
Last modified: 2012-09-11 05:40:04 UTC
Pointer fields are special cased for getting but not setting of None to NULL. For instance, Gtk.TreeIter specifies a series of "user_data" void pointers which are initially NULLed out and reported as "None" within python. However, setting them to None sets the pointers to the address of the None python object instead of NULL. For example: import sys import ctypes from gi.repository import Gtk class RawTreeIter(ctypes.Structure): _fields_ = [('stamp', ctypes.c_int), ('user_data', ctypes.c_void_p), ('user_data2', ctypes.c_void_p), ('user_data3', ctypes.c_void_p)] @classmethod def from_iter(cls, iter): offset = sys.getsizeof(object()) # size of PyObject_HEAD return ctypes.POINTER(cls).from_address(id(iter)+offset) pyiter = Gtk.TreeIter() rawiter = RawTreeIter.from_iter(pyiter) # Initially both the iter and raw iter show user_data as None pyiter.user_data == None # True rawiter.contents.user_data == None # True # Explicitly setting to None causes the address of the None python object to be set: pyiter.user_data = None pyiter.user_data == None # True rawiter.contents.user_data == None # False, should be True rawiter.contents.user_data == id(None) # True, should be False
Created attachment 223131 [details] [review] Fixed ability to set pointer fields/arguments to NULL using None This is a simple patch which sets pointers to NULL when using pythons None, rather then setting to the address of the None instance.
Great fix, thanks Simon! Applied with a slightly shorter changelog short description.
Created attachment 223734 [details] [review] Fix setting pointer fields/arguments to NULL using None (redux) This additional patch fixes the same problem but does not mess with the ref count of None when filling out the argument. But instead changes pygi-info to increment what is assigned to the pointer struct rather then the input python object. Which in this case will be NULL. Also added some comments to hopefully help readers understand what is going on in this part of the code. I'm unsure of the process but since this ticket has been close, should it be re-opened or a new ticket created in situations like this?
Un-resolving as there is a newer better patch, let me know if this is the right thing to do.
Simon, your patches are exactly identical, it seems you attached the old one again? Thanks for working on this!
Comment on attachment 223734 [details] [review] Fix setting pointer fields/arguments to NULL using None (redux) (this one already was committed)
Created attachment 223787 [details] [review] Fix setting pointer fields/arguments to NULL using None (redux) Yep, that was that last patch was a mistake, here is the new one.
Hello Simon, Admittedly I don't quite understand this, as I'm not familiar with that part of the code, so I'd appreciate some explanation (I'd like to learn about this). Why does it not need the Py_DECREF() any more? Certainly because of the last hunk, but admittedly I don't understand what the Py_XINCREF part does (not inc'ing the pyvalue any more, but the raw C struct? does that even work?) Thanks! Martin
Created attachment 223966 [details] [review] Added more comments Hi Martin, The GI struct accessors doubly obfuscate what's going on. I've added some additional comments which also show what the code would look like with as if it were a regular C struct if it helps any. In terms of the initial patch I submitted for this, the code in pygi-argument.c:_pygi_argument_from_object was explicitly decrefing Py_None because it was assuming the caller was incorrectly increfing it and it wasn't going to be used for this case. A callee having this kind of knowledge seems incorrect. The caller in this case (pygi-info.c:_wrap_g_field_info_set_value) was always increfing the input py_value variable used for the assignment as opposed to what is actually being assigned to the struct. Even if the value was None and didn't need the incref because NULL would be assigned. The newer patch removes the incorrect ref counting knowledge from _pygi_argument_from_object and changes the caller to incref the assigned pointer on the struct itself not the input py_value, in the case of None, the struct pointer would end up as NULL which Py_XINCREF will ignore, hence the previous decref became unnecessary. Hope this helps clear it up a bit :)
It does, thanks Simon! However, I'm afraid you attached the wrong patch again :-(
Created attachment 223976 [details] [review] Correct patch Time to delete all the patch files in my pygobject directory as it is getting confusing.
Comment on attachment 223976 [details] [review] Correct patch Thanks! Committed with some changelog tweaking.
(In reply to comment #11) > Created an attachment (id=223976) [details] [review] > Correct patch > > Time to delete all the patch files in my pygobject directory as it is getting > confusing. git-bz works great for this: http://git.fishsoup.net/man/git-bz.html
That looks very useful, thanks Tomeu!