[libvirt] [PATCH v2 2/3] cmdDomblkinfo: add --all to show all block devices info

[John Ferlan]

On 06/15/2018 04:58 AM, Chen Hanxiao wrote:
> 
> 
> At 2018-06-15 05:41:48, "John Ferlan" <jferlan at redhat.com> wrote:
>>
>>
>> On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
>>> From: Chen Hanxiao <chenhanxiao at gmail.com>
>>>
> [...]
>>>
>>
>> Do you have networked disks in your domain configs? For a non running
>> guest, t; otherwise, you would have noted:
>>
>> # virsh domblkinfo $dom --all
>> Target     Capacity        Allocation      Physical
>> -----------------------------------------------------
>> vda        10737418240     2086727680      10737418240
>> error: internal error: missing storage backend for network files using
>> iscsi protocol
>>
> 
> Yes, I tested this cases.
> This issue already existed for the original domblkinfo, so I didn't change this.
> Maybe we should fix it in another patch.
> 

True, but printing a partial list and then failing is I think worse than
the "singular error" that one would get normally, e.g.:

   # virsh domblkinfo $dom $networked-target
   error: internal error: missing storage backend for network files
using iscsi protocol
   #

The one thing that draws my attention though is using "internal error"
could make a consumer think they did something wrong or libvirt did
something wrong, I wonder if we change that to 'Operation not supported'
which may also help in the detection at [1]...

>>> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
>>> index daa86e8310..22c0b740c6 100644
> ...
>>> +        if (device) {
>>> +            if (virAsprintf(&cap, "%.3lf %s", val_cap, unit_cap) < 0)
>>> +                goto cleanup;
>>> +
>>> +            if (virAsprintf(&alloc, "%.3lf %s", val_alloc, unit_alloc) < 0)
>>> +                goto cleanup;
>>> +
>>> +            if (virAsprintf(&phy, "%.3lf %s", val_phy, unit_phy) < 0)
>>> +                goto cleanup;
>>
>> it would be fine I think to do:
>>
>>    if (virAsprintf(&cap, "%.3lf %s", val_cap, unit_cap) < 0 ||
>>        virAsprintf(&alloc, "%.3lf %s", val_alloc, unit_alloc) < 0 ||
>>        virAsprintf(&phy, "%.3lf %s", val_phy, unit_phy) < 0)
>>        goto cleanup;
>>
>> But that's not required.
>>
> 
> Looks much better, will be changed in the next series.
> 
>>> +
>>> +            vshPrint(ctl, "%-10s %-15s %-15s %-15s\n",
> [...]
> 
>>> +            ctxt->node = disks[i];
>>> +            target = virXPathString("string(./target/@dev)", ctxt);
>>> +
>>> +            if (virDomainGetBlockInfo(dom, target, &info, 0) < 0)
>>> +                goto cleanup;
>>
>> If the domain is not running, then it's possible to return an error for
>> a networked disk (e.g. <source protocol='network' ...>)... This is
>> because qemuDomainGetBlockInfo calls qemuStorageLimitsRefresh which
>> calls qemuDomainStorageOpenStat and for non local storage the
>> virStorageFileInitAs will eventually fail in virStorageFileBackendForType.
>>
>> A couple of options come to mind...
>>
>> ... let the failure occur as is, so be it...
>>
>> ... check the last error message for code == VIR_ERR_INTERNAL_ERROR and
>> domain == VIR_FROM_STORAGE and we have a source protocol from an
>> inactive domain, then assume it's a we cannot get there from here.
>>
>> ... Other options?
>>
>> If we fail virDomainGetBlockInfo we could still display values as long
>> as there's an appropriately placed  memset(&info, 0, sizeof(info)). That
>> way we display only the name and 0's for everything else. Not optimal,
>> but easily described in the man page that an inactive guest, using
>> network protocol for storage may not be able to get the size values and
>> virsh will display as 0's instead... or get more creative and display
>> "-" for each size column.
> 
> I prefer this solutions.
> Also, I think domblkinfo DOM DEVICE should follow this if it's a network disk.
> 

...[1]

For the non --all case, you don't have the XML to check whether the
"protocol" field exists which makes the error recovery tricky because
internal error can be more than just this reason. Still taking the
earlier suggestion to change to VIR_ERR_OPERATION_UNSUPPORTED may help
in the detection. Using protocol also gave that extra reason since we
then know the device isn't local... Knowing that the error domain was
VIR_FROM_STORAGE and domain is not running lent more credence to the
"assumption" one could make about the error without checking the actual
message which is something we cannot do (think internationalization).


John

[...]