[libvirt] [libvirt-python PATCH 03/23] remove useless check for NULL before Py_XDECREF

[John Ferlan]

On 09/29/2015 10:29 AM, Pavel Hrdina wrote:
> On Sat, Sep 26, 2015 at 09:08:35AM -0400, John Ferlan wrote:
>>
>>
>> On 09/24/2015 10:01 AM, Pavel Hrdina wrote:
>>> Signed-off-by: Pavel Hrdina <phrdina at redhat.com>
>>> ---
>>>  libvirt-override.c | 9 +++------
>>>  1 file changed, 3 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/libvirt-override.c b/libvirt-override.c
>>> index 14aa0e9..114104b 100644
>>> --- a/libvirt-override.c
>>> +++ b/libvirt-override.c
>>> @@ -2303,12 +2303,9 @@ libvirt_virRegisterErrorHandler(ATTRIBUTE_UNUSED PyObject * self,
>>>            pyobj_f);
>>>  
>>>      virSetErrorFunc(NULL, libvirt_virErrorFuncHandler);
>>> -    if (libvirt_virPythonErrorFuncHandler != NULL) {
>>> -        Py_XDECREF(libvirt_virPythonErrorFuncHandler);
>>> -    }
>>> -    if (libvirt_virPythonErrorFuncCtxt != NULL) {
>>> -        Py_XDECREF(libvirt_virPythonErrorFuncCtxt);
>>> -    }
>>> +
>>> +    Py_XDECREF(libvirt_virPythonErrorFuncHandler);
>>> +    Py_XDECREF(libvirt_virPythonErrorFuncCtxt);
>>
>> I keep looking at this and thinking why?  Why was it added and what is
>> it protecting.  Looking at libvirt_virErrorFuncHandler it seems only
>> libvirt_virPythonErrorFuncCtxt gets the Py_XINCREF and that's based on
>> whether libvirt_virPythonErrorFuncHandler is NULL or not.
> 
> I don't understand what do you mean by this??
> 

I guess the whole Py_XDECREF usage is still a bit of a mystery. I
believe I was trying to consider why someone would have checked for NULL
previously. I know from checking it was written that way.  But I assume
now that if on input it was NULL then nothing happens.

No issues otherwise -

John
>>
>> Just feels like something subtle is going on. Also it's a register
>> handler and we're decrementing something that never got incremented.
> 
> Check the whole function.  It will decrement the old handlers, check for
> existence of new handlers and clean/set new handlers.  And before setting the
> new handlers, we need to increment them, otherwise they will be cleared by GC.
> 
> Everything that is parsed by 'PyArg_ParseTuple' has to be incremented, if you
> need to use those object outside of the function scope, where you've called that
> parser.
> 
> Pavel
> 
>>
>> John
>>>  
>>>      if ((pyobj_f == Py_None) && (pyobj_ctx == Py_None)) {
>>>          libvirt_virPythonErrorFuncHandler = NULL;
>>>