At 2018-06-15 05:41:48, "John Ferlan" <jferlan@xxxxxxxxxx> wrote:
>
>
>On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
>> From: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>>
[...]
>>
>
>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
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
