[libvirt] [PATCH] qemu: Don't overwrite errors by closefd in error paths
When qemuMonitorCloseFileHandle is called in error path, we need to preserve the original error since a possible further error when running closefd monitor command is not very useful to users. --- src/qemu/qemu_monitor.c | 34 +++++++++++++++++++++++++--------- src/qemu/qemu_monitor.h | 3 ++- 2 files changed, 27 insertions(+), 10 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index e593642..4c6c66f 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1552,7 +1552,7 @@ int qemuMonitorMigrateToFd(qemuMonitorPtr mon, ret = qemuMonitorTextMigrate(mon, flags, "fd:migrate"); if (ret < 0) { - if (qemuMonitorCloseFileHandle(mon, "migrate") < 0) + if (qemuMonitorCloseFileHandle(mon, "migrate", true) < 0) VIR_WARN("failed to close migration handle"); } @@ -1962,22 +1962,34 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon, int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, - const char *fdname) + const char *fdname, + bool preserveError) { - int ret; + int ret = -1; + virErrorPtr error = NULL; + VIR_DEBUG("mon=%p fdname=%s", mon, fdname); + if (preserveError) + error = virSaveLastError(); + if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("monitor must not be NULL")); - return -1; + goto cleanup; } if (mon->json) ret = qemuMonitorJSONCloseFileHandle(mon, fdname); else ret = qemuMonitorTextCloseFileHandle(mon, fdname); + +cleanup: + if (error) { + virSetError(error); + virFreeError(error); + } return ret; } @@ -2014,9 +2026,11 @@ int qemuMonitorAddHostNetwork(qemuMonitorPtr mon, cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) + if (tapfd >= 0 && + qemuMonitorCloseFileHandle(mon, tapfd_name, true) < 0) VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) + if (vhostfd >= 0 && + qemuMonitorCloseFileHandle(mon, vhostfd_name, true) < 0) VIR_WARN("failed to close device handle '%s'", vhostfd_name); } @@ -2078,9 +2092,11 @@ int qemuMonitorAddNetdev(qemuMonitorPtr mon, cleanup: if (ret < 0) { - if (tapfd >= 0 && qemuMonitorCloseFileHandle(mon, tapfd_name) < 0) + if (tapfd >= 0 && + qemuMonitorCloseFileHandle(mon, tapfd_name, true) < 0) VIR_WARN("failed to close device handle '%s'", tapfd_name); - if (vhostfd >= 0 && qemuMonitorCloseFileHandle(mon, vhostfd_name) < 0) + if (vhostfd >= 0 && + qemuMonitorCloseFileHandle(mon, vhostfd_name, true) < 0) VIR_WARN("failed to close device handle '%s'", vhostfd_name); } @@ -2258,7 +2274,7 @@ int qemuMonitorAddDeviceWithFd(qemuMonitorPtr mon, ret = qemuMonitorTextAddDevice(mon, devicestr); if (ret < 0 && fd >= 0) { - if (qemuMonitorCloseFileHandle(mon, fdname) < 0) + if (qemuMonitorCloseFileHandle(mon, fdname, true) < 0) VIR_WARN("failed to close device handle '%s'", fdname); } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 893f3e9..71ee932 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -363,7 +363,8 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon, int fd); int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, - const char *fdname); + const char *fdname, + bool preserveError); /* XXX do we really want to hardcode 'netstr' as the -- 1.7.6
On 07/13/2011 03:25 AM, Jiri Denemark wrote:
When qemuMonitorCloseFileHandle is called in error path, we need to preserve the original error since a possible further error when running closefd monitor command is not very useful to users. --- src/qemu/qemu_monitor.c | 34 +++++++++++++++++++++++++--------- src/qemu/qemu_monitor.h | 3 ++- 2 files changed, 27 insertions(+), 10 deletions(-)
@@ -1962,22 +1962,34 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon,
int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, - const char *fdname) + const char *fdname, + bool preserveError)
Why the bool argument? Every one of the callers was adjusted to set it to true, so if no one sets it to false, it seems like it makes more sense to blindly preserveError instead of make it parameterizable, without having to tweak any callers.
{ - int ret; + int ret = -1; + virErrorPtr error = NULL; + VIR_DEBUG("mon=%p fdname=%s", mon, fdname);
+ if (preserveError) + error = virSaveLastError(); + if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("monitor must not be NULL")); - return -1; + goto cleanup; }
if (mon->json) ret = qemuMonitorJSONCloseFileHandle(mon, fdname); else ret = qemuMonitorTextCloseFileHandle(mon, fdname); + +cleanup: + if (error) { + virSetError(error); + virFreeError(error); + } return ret; }
ACK to the concept, though. If you have a future patch that will pass false, then this looks okay, otherwise, it's probably worth a simpler v2. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Wed, Jul 13, 2011 at 10:09:21 -0600, Eric Blake wrote:
On 07/13/2011 03:25 AM, Jiri Denemark wrote:
When qemuMonitorCloseFileHandle is called in error path, we need to preserve the original error since a possible further error when running closefd monitor command is not very useful to users. --- src/qemu/qemu_monitor.c | 34 +++++++++++++++++++++++++--------- src/qemu/qemu_monitor.h | 3 ++- 2 files changed, 27 insertions(+), 10 deletions(-)
@@ -1962,22 +1962,34 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon,
int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, - const char *fdname) + const char *fdname, + bool preserveError)
Why the bool argument? Every one of the callers was adjusted to set it to true, so if no one sets it to false, it seems like it makes more sense to blindly preserveError instead of make it parameterizable, without having to tweak any callers.
I guess I just wanted to stress that qemuMonitorCloseFileHandle preserves the error :-) The simpler v2 follows... Jirka
When qemuMonitorCloseFileHandle is called in error path, we need to preserve the original error since a possible further error when running closefd monitor command is not very useful to users. --- src/qemu/qemu_monitor.c | 14 ++++++++++++-- src/qemu/qemu_monitor.h | 3 +++ 2 files changed, 15 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_monitor.c b/src/qemu/qemu_monitor.c index cff7220..3a30a15 100644 --- a/src/qemu/qemu_monitor.c +++ b/src/qemu/qemu_monitor.c @@ -1964,20 +1964,30 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon, int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, const char *fdname) { - int ret; + int ret = -1; + virErrorPtr error; + VIR_DEBUG("mon=%p fdname=%s", mon, fdname); + error = virSaveLastError(); + if (!mon) { qemuReportError(VIR_ERR_INVALID_ARG, "%s", _("monitor must not be NULL")); - return -1; + goto cleanup; } if (mon->json) ret = qemuMonitorJSONCloseFileHandle(mon, fdname); else ret = qemuMonitorTextCloseFileHandle(mon, fdname); + +cleanup: + if (error) { + virSetError(error); + virFreeError(error); + } return ret; } diff --git a/src/qemu/qemu_monitor.h b/src/qemu/qemu_monitor.h index 893f3e9..f246d21 100644 --- a/src/qemu/qemu_monitor.h +++ b/src/qemu/qemu_monitor.h @@ -362,6 +362,9 @@ int qemuMonitorSendFileHandle(qemuMonitorPtr mon, const char *fdname, int fd); +/* The function preserves previous error and only sets it's own error if no + * error was set before. + */ int qemuMonitorCloseFileHandle(qemuMonitorPtr mon, const char *fdname); -- 1.7.6
On 07/14/2011 06:50 AM, Jiri Denemark wrote:
When qemuMonitorCloseFileHandle is called in error path, we need to preserve the original error since a possible further error when running closefd monitor command is not very useful to users. --- src/qemu/qemu_monitor.c | 14 ++++++++++++-- src/qemu/qemu_monitor.h | 3 +++ 2 files changed, 15 insertions(+), 2 deletions(-)
ACK. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org
On Thu, Jul 14, 2011 at 07:02:57 -0600, Eric Blake wrote:
On 07/14/2011 06:50 AM, Jiri Denemark wrote:
When qemuMonitorCloseFileHandle is called in error path, we need to preserve the original error since a possible further error when running closefd monitor command is not very useful to users. --- src/qemu/qemu_monitor.c | 14 ++++++++++++-- src/qemu/qemu_monitor.h | 3 +++ 2 files changed, 15 insertions(+), 2 deletions(-)
ACK.
Pushed, thanks. Jirka
participants (2)
-
Eric Blake -
Jiri Denemark