On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
> From: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
>
> This patch introduces --all to show all block devices info
> of guests like:
>
> virsh # domblkinfo --all
> Target Capacity Allocation Physical
> ---------------------------------------------------
> hda 42949672960 9878110208 9878110208
> vda 10737418240 10736439296 10737418240
>
> # domblkinfo --all --human
> Target Capacity Allocation Physical
> ---------------------------------------------------
> hda 40.000 GiB 9.200 GiB 9.200 GiB
> vda 10.000 GiB 9.999 GiB 10.000 GiB
>
> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxx>
> ---
> v2:
> add support --human to --all
> v1.1:
> fix self-test
>
> tools/virsh-domain-monitor.c | 118 +++++++++++++++++++++++++++++------
> tools/virsh.pod | 5 +-
> 2 files changed, 102 insertions(+), 21 deletions(-)
>
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
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index daa86e8310..22c0b740c6 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -388,8 +388,7 @@ static const vshCmdInfo info_domblkinfo[] = {
> static const vshCmdOptDef opts_domblkinfo[] = {
> VIRSH_COMMON_OPT_DOMAIN_FULL(0),
> {.name = "device",
> - .type = VSH_OT_DATA,
> - .flags = VSH_OFLAG_REQ,
> + .type = VSH_OT_STRING,
> .completer = virshDomainDiskTargetCompleter,
> .help = N_("block device")
> },
> @@ -397,30 +396,71 @@ static const vshCmdOptDef opts_domblkinfo[] = {
> .type = VSH_OT_BOOL,
> .help = N_("Human readable output")
> },
> + {.name = "all",
> + .type = VSH_OT_BOOL,
> + .help = N_("display all block devices info")
> + },
> {.name = NULL}
> };
>
> static void
> cmdDomblkinfoPrint(vshControl *ctl,
> const virDomainBlockInfo *info,
> - bool human)
> + const char *device,
> + bool human, bool title)
> {
> + char *cap = NULL, *alloc = NULL, *phy = NULL;
> +
> + if (title) {
> + vshPrintExtra(ctl, "%-10s %-15s %-15s %-15s\n", _("Target"),
> + _("Capacity"), _("Allocation"), _("Physical"));
> + vshPrintExtra(ctl, "-----------------------------"
> + "------------------------\n");
> + return;
> + }
> +
> if (!human) {
> - vshPrint(ctl, "%-15s %llu\n", _("Capacity:"), info->capacity);
> - vshPrint(ctl, "%-15s %llu\n", _("Allocation:"), info->allocation);
> - vshPrint(ctl, "%-15s %llu\n", _("Physical:"), info->physical);
> + if (device) {
> + vshPrint(ctl, "%-10s %-15llu %-15llu %-15llu\n", device,
> + info->capacity, info->allocation, info->physical);
> + } else {
> + vshPrint(ctl, "%-15s %llu\n", _("Capacity:"), info->capacity);
> + vshPrint(ctl, "%-15s %llu\n", _("Allocation:"), info->allocation);
> + vshPrint(ctl, "%-15s %llu\n", _("Physical:"), info->physical);
> + }
> } else {
> - double val;
> - const char *unit;
> -
> - val = vshPrettyCapacity(info->capacity, &unit);
> - vshPrint(ctl, "%-15s %-.3lf %s\n", _("Capacity:"), val, unit);
> - val = vshPrettyCapacity(info->allocation, &unit);
> - vshPrint(ctl, "%-15s %-.3lf %s\n", _("Allocation:"), val, unit);
> - val = vshPrettyCapacity(info->physical, &unit);
> - vshPrint(ctl, "%-15s %-.3lf %s\n", _("Physical:"), val, unit);
> + double val_cap, val_alloc, val_phy;
> + const char *unit_cap, *unit_alloc, *unit_phy;
> +
> + val_cap = vshPrettyCapacity(info->capacity, &unit_cap);
> + val_alloc = vshPrettyCapacity(info->allocation, &unit_alloc);
> + val_phy = vshPrettyCapacity(info->physical, &unit_phy);
> + 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.
> +
> + vshPrint(ctl, "%-10s %-15s %-15s %-15s\n",
> + device, cap, alloc, phy);
> + } else {
> + vshPrint(ctl, "%-15s %-.3lf %s\n", _("Capacity:"),
> + val_cap, unit_cap);
> + vshPrint(ctl, "%-15s %-.3lf %s\n", _("Allocation:"),
> + val_alloc, unit_alloc);
> + vshPrint(ctl, "%-15s %-.3lf %s\n", _("Physical:"),
> + val_phy, unit_phy);
> + }
> }
>
> + cleanup:
> + VIR_FREE(cap);
> + VIR_FREE(alloc);
> + VIR_FREE(phy);
> }
>
> static bool
> @@ -430,25 +470,63 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> virDomainPtr dom;
> bool ret = false;
> bool human = false;
> + bool all = false;
> const char *device = NULL;
> + xmlDocPtr xmldoc = NULL;
> + xmlXPathContextPtr ctxt = NULL;
> + int ndisks;
> + size_t i;
> + xmlNodePtr *disks = NULL;
> + char *target = NULL>
> if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> return false;
>
> - if (vshCommandOptStringReq(ctl, cmd, "device", &device) < 0)
> - goto cleanup;
> -
> - if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
> + all = vshCommandOptBool(cmd, "all");
> + if (!all && vshCommandOptStringQuiet(ctl, cmd, "device", &device) <= 0) {
> + vshError(ctl, "command 'domblkinfo' requires <device> option");
> goto cleanup;
> + }
>
> human = vshCommandOptBool(cmd, "human");
>
> - cmdDomblkinfoPrint(ctl, &info, human);
> + if (all) {
> + if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0)
> + goto cleanup;
> +
> + ndisks = virXPathNodeSet("./devices/disk", ctxt, &disks);
> + if (ndisks < 0)
> + goto cleanup;
> +
> + /* print the title */
> + cmdDomblkinfoPrint(ctl, NULL, NULL, false, true);
> +
> + for (i = 0; i < ndisks; i++) {
> + 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.
> +
> + 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...
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 ;-)
John
>
> =item B<domblklist> I<domain> [I<--inactive>] [I<--details>]
>
>
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list
