[libvirt] [PATCHv2 07/14] Fix vmdef usage while in monitor in BlockStat* APIs

[Ján Tomko]

On 01/12/2015 11:30 PM, John Ferlan wrote:
> <No commit message>?
> 
> Seems we're fixing two issues here
> 
> #1. The ExitMonitor issue
> 
> #2. Somehow the disk->info.alias becomes NULL and thus we're making a
> local copy to avoid that.
> 
> On 01/07/2015 10:42 AM, Ján Tomko wrote:
>> ---
>>  src/qemu/qemu_driver.c | 26 +++++++++++++++++++-------
>>  1 file changed, 19 insertions(+), 7 deletions(-)
>>

>> @@ -10231,6 +10238,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>>                              disk->dst);
>>               goto endjob;
>>          }
>> +        if (VIR_STRDUP(diskAlias, disk->info.alias) < 0)
>> +            goto endjob;
> 
> so diskAlias is only set if *nparams != 0, but...
> 
>>      }
>>  
>>      priv = vm->privateData;
>> @@ -10241,12 +10250,12 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>>      ret = qemuMonitorGetBlockStatsParamsNumber(priv->mon, nparams);
>>  
>>      if (tmp == 0 || ret < 0) {
>> -        qemuDomainObjExitMonitor(driver, vm);
>> +        ignore_value(qemuDomainObjExitMonitor(driver, vm));
> 
> Can tmp == 0 and ret == 0?  Thus when go to endjob and eventually exit
> we don't a failure?  Does it matter? I suppose for consistency it does.

Just a few lines above *nparams gets assigned to tmp. So if no block stats
were requested, there's nothing to return back to the user and no need to fail.

> 
>>          goto endjob;
>>      }
>>  
>>      ret = qemuMonitorGetBlockStatsInfo(priv->mon,
>> -                                       disk->info.alias,
>> +                                       diskAlias,
> 
> ...used here regardless of *nparams...  Can we get here with diskAlias
> == NULL?
> 

No, if tmp == 0 we jump to endjob.

>>                                         &rd_req,
>>                                         &rd_bytes,
>>                                         &rd_total_times,
>> @@ -10257,7 +10266,8 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>>                                         &flush_total_times,
>>                                         &errs);
>>  
>> -    qemuDomainObjExitMonitor(driver, vm);
>> +    if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> +        ret = -1;
>>  
>>      if (ret < 0)
>>          goto endjob;
>> @@ -10342,6 +10352,7 @@ qemuDomainBlockStatsFlags(virDomainPtr dom,
>>      qemuDomainObjEndJob(driver, vm);
>>  
>>   cleanup:
>> +    VIR_FREE(diskAlias);
>>      qemuDomObjEndAPI(&vm);
>>      return ret;
>>  }
>> @@ -16853,7 +16864,8 @@ qemuDomainSetBlockIoTune(virDomainPtr dom,
>>          qemuDomainObjEnterMonitor(driver, vm);
>>          ret = qemuMonitorSetBlockIoThrottle(priv->mon, device,
>>                                              &info, supportMaxOptions);
>> -        qemuDomainObjExitMonitor(driver, vm);
>> +        if (qemuDomainObjExitMonitor(driver, vm) < 0)
>> +            goto endjob;
> 
> Potentially leaving ret = 0 from the IoThrottle call... Should just set
> ret = -1 (liek done in qemuDomainBlockStatsFlags above)
> 

I've fixed that and wrote a commit message.

Jan

> 
> ACK with a commit message and the issues resolved.
> 
> John
> 
>>          if (ret < 0)
>>              goto endjob;
>>          vm->def->disks[idx]->blkdeviotune = info;
>>
> 
> --
> libvir-list mailing list
> libvir-list at redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
> 


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: OpenPGP digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150113/4d5662d7/attachment-0001.sig>