[libvirt] [PATCH v2 2/3] virfile: Report error in virFileWrapperFdFree()

[John Ferlan]

On 2/19/19 8:27 AM, Andrea Bolognani wrote:
> On Mon, 2019-02-18 at 11:04 -0500, John Ferlan wrote:
>>
>> On 2/13/19 7:04 AM, Andrea Bolognani wrote:
>>> Logging the error is fine and all, but getting the information
>>> to the user directly is even better.
>>>
>>> https://bugzilla.redhat.com/show_bug.cgi?id=1578741
>>> Signed-off-by: Andrea Bolognani <abologna at redhat.com>
>>> ---
>>>  src/util/virfile.c | 6 +++++-
>>>  1 file changed, 5 insertions(+), 1 deletion(-)
>>>
>>
>> Shall I assume this works because qemuProcessHandleDumpCompleted will
>> move this message now as opposed to some other message related to EPIPE
>> as noted in the bz response?
> 
> libvirt_iohelper is used internally by the virFileWrapperFd APIs;
> more specifically, in the QEMU driver we have the doCoreDump() and
> qemuDomainSaveMemory() helper functions as users, and those in turn
> end up being called by the implementation of several driver APIs.
> 
> By calling virReportError() if libvirt_iohelper has failed, we
> overwrite whatever generic error message QEMU might have raised
> with the more useful one generated by the helper program.
> 
> I'm not sure how qemuProcessHandleDumpCompleted() fits into the
> picture.
> 

My recollection is/was event based mechanism and the HandleDumpCompleted
was what ended up being where the message came through whether success
or failure. Could be wrong - I perhaps had more in short term recall
when I added some processing for memory dump event processing.

>> I think it's good to fill in pieces of the commit message at least so if
>> someone else ends up in this same code they have a few hints where to start.
> 
> Yeah, I guess the commit message is a bit terse. Would adding
> something along the lines of the first two paragraphs above work,
> in your opinion?
> 

Sure that's fine. I understand it doesn't mean someone will look at the
commit message, but for those that do it can help.

>> My other throughts would be to note commit 1f25194ad and 34e8f63a3 as
>> the genesis of at least printing the message "somewhere"... Then commit
>> b0c3e931 at least made sure the message got printed in the event that
>> the *FdClose never occurred. Just saying "is fine and all" hand waves a
>> bit too much for me compared to what you got to figure out here ;-).
> 
> Honestly, I didn't bother digging into the history of the
> functionality and simply fixed what was broken without really
> wondering how it ended up like that in the first place :)
> 

Thankfully gitk makes it easy for me to trace through the history. I
know everyone has their favorite blame tools.

John