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

[Marc Hartmayer]

On Thu, Feb 23, 2017 at 05:46 PM +0100, Michal Privoznik <mprivozn at 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 at 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 at 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