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

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_virGetLastError... 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.