On 07/16/2014 08:30 AM, Eric Blake wrote: > On 07/16/2014 08:12 AM, Jason J. Herne wrote: >> From: "Jason J. Herne" <jjherne@xxxxxxxxxx> >> >> libvirt_iohelper is a helper process that is exec'ed and used to handle I/O >> during a Qemu managed save operation. Due to a missing call to >> virFileWrapperFdClose, all I/O error messages reported by iohelper are lost. >> >> This patch adds a call to virFileWrapperFdClose to the cleanup phase of >> qemuDomainSaveMemory. >> >> This patch also modifies virFileWrapperFdClose such that errors are only >> reported when the length of the err_msg buffer is > 0. Before now, the >> existence of the buffer would trigger error reporting in virFileWrapperFdClose. >> >> Signed-off-by: Jason J. Herne <jjherne@xxxxxxxxxx> >> --- >> src/qemu/qemu_driver.c | 1 + >> src/util/virfile.c | 2 +- >> 2 files changed, 2 insertions(+), 1 deletion(-) > > Nice! Thanks for persevering on this one. Congrats on your first > libvirt patch. Sadly, the more I look at this, the more I wonder how it can fix anything. > >> >> diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c >> index ecccf6c..8d78805 100644 >> --- a/src/qemu/qemu_driver.c >> +++ b/src/qemu/qemu_driver.c >> @@ -3015,6 +3015,7 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver, >> >> cleanup: >> VIR_FORCE_CLOSE(fd); >> + virFileWrapperFdClose(wrapperFd); >> virFileWrapperFdFree(wrapperFd); >> VIR_FREE(xml); But qemuDomainSaveMemory() already has a call to virFileWrapperFdClose() earlier on; worse, the current implementation of virFileWrapperFdClose() is not designed to be called twice if an error occurred (rather, if there is an error, two calls will end up logging the error twice, when once would have been sufficient). How are you getting to a point where the cleanup label is reached without going through the earlier close? Is there some easy setup you used in testing this patch, so that I can reproduce a scenario where iohelper will reliably fail? -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
Attachment:
signature.asc
Description: OpenPGP digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list