[libvirt] [PATCH 0/2] *** use virGetLastErrorMessage ***

[Cole Robinson]

On 05/11/2016 07:45 AM, Erik Skultety wrote:
> On 10/05/16 18:51, Cole Robinson wrote:
>> On 05/10/2016 11:44 AM, Erik Skultety wrote:
>>> On 10/05/16 14:16, Jovanka Gulicoska wrote:
>>>> *** BLURB HERE ***
>>>>
>>>
>>> Hi, thank you for patches :), just a small advice to the future, we tend
>>> to write a couple of words/sentences, instead of the "BLURB HERE", what
>>> the series actually does. Also, the subject could be slightly more
>>> verbose and the asterisks are unnecessary.
>>>
>>> Anyway, I've seen some minor issues in the patches, but those really
>>> were just details like indentation being off in 2 places I think, on
>>> specific places virGetLastErrorMessage could be used directly as an
>>> argument to another function, instead of first assigning the result of
>>> this function to a variable and then using that variable as an argument
>>> to another function without any other usage for that variable, and maybe
>>> slightly more descriptive commit messages, but I went through them only
>>> briefly...
>>>
>>> The reason why I'm replying to this mail is that regardless of the
>>> patches being correct or not, I would like to postpone accepting these
>>> patches until the virGetLastErrorMessage function is fixed.
>>> I know that the original idea comes from our wiki page
>>> http://wiki.libvirt.org/page/BiteSizedTasks#More_usage_of_virGetLastErrorMessage.28.29
>>> and the patches followed this example
>>> http://www.redhat.com/archives/libvir-list/2016-March/msg00896.html.
>>> But it wasn't until I reviewed
>>> https://www.redhat.com/archives/libvir-list/2016-May/msg00516.html that
>>> I realized that there is a problem with virGetLastErrorMessage and
>>> cannot be used as a replacement for virGetLastError until it's fixed. So
>>> I'd like to express my concerns once again. Basically, the problem lies
>>> within calling virLastErrorObject from both virGetLastError and
>>> virGetLastErrorMessage functions. It's a slight misinterpretation from
>>> our side that if virLastErrorObject returns NULL it means that no error
>>> occurred - wrong, virLastErrorObject would always return a valid object
>>> stating that there was or wasn't an error, while NULL means that either
>>> it failed to allocate a new thread-local error object or it failed to
>>> set this newly allocated thread-local object. But while virGetLastError
>>> handles this just by passing the pointer back to the original caller and
>>> they need to check for it (err && err->message), virGetLastErrorMessage
>>> on the other hand would just return "no error" in this case and the user
>>> would get a message like "internal error: no error" which surely looks odd.
>>>
>>> Now, I know that patch 1/2 also fixes an issue with our tests where a
>>> NULL dereference might occur and should be fixed, but that isn't the
>>> case for libvirt's code.
>>> This reply was meant to provide some details for the next reviewer, so
>>> they could take that into account when reviewing, but from my
>>> perspective, we can wait a little longer until we sort things out in the
>>> virterror module first, namely in the virGetLastErrorMessage function.
>>>
>>
>> Hmmmm. So to summarize, the cases that virGetLastErrorMessage() will return
>> "no error" when a possible error actually occurred is:
>>
>> - virLastErrorObject VIR_ALLOC_QUIET(err) fails
>> - virLastErrorObject virThreadLocalSet fails
>>
>> virThreadLocalSet is just pthread_setspecific, which the only error code
>> that's documented is ENOMEM. So it's basically the same as the ALLOC failure.
>>
>> Maybe have virGetLastErrorMessage() save errno and check for ENOMEM after
>> calling virLastErrorObject(), but I'm not really sure it's worth the
>> complexity: once we get to the point of malloc failing I assume functionality
>> is going to degrade rapidly, so a less accurate error isn't going to be
>> impactful in that case.
>>
>> What are your thoughts Erik?
>>
>> Thanks,
>> Cole
>>
> I have to admit that I didn't know that pthread_setspecific returned
> ENOMEM. Sure, once malloc fails, either being called directly from our
> code or indirectly from a system library, incorrect message would be the
> least of our concerns.
> Hmm, you're right about saving the errno, it probably wouldn't be worth
> it. How about just tweaking the conditions like this (the complexity of
> the change equals one more operand and a change in logical operation):
> 
> diff --git a/src/util/virerror.c b/src/util/virerror.c
> index 5d875e3..1177570 100644
> --- a/src/util/virerror.c
> +++ b/src/util/virerror.c
> @@ -281,9 +281,9 @@ const char *
>  virGetLastErrorMessage(void)
>  {
>      virErrorPtr err = virLastErrorObject();
> -    if (!err || err->code == VIR_ERR_OK)
> +    if (err && err->code == VIR_ERR_OK)
>          return _("no error");
> -    if (err->message == NULL)
> +    if (!err || !err->message)
>          return _("unknown error");
>      return err->message;
>  }
> 
> Compared to virGetLastError which returns NULL when malloc failed (one
> way or the other) and we just format it as "Unknown error", the
> behaviour would remain the same with the above proposed change. What do
> you think?
> 

Good idea, I think that's an improvement.

Thanks,
Cole