After eca76884ea in case of error in qemuDomainSetPrivatePaths() in pretended start we jump to stop. I've changed this during review from 'cleanup' which turned out to be correct. Well, sort of. We can't call qemuProcessStop() as it decrements driver->nactive and we did not increment it. However, it calls virDomainObjRemoveTransientDef() which is basically the only function we need to call. So call that function and goto cleanup; Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index df1fa0371..9306e0e18 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -4758,8 +4758,10 @@ qemuProcessInit(virQEMUDriverPtr driver, goto cleanup; if (flags & VIR_QEMU_PROCESS_START_PRETEND) { - if (qemuDomainSetPrivatePaths(driver, vm) < 0) - goto stop; + if (qemuDomainSetPrivatePaths(driver, vm) < 0) { + virDomainObjRemoveTransientDef(vm);

+ } } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(driver, vm, false); -- 2.11.0

On 02/23/2017 05:40 PM, Marc Hartmayer wrote: > On Thu, Feb 23, 2017 at 05:15 PM +0100, Michal Privoznik <mprivozn(a)redhat.com&gt; wrote: >> After eca76884ea in case of error in qemuDomainSetPrivatePaths() >> in pretended start we jump to stop. I've changed this during >> review from 'cleanup' which turned out to be correct. Well, sort >> of. We can't call qemuProcessStop() as it decrements >> driver->nactive and we did not increment it. However, it calls >> virDomainObjRemoveTransientDef() which is basically the only >> function we need to call. So call that function and goto cleanup; >> >> Signed-off-by: Michal Privoznik <mprivozn(a)redhat.com&gt; >> --- >> src/qemu/qemu_process.c | 6 ++++-- >> 1 file changed, 4 insertions(+), 2 deletions(-) >> >> diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c >> index df1fa0371..9306e0e18 100644 >> --- a/src/qemu/qemu_process.c >> +++ b/src/qemu/qemu_process.c >> @@ -4758,8 +4758,10 @@ qemuProcessInit(virQEMUDriverPtr driver, >> goto cleanup; >> >> if (flags & VIR_QEMU_PROCESS_START_PRETEND) { >> - if (qemuDomainSetPrivatePaths(driver, vm) < 0) >> - goto stop; >> + if (qemuDomainSetPrivatePaths(driver, vm) < 0) { >> + virDomainObjRemoveTransientDef(vm); > > I'm not sure if this is needed (I think every caller of qemuProcessInit > will unref/free @vm/the transient domain in case of returning -1) but at > least it's not wrong and probably more safe :) The idea that we try to honour is to whomever allocated the memory, should be also the one who frees it. I'm not saying that we do it all the time at all places. In fact I'd say in some areas of the code we are far from that. But a) we can blame historic reasons, b) sometimes it's not as easy to follow the idea as 1 2 3. In general, following this rule means that if a function fails, it hasn't left any side effects on the object and basically was NO-OP. But here, none of the above reasons stands. So I think we should free it here regardless of what caller does afterwards. Michal