[libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error
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@redhat.com> --- 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); + goto cleanup; + } } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(driver, vm, false); -- 2.11.0
On Thu, Feb 23, 2017 at 05:15 PM +0100, Michal Privoznik <mprivozn@redhat.com> 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@redhat.com> --- 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 :)
+ } } else { vm->def->id = qemuDriverAllocateID(driver); qemuDomainSetFakeReboot(driver, vm, false); -- 2.11.0
Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On 02/23/2017 05:40 PM, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 05:15 PM +0100, Michal Privoznik <mprivozn@redhat.com> 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@redhat.com> --- 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
On Thu, Feb 23, 2017 at 05:46 PM +0100, Michal Privoznik <mprivozn@redhat.com> wrote:
On 02/23/2017 05:40 PM, Marc Hartmayer wrote:
On Thu, Feb 23, 2017 at 05:15 PM +0100, Michal Privoznik <mprivozn@redhat.com> 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@redhat.com> --- 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
Thanks for the explanation :) -- Beste Grüße / Kind regards Marc Hartmayer IBM Deutschland Research & Development GmbH Vorsitzende des Aufsichtsrats: Martina Koederitz Geschäftsführung: Dirk Wittkopp Sitz der Gesellschaft: Böblingen Registergericht: Amtsgericht Stuttgart, HRB 243294
On Thu, Feb 23, 2017 at 05:15:47PM +0100, Michal Privoznik 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@redhat.com> --- src/qemu/qemu_process.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)
ACK Jan
participants (3)
-
Ján Tomko -
Marc Hartmayer -
Michal Privoznik