On 06/11/2018 06:52 AM, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

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@gmail.com> --- 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>]

On 06/11/2018 06:52 AM, Chen Hanxiao wrote:

From: Chen Hanxiao <chenhanxiao@gmail.com>

Update news for cmdDomblkinfo --all option.

Signed-off-by: Chen Hanxiao <chenhanxiao@gmail.com> --- docs/news.xml | 8 ++++++++ 1 file changed, 8 insertions(+)

diff --git a/docs/news.xml b/docs/news.xml index e0cf381f33..c07db04809 100644 --- a/docs/news.xml +++ b/docs/news.xml @@ -193,6 +193,14 @@ secret-event, pool-event and nodedev-event) </description> </change> + <change> + <summary> + cmdDomblkinfo: introduce --all to show all block devices info

virsh: Add --all to domblkinfo command

+ </summary> + <description> + Add option --all to show all block deviecs info in one command.

Alter the <code>domblkinfo</code> command to add the option --all in order to display the size details of each domain block device from one command in a output table. John

+ </description> + </change> </section> <section title="Bug fixes"> </section>