[libvirt] [PATCH] virterror: Don't invoke error callback for ERR_OK

[Cole Robinson]

On 01/13/2010 07:45 AM, Cole Robinson wrote:
> On 01/13/2010 04:23 AM, Daniel P. Berrange wrote:
>> On Tue, Jan 12, 2010 at 03:57:33PM -0500, Cole Robinson wrote:
>>> On 01/12/2010 03:48 PM, Daniel P. Berrange wrote:
>>>> On Tue, Jan 12, 2010 at 03:26:28PM -0500, Cole Robinson wrote:
>>>>> Since virDispatchError is now responsible for invoking the error callback,
>>>>> give it the same semantics as ReportError, which will skip VIR_ERR_OK
>>>>> (which is encountered when no error was raised).
>>>>>
>>>>> This fixes invoking the error callback after every non-erroring API call.
>>>>>
>>>>> Signed-off-by: Cole Robinson <crobinso at redhat.com>
>>>>> ---
>>>>>  src/util/virterror.c |    6 +++++-
>>>>>  1 files changed, 5 insertions(+), 1 deletions(-)
>>>>>
>>>>> diff --git a/src/util/virterror.c b/src/util/virterror.c
>>>>> index e2128b9..78974ee 100644
>>>>> --- a/src/util/virterror.c
>>>>> +++ b/src/util/virterror.c
>>>>> @@ -603,8 +603,12 @@ virDispatchError(virConnectPtr conn)
>>>>>      if (!err)
>>>>>          return;
>>>>>  
>>>>> -    /* Set a generic error message if none is already set */
>>>>> +    /* We never used to raise ERR_OK, so maintain existing behavior */
>>>>>      if (err->code == VIR_ERR_OK)
>>>>> +        return;
>>>>> +
>>>>> +    /* Set a generic error message if none is already set */
>>>>> +    if (!err->message)
>>>>>          virErrorGenericFailure(err);
>>>>>  
>>>>>      /* Copy the global error to per-connection error if needed */
>>>>
>>>> We should only ever be invoking virDispatchError() in error paths, so
>>>> if err->code == VIR_ERR_OK, this means we do need set a generic message
>>>> because the earlier code indicated an error but forgot to report one.
>>>> So I don't think this is correct.
>>>>
>>>
>>> Ah, I think I wanted to check VIR_ERR_NONE here actually.
>>> virDispatchError is called regardless of whether an error is actually
>>> raised, so it may receive a zero'd out/empty virLastErrorObject, which
>>> is what I'm trying to avoid reporting.
>>
>> I still don't think you are correct in that. If you run
>>
>>   # grep --after 1 virDispatchError libvirt.c
>>         virDispatchError(NULL);
>>         return (-1);
>>   --
>>       virDispatchError(net->conn);
>>       return -1;
>>   --
>>           virDispatchError(NULL);
>>           return (-1);
>>   --
>>       virDispatchError(pool->conn);
>>       return -1;
>>
>> Then all cases where virDispatchError() is called should be followed by the
>> return of an error code, 99% of them are -1 or NULL. There are one or two
>> where we use '0' for error as a special case. I don't see any places where
>> virDispatchError() is called in a successful return path. So we should
>> always be invoking the error callback, and ensuring an error is actually 
>> set before doing so.
>>
> 
> Whoops, sorry for the confusion. I'll investigate more.

A couple issues were conspiring to raise the 'Unknown error'. In the daemon we
were blindly trying to unregister domain events via an API call, which would
rightly error if not events had ever been registered. However, the
implementation wasn't actually setting an error message. Patches coming shortly.

Thanks,
Cole