[Date Prev][Date Next] [Thread Prev][Thread Next]
Re: [libvirt] [PATCH 0/2] *** use virGetLastErrorMessage ***
- From: Cole Robinson <crobinso redhat com>
- To: Erik Skultety <eskultet redhat com>, libvir-list redhat com
- Subject: Re: [libvirt] [PATCH 0/2] *** use virGetLastErrorMessage ***
- Date: Wed, 11 May 2016 09:38:59 -0400
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
>>> 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
>>> and the patches followed this example
>>> 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?
> 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 *
> 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.
[Date Prev][Date Next] [Thread Prev][Thread Next]