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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 01.02.2016 15:08, Jiri Denemark wrote:
> 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@xxxxxxxxxxxxx>
>> ---
>>  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;
True. Will fix.
> 
> Jirka
> 

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list



[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]