[libvirt] [PATCH v2] virsh: Don't break loop of domblkinfo for disks

[John Ferlan]

On 09/21/2018 08:32 AM, Michal Privoznik wrote:
> On 08/22/2018 11:46 AM, Han Han wrote:
>> https://bugzilla.redhat.com/show_bug.cgi?id=1619625
>>
>> --all option is added to cmdDomblkinfo since commit 62c39193 allowing to
>> show all block devices info. Reset error when empty source in case error
>> breaks the loop of domblkinfo for disks.
>>
>> Signed-off-by: Han Han <hhan at redhat.com>
>> ---
>>  tools/virsh-domain-monitor.c | 13 +++++++++----
>>  1 file changed, 9 insertions(+), 4 deletions(-)
>>
>> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
>> index b9b4f9739b..576610f005 100644
>> --- a/tools/virsh-domain-monitor.c
>> +++ b/tools/virsh-domain-monitor.c
>> @@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>>      int ndisks;
>>      size_t i;
>>      xmlNodePtr *disks = NULL;
>> +    char *source = NULL;
>>      char *target = NULL;
>>      char *protocol = NULL;
>>  
>> @@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>>  
>>          for (i = 0; i < ndisks; i++) {
>>              ctxt->node = disks[i];
>> +            source = virXPathString("string(./source)", ctxt);
>>              protocol = virXPathString("string(./source/@protocol)", ctxt);
>>              target = virXPathString("string(./target/@dev)", ctxt);
>>  
>>              rc = virDomainGetBlockInfo(dom, target, &info, 0);
>>  
>>              if (rc < 0) {
>> -                /* If protocol is present that's an indication of a networked
>> -                 * storage device which cannot provide statistics, so generate
>> -                 * 0 based data and get the next disk. */
>> -                if (protocol && !active &&
>> +                /* For the case of empty cdrom, networked disk which cannot
>> +                 * provide statistics, generate 0 based data and get the next
>> +                 * disk.
>> +                 */
>> +                if (!source && protocol && !active &&
> 
> Shouldn't this look like:
> 
> if ((!source || protocol) && !active &&
> 
> I guess we want this to be:
> 
>   - either source is missing, or
>   - disk is a network one
> 
> Michal
> 

There's a v3 already:

https://www.redhat.com/archives/libvir-list/2018-August/msg01418.html

John