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? Erik -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list