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

[Chen Hanxiao]

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.

>> 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.

>
>
>> +
>> +            cmdDomblkinfoPrint(ctl, &info, target, human, false);
>> +
>> +            VIR_FREE(target);
>> +        }
>> +    } else {
>> +        if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
>> +            goto cleanup;
>> +
>> +        cmdDomblkinfoPrint(ctl, &info, NULL, human, false);
>> +    }
>>  
>>      ret = true;
>>  
>>   cleanup:
>>      virshDomainFree(dom);
>> +    VIR_FREE(target);
>> +    VIR_FREE(disks);
>> +    xmlXPathFreeContext(ctxt);
>> +    xmlFreeDoc(xmldoc);
>>      return ret;
>>  }
>
>Taking the handle the error path option and a bit of poetic license,
>here's some diffs...

Will do in v3.
>
>     char *target = NULL;
>+    char *protocol = NULL;
>...
>     if (all) {
>+        bool active = virDomainIsActive(dom) == 1;
>+        int rc;
>+
>...
>         for (i = 0; i < ndisks; i++) {
>             ctxt->node = disks[i];
>+            protocol = virXPathString("string(./source/@protocol)", ctxt);
>             target = virXPathString("string(./target/@dev)", ctxt);
>...
>-            if (virDomainGetBlockInfo(dom, target, &info, 0) < 0)
>-                goto cleanup;
>+            rc = virDomainGetBlockInfo(dom, target, &info, 0);
>+
>+            if (rc < 0) {
>+                if (protocol && !active &&
>+                    virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&
>+                    virGetLastErrorDomain() == VIR_FROM_STORAGE)
>+                    memset(&info, 0, sizeof(info));
>+                else
>+                    goto cleanup;
>+            }
>...
>+    VIR_FREE(protocol);
>     VIR_FREE(target);
>
>>  
>> diff --git a/tools/virsh.pod b/tools/virsh.pod
>> index 3f3314a87e..e273011037 100644
>> --- a/tools/virsh.pod
>> +++ b/tools/virsh.pod
>> @@ -949,13 +949,16 @@ B<domstate> command says that a domain was paused due to I/O error.
>>  The B<domblkerror> command lists all block devices in error state and
>>  the error seen on each of them.
>>  
>> -=item B<domblkinfo> I<domain> I<block-device> [I<--human>]
>> +=item B<domblkinfo> I<domain> [I<block-device> I<--all>] [I<--human>]
>>  
>>  Get block device size info for a domain.  A I<block-device> corresponds
>>  to a unique target name (<target dev='name'/>) or source file (<source
>>  file='name'/>) for one of the disk devices attached to I<domain> (see
>>  also B<domblklist> for listing these names). If I<--human> is set, the
>>  output will have a human readable output.
>> +If I<--all> is set, the output will be a table showing all block devices
>> +size info associated with I<domain>.
>> +The I<--all> option takes precedence of the others.
>
>Depending on how to handle the inactive networked storage, the above
>changes slightly.
>
>Maybe someone else will have some thoughts on this as well - so let's
>give it a couple of days to get that kind of feedback before deciding
>what to do and posting another series. Of course once anything is pushed
>we may find a differing opinion ;-)
>

Agree. I'll post v3 next Monday.

Regards,
- Chen