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

[Erik Skultety]

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.

Regards,
Erik