1. Patchew
  2. Libvirt
  3. View series

[libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error

Michal Privoznik posted 1 patch 7 years, 1 month ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/e802dcadf40fae4398018457282d39e8d923ff1c.1487866516.git.mprivozn@redhat.com
src/qemu/qemu_process.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
[libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error
Posted by Michal Privoznik 7 years, 1 month ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error
Posted by Marc Hartmayer 7 years, 1 month ago 
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error
Posted by Michal Privoznik 7 years, 1 month ago 
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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error
Posted by Marc Hartmayer 7 years, 1 month ago 
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


--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] qemuProcessInit: Jump onto correct label in case of error
Posted by Ján Tomko 7 years, 1 month ago 
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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.