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 >>> 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? > Good idea, I think that's an improvement. Thanks, Cole -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list