Re: [libvirt] [PATCH 1/3] Use virGetLastErrorMessage to avoid Coverity message
On 07/05/16 17:46, Cole Robinson wrote:
On 05/07/2016 11:04 AM, Erik Skultety wrote:
> On 06/05/16 21:34, John Ferlan wrote:
>> Both instances use VIR_WARN() to print the error from a failed
>> virDBusGetSystemBus() call. Rather than use the virGetLastError
>> and need to check for valid return err pointer, just use the
>> virGetLastErrorMessage.
>>
>
> I'm afraid these are not equivalent. virGetLastError states that it
> returns NULL if no error occurred, which isn't true in all the cases,
> i.e. both virGetLastError and virGetLastErrorMessage call
> virLastErrorObject which can actually fail when no threadlocal error
> object was found (this is OK), but then we try to create an empty one
> and assign it to the threadlocal storage, so if the allocation fails
> quietly (I think trying to log a proper message would be least of our
> concerns), but if setting the new, empty error object as thread-specific
> data fails, it will return NULL which virGetLastError just passes along
> and the original caller deals with this by checking the pointer and if
> it's NULL, "Unknown error" is used instead. Now, in your case however,
> should such a corner-case scenario happen, virGetLastErrorMessage
> interprets the NULL from virLastErrorObject as "no error" which isn't
> quite the same, it might even get a little confusing back at the client
> side. So I prefer we stick to the "checking" convention, unless we want
> to make some adjustments to the virerror module.
>
Prior to this patch, virGetLastError returning NULL would lead to a NULL
dereference. So using virGetLastErrorMessage() here is a clear win, and is
exactly what the API is meant to handle.
Well, I do know that in this case it would lead to NULL dereference, but
that wasn't the point I was trying to make.
_if_ the original code had checked for err != NULL before trying to
print
err->message, then indeed the behavior would change a bit. That said I think
converting to virGetLastErrorMessage() is still a win, and we should just
improve/clarify virGetLastErrorMessage() separately.
Exactly as I said at the end, we either stick to the way we do it now
(be it or be it not the optimal way) or we need to do some adjustments
to virterror module which, as as you pointed out specifically, would
lead to some cosmetic improvements/adjustments/clarifications to
virGetLastErrorMessage and as long as there is a chance of doing it one
day, I'm of course okay with that.
Erik
ACK to this patch.
- Cole