[libvirt] [PATCH 2/2] python: Expose binding for virNodeGetMemoryStats()

[Peter Krempa]

Dňa 2.12.2011 18:23, Eric Blake  wrote / napísal(a):
> On 11/28/2011 10:19 AM, Peter Krempa wrote:
>> +    if (!(ret = PyDict_New())) {
>> +        free(stats);
>> +        return VIR_PY_NONE;
>> +    }
>> +    for (i = 0; i<  nparams; i++) {
>> +        PyDict_SetItem(ret,
>> +                       libvirt_constcharPtrWrap(stats[i].field),
>> +                       libvirt_ulonglongWrap(stats[i].value));
>> +    }
>
> Copy and paste, so not a problem with this patch any more so than the
> other functions that used the same code pattern, but can PyDict_SetItem
> fail?  If so, should be be reclaiming the entries added so far before
> returning overall failure, instead of silently truncating the return
> dictionary by omitting the entries that weren't inserted properly?
>

Well, I was wondering myself why there's no check for insertion of the 
item into the dictionary as it is apparently allocating (tons of) 
memory. I was following the pattern used in already existing code.

The better approach would be:

+    for (i = 0; i<  nparams; i++) {
+        if (PyDict_SetItem(ret,
+                           libvirt_constcharPtrWrap(stats[i].field),
+                           libvirt_ulonglongWrap(stats[i].value))<0){ + 
            Py_XDECREF(ret);
+            return VIR_PY_NONE;
+        }
+    }

Free the reference and return an error. Maybe it would be useful to 
cause an exception, but I'm not a python binding guru. Should I make it 
more robust? Or maybe we should clean this up later in all instances of 
improper dictionary insertions?

Peter