4:58 p.m.
At 2018-06-15 05:41:48, "John Ferlan" <jferlan(a)redhat.com> wrote:
On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao(a)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