8:24 a.m.
Changes from [v3]:
* Make sure errors from virFileWrapperFdClose() result in an
overall failure being bubbled up;
* don't call virReportError() unless the function is going to
report the failure through its return code as well;
* revert b0c3e931804a more thoroughly.
Changes from [v2]:
* Move error reporting from virFileWrapperFdFree() to
virFileWrapperFdClose().
Changes from [v1]:
* Use VIR_FREE() followed by VIR_ALLOC_N() instead of manually
setting the last (and only) byte of the array returned by
VIR_REALLOC_N() to zero.
[v3] https://www.redhat.com/archives/libvir-list/2019-February/msg01069.html
[v2] https://www.redhat.com/archives/libvir-list/2019-February/msg00782.html
[v1] https://www.redhat.com/archives/libvir-list/2019-February/msg00156.html
Andrea Bolognani (4):
util: Make it safe to call virFileWrapperFdClose() multiple times
qemu: Always call virFileWrapperFdClose()
util: Move error reporting back to virFileWrapperFdClose()
util: Report error in virFileWrapperFdClose()
src/qemu/qemu_driver.c | 16 ++++++++++------
src/util/virfile.c | 22 ++++++++++++++++------
2 files changed, 26 insertions(+), 12 deletions(-)
--
2.20.1
8:24 a.m.
New subject: [libvirt] [PATCH v4 1/4] util: Make it safe to call virFileWrapperFdClose() multiple times
We'll want to use this function in the cleanup path soon,
and in order to be able to do that we need to make sure we
can call it multiple times on the same virFileWrapperFd
without side effects.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virfile.c | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 271bf5e796..42add5a2cd 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -175,6 +175,7 @@ virFileDirectFdFlag(void)
/* Opaque type for managing a wrapper around a fd. For now,
* read-write is not supported, just a single direction. */
struct _virFileWrapperFd {
+ bool closed; /* Whether virFileWrapperFdClose() has been already called */
virCommandPtr cmd; /* Child iohelper process to do the I/O. */
char *err_msg; /* stderr of @cmd */
};
@@ -323,16 +324,21 @@ virFileWrapperFdNew(int *fd ATTRIBUTE_UNUSED,
* callers can conditionally create a virFileWrapperFd wrapper but
* unconditionally call the cleanup code. To avoid deadlock, only
* call this after closing the fd resulting from virFileWrapperFdNew().
+ *
+ * This function can be safely called multiple times on the same @wfd.
*/
int
virFileWrapperFdClose(virFileWrapperFdPtr wfd)
{
int ret;
- if (!wfd)
+ if (!wfd || wfd->closed)
return 0;
ret = virCommandWait(wfd->cmd, NULL);
+
+ wfd->closed = true;
+
return ret;
}
--
2.20.1
8:24 a.m.
New subject: [libvirt] [PATCH v4 2/4] qemu: Always call virFileWrapperFdClose()
Right now we're reporting errors in virFileWrapperFdFree(),
but that's hardly the appropriate place to do so, as free
functions are supposed to do nothing more than release
allocated resources.
We want to move that code back into virFileWrapperFdClose(),
but before we can do that we need to make sure the function
is actually called every time we're done processing the
wrapped file. The cleanup path is the obvious candidate.
In a couple of cases we can just move the call, but for the
remaining ones we need to duplicate it instead in order not
to alter the existing behavior. We do, however, make sure
that in all cases a failure to properly close the wrapper
results in the overall operation being reported as failed.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/qemu/qemu_driver.c | 16 ++++++++++------
1 file changed, 10 insertions(+), 6 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c
index fe1b7801e9..323c7824be 100644
--- a/src/qemu/qemu_driver.c
+++ b/src/qemu/qemu_driver.c
@@ -3231,6 +3231,8 @@ qemuDomainSaveMemory(virQEMUDriverPtr driver,
cleanup:
VIR_FORCE_CLOSE(fd);
+ if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
+ ret = -1;
virFileWrapperFdFree(wrapperFd);
virObjectUnref(cfg);
@@ -3834,9 +3836,11 @@ doCoreDump(virQEMUDriverPtr driver,
cleanup:
VIR_FORCE_CLOSE(fd);
+ if (qemuFileWrapperFDClose(vm, wrapperFd) < 0)
+ ret = -1;
+ virFileWrapperFdFree(wrapperFd);
if (ret != 0)
unlink(path);
- virFileWrapperFdFree(wrapperFd);
VIR_FREE(compressedpath);
virObjectUnref(cfg);
return ret;
@@ -7043,17 +7047,17 @@ qemuDomainRestoreFlags(virConnectPtr conn,
ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, data, path,
false, QEMU_ASYNC_JOB_START);
- if (virFileWrapperFdClose(wrapperFd) < 0)
- VIR_WARN("Failed to close %s", path);
qemuProcessEndJob(driver, vm);
cleanup:
virDomainDefFree(def);
VIR_FORCE_CLOSE(fd);
+ if (virFileWrapperFdClose(wrapperFd) < 0)
+ ret = -1;
+ virFileWrapperFdFree(wrapperFd);
virQEMUSaveDataFree(data);
VIR_FREE(xmlout);
- virFileWrapperFdFree(wrapperFd);
if (vm && ret < 0)
qemuDomainRemoveInactiveJob(driver, vm);
virDomainObjEndAPI(&vm);
@@ -7316,14 +7320,14 @@ qemuDomainObjRestore(virConnectPtr conn,
ret = qemuDomainSaveImageStartVM(conn, driver, vm, &fd, data, path,
start_paused, asyncJob);
- if (virFileWrapperFdClose(wrapperFd) < 0)
- VIR_WARN("Failed to close %s", path);
cleanup:
virQEMUSaveDataFree(data);
VIR_FREE(xmlout);
virDomainDefFree(def);
VIR_FORCE_CLOSE(fd);
+ if (virFileWrapperFdClose(wrapperFd) < 0)
+ ret = -1;
virFileWrapperFdFree(wrapperFd);
return ret;
}
--
2.20.1
8:24 a.m.
New subject: [libvirt] [PATCH v4 3/4] util: Move error reporting back to virFileWrapperFdClose()
virFileWrapperFdFree(), like all free functions, is supposed
to only release allocated resources, so error reporting is
better suited for virFileWrapperFdClose().
This reverts commit b0c3e931804a86cd7146db0164ab4843039c410b.
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virfile.c | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index 42add5a2cd..d35206b0dd 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -337,6 +337,9 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
ret = virCommandWait(wfd->cmd, NULL);
+ if (wfd->err_msg && *wfd->err_msg)
+ VIR_WARN("iohelper reports: %s", wfd->err_msg);
+
wfd->closed = true;
return ret;
@@ -357,11 +360,6 @@ virFileWrapperFdFree(virFileWrapperFdPtr wfd)
if (!wfd)
return;
- if (wfd->err_msg && *wfd->err_msg)
- VIR_WARN("iohelper reports: %s", wfd->err_msg);
-
- virCommandAbort(wfd->cmd);
-
VIR_FREE(wfd->err_msg);
virCommandFree(wfd->cmd);
VIR_FREE(wfd);
--
2.20.1
8:25 a.m.
New subject: [libvirt] [PATCH v4 4/4] util: Report error in virFileWrapperFdClose()
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.
After this commit, the user will be able to see the error directly
instead of having to dig in the journal or libvirtd log.
https://bugzilla.redhat.com/show_bug.cgi?id=1578741
Signed-off-by: Andrea Bolognani <abologna(a)redhat.com>
---
src/util/virfile.c | 10 ++++++++--
1 file changed, 8 insertions(+), 2 deletions(-)
diff --git a/src/util/virfile.c b/src/util/virfile.c
index d35206b0dd..31030c7b34 100644
--- a/src/util/virfile.c
+++ b/src/util/virfile.c
@@ -337,8 +337,14 @@ virFileWrapperFdClose(virFileWrapperFdPtr wfd)
ret = virCommandWait(wfd->cmd, NULL);
- if (wfd->err_msg && *wfd->err_msg)
- VIR_WARN("iohelper reports: %s", wfd->err_msg);
+ /* If the command used to process I/O has failed and produced some
+ * messages on stderr, it's fair to assume those will be more
+ * relevant to the user than whatever eg. QEMU can figure out on its
+ * own having no knowledge of the fact a command is handling its I/O
+ * in the first place, so it's okay if we end up discarding an
+ * existing error here */
+ if (ret < 0 && wfd->err_msg && *wfd->err_msg)
+ virReportError(VIR_ERR_OPERATION_FAILED, "%s", wfd->err_msg);
wfd->closed = true;
--
2.20.1
7:18 a.m.
On Wed, Feb 20, 2019 at 03:24:56PM +0100, Andrea Bolognani wrote:
Changes from [v3]:
* Make sure errors from virFileWrapperFdClose() result in an
overall failure being bubbled up;
* don't call virReportError() unless the function is going to
report the failure through its return code as well;
* revert b0c3e931804a more thoroughly.
Changes from [v2]:
* Move error reporting from virFileWrapperFdFree() to
virFileWrapperFdClose().
Changes from [v1]:
* Use VIR_FREE() followed by VIR_ALLOC_N() instead of manually
setting the last (and only) byte of the array returned by
VIR_REALLOC_N() to zero.
[v3] https://www.redhat.com/archives/libvir-list/2019-February/msg01069.html
[v2] https://www.redhat.com/archives/libvir-list/2019-February/msg00782.html
[v1] https://www.redhat.com/archives/libvir-list/2019-February/msg00156.html
Andrea Bolognani (4):
util: Make it safe to call virFileWrapperFdClose() multiple times
qemu: Always call virFileWrapperFdClose()
util: Move error reporting back to virFileWrapperFdClose()
util: Report error in virFileWrapperFdClose()
src/qemu/qemu_driver.c | 16 ++++++++++------
src/util/virfile.c | 22 ++++++++++++++++------
2 files changed, 26 insertions(+), 12 deletions(-)
Reviewed-by: Ján Tomko <jtomko(a)redhat.com>
Jano
2098
days inactive
2103
days old
5 comments
2 participants
participants (2)
-
Andrea Bolognani -
Ján Tomko