[libvirt] [PATCH v2] qemu: return -1 on error paths in qemuDomainSaveImageStartVM
Error paths after sending the event that domain is started written as if ret = -1 which is set at the beginning of the function. It's common idioma to keep 'ret' equal to -1 until the end of function where it is set to 0. But here we use ret to keep result of restore operation too and thus breaks the idioma and its users :) Let's use different variable to hold restore result. Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-) diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 351e529..ced105b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6732,6 +6732,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainAsyncJob asyncJob) { int ret = -1; + bool restored; virObjectEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; @@ -6758,13 +6759,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, } /* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, asyncJob, - "stdio", *fd, path, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, - VIR_QEMU_PROCESS_START_PAUSED); + restored = qemuProcessStart(conn, driver, vm, asyncJob, + "stdio", *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, + VIR_QEMU_PROCESS_START_PAUSED) >= 0; if (intermediatefd != -1) { - if (ret < 0) { + if (!restored) { /* if there was an error setting up qemu, the intermediate * process will wait forever to write to stdout, so we * must manually kill it. @@ -6775,7 +6776,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (virCommandWait(cmd, NULL) < 0) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0); - ret = -1; + restored = false; } VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf)); } @@ -6783,18 +6784,16 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, if (VIR_CLOSE(*fd) < 0) { virReportSystemError(errno, _("cannot close file: %s"), path); - ret = -1; + restored = false; } - if (ret < 0) { - virDomainAuditStart(vm, "restored", false); + virDomainAuditStart(vm, "restored", restored); + if (!restored) goto cleanup; - } event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED); - virDomainAuditStart(vm, "restored", true); qemuDomainEventQueue(driver, event); -- 1.8.3.1
29.01.2016 11:49, Nikolay Shirokovskiy пишет:
Error paths after sending the event that domain is started written as if ret = -1 which is set at the beginning of the function. It's common idioma to keep 'ret' equal to -1 until the end of function where it is set to 0. But here we use ret to keep result of restore operation too and thus breaks the idioma and its users :)
Let's use different variable to hold restore result.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 351e529..ced105b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6732,6 +6732,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainAsyncJob asyncJob) { int ret = -1; + bool restored; virObjectEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; @@ -6758,13 +6759,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, }
/* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, asyncJob, - "stdio", *fd, path, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, - VIR_QEMU_PROCESS_START_PAUSED); + restored = qemuProcessStart(conn, driver, vm, asyncJob, + "stdio", *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, + VIR_QEMU_PROCESS_START_PAUSED) >= 0;
if (intermediatefd != -1) { - if (ret < 0) { + if (!restored) { /* if there was an error setting up qemu, the intermediate * process will wait forever to write to stdout, so we * must manually kill it. @@ -6775,7 +6776,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
if (virCommandWait(cmd, NULL) < 0) { qemuProcessStop(driver, vm, VIR_DOMAIN_SHUTOFF_FAILED, 0); - ret = -1; + restored = false; } VIR_DEBUG("Decompression binary stderr: %s", NULLSTR(errbuf)); } @@ -6783,18 +6784,16 @@ qemuDomainSaveImageStartVM(virConnectPtr conn,
if (VIR_CLOSE(*fd) < 0) { virReportSystemError(errno, _("cannot close file: %s"), path); - ret = -1; + restored = false; }
- if (ret < 0) { - virDomainAuditStart(vm, "restored", false); + virDomainAuditStart(vm, "restored", restored); + if (!restored) goto cleanup; - }
event = virDomainEventLifecycleNewFromObj(vm, VIR_DOMAIN_EVENT_STARTED, VIR_DOMAIN_EVENT_STARTED_RESTORED); - virDomainAuditStart(vm, "restored", true); qemuDomainEventQueue(driver, event);
ACK.
On Fri, Jan 29, 2016 at 11:49:45 +0300, Nikolay Shirokovskiy wrote:
Error paths after sending the event that domain is started written as if ret = -1 which is set at the beginning of the function. It's common idioma to keep 'ret' equal to -1 until the end of function where it is set to 0. But here we use ret to keep result of restore operation too and thus breaks the idioma and its users :)
Let's use different variable to hold restore result.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 351e529..ced105b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6732,6 +6732,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainAsyncJob asyncJob) { int ret = -1; + bool restored; virObjectEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; @@ -6758,13 +6759,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, }
/* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, asyncJob, - "stdio", *fd, path, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, - VIR_QEMU_PROCESS_START_PAUSED); + restored = qemuProcessStart(conn, driver, vm, asyncJob, + "stdio", *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, + VIR_QEMU_PROCESS_START_PAUSED) >= 0;
I was pretty confused when I saw this for the first time... it's too easy to miss the comparison at the end. I suggest changing the code to something similar to the following: bool restored = false; ... if (qemuProcessStart(conn, driver, vm, asyncJob, "stdio", *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, VIR_QEMU_PROCESS_START_PAUSED) == 0) restored = true; Jirka
On 01.02.2016 15:08, Jiri Denemark wrote:
On Fri, Jan 29, 2016 at 11:49:45 +0300, Nikolay Shirokovskiy wrote:
Error paths after sending the event that domain is started written as if ret = -1 which is set at the beginning of the function. It's common idioma to keep 'ret' equal to -1 until the end of function where it is set to 0. But here we use ret to keep result of restore operation too and thus breaks the idioma and its users :)
Let's use different variable to hold restore result.
Signed-off-by: Nikolay Shirokovskiy <nshirokovskiy@virtuozzo.com> --- src/qemu/qemu_driver.c | 21 ++++++++++----------- 1 file changed, 10 insertions(+), 11 deletions(-)
diff --git a/src/qemu/qemu_driver.c b/src/qemu/qemu_driver.c index 351e529..ced105b 100644 --- a/src/qemu/qemu_driver.c +++ b/src/qemu/qemu_driver.c @@ -6732,6 +6732,7 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, qemuDomainAsyncJob asyncJob) { int ret = -1; + bool restored; virObjectEventPtr event; int intermediatefd = -1; virCommandPtr cmd = NULL; @@ -6758,13 +6759,13 @@ qemuDomainSaveImageStartVM(virConnectPtr conn, }
/* Set the migration source and start it up. */ - ret = qemuProcessStart(conn, driver, vm, asyncJob, - "stdio", *fd, path, NULL, - VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, - VIR_QEMU_PROCESS_START_PAUSED); + restored = qemuProcessStart(conn, driver, vm, asyncJob, + "stdio", *fd, path, NULL, + VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, + VIR_QEMU_PROCESS_START_PAUSED) >= 0;
I was pretty confused when I saw this for the first time... it's too easy to miss the comparison at the end. I suggest changing the code to something similar to the following:
bool restored = false; ... if (qemuProcessStart(conn, driver, vm, asyncJob, "stdio", *fd, path, NULL, VIR_NETDEV_VPORT_PROFILE_OP_RESTORE, VIR_QEMU_PROCESS_START_PAUSED) == 0) restored = true; True. Will fix.
Jirka
participants (3)
-
Jiri Denemark -
Maxim Nestratov -
Nikolay Shirokovskiy