Re: [libvirt] [PATCH v2] qemu: return -1 on error paths in qemuDomainSaveImageStartVM

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(a)virtuozzo.com&gt; > --- > 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