5:52 a.m.
This series introduce --all to cmdDomblkinfo to show
all block devices info in one cmd.
Likes a combination of domblklist and domblkinfo.
v2:
add support --human for --all
v1.1
fix a self test
Chen Hanxiao (3):
cmdDomblkinfo: introduce helper cmdDomblkinfoPrint
cmdDomblkinfo: add --all to show all block devices info
news: add cmdDomblkinfo --all option
docs/news.xml | 8 +++
tools/virsh-domain-monitor.c | 125 +++++++++++++++++++++++++++++------
tools/virsh.pod | 5 +-
3 files changed, 118 insertions(+), 20 deletions(-)
--
2.17.1
5:52 a.m.
New subject: [libvirt] [PATCH v2 1/3] cmdDomblkinfo: introduce helper cmdDomblkinfoPrint
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Introduce helper cmdDomblkinfoPrint for printing.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
tools/virsh-domain-monitor.c | 39 ++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 15 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index 8cbb3db37c..daa86e8310 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -400,6 +400,29 @@ static const vshCmdOptDef opts_domblkinfo[] = {
{.name = NULL}
};
+static void
+cmdDomblkinfoPrint(vshControl *ctl,
+ const virDomainBlockInfo *info,
+ bool human)
+{
+ 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);
+ } 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);
+ }
+
+}
+
static bool
cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
{
@@ -420,21 +443,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
human = vshCommandOptBool(cmd, "human");
- 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);
- } 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);
- }
+ cmdDomblkinfoPrint(ctl, &info, human);
ret = true;
--
2.17.1
4:36 p.m.
New subject: [libvirt] [PATCH v2 1/3] cmdDomblkinfo: introduce helper cmdDomblkinfoPrint
On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Introduce helper cmdDomblkinfoPrint for printing.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)gmail.com>
---
tools/virsh-domain-monitor.c | 39 ++++++++++++++++++++++--------------
1 file changed, 24 insertions(+), 15 deletions(-)
Reviewed-by: John Ferlan <jferlan(a)redhat.com>
John
5:52 a.m.
New subject: [libvirt] [PATCH v2 2/3] cmdDomblkinfo: add --all to show all block devices info
From: Chen Hanxiao <chenhanxiao(a)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(a)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(-)
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;
+
+ 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;
+
+ 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;
}
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.
=item B<domblklist> I<domain> [I<--inactive>] [I<--details>]
--
2.17.1
4:41 p.m.
New subject: [libvirt] [PATCH v2 2/3] cmdDomblkinfo: add --all to show all block devices info
On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)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(a)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>]
4:58 p.m.
New subject: [libvirt] [PATCH v2 2/3] cmdDomblkinfo: add --all to show all block devices info
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
9:49 a.m.
New subject: [libvirt] [PATCH v2 2/3] cmdDomblkinfo: add --all to show all block devices info
On 06/15/2018 04:58 AM, Chen Hanxiao wrote:
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.
True, but printing a partial list and then failing is I think worse than
the "singular error" that one would get normally, e.g.:
# virsh domblkinfo $dom $networked-target
error: internal error: missing storage backend for network files
using iscsi protocol
#
The one thing that draws my attention though is using "internal error"
could make a consumer think they did something wrong or libvirt did
something wrong, I wonder if we change that to 'Operation not supported'
which may also help in the detection at [1]...
>> 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.
...[1]
For the non --all case, you don't have the XML to check whether the
"protocol" field exists which makes the error recovery tricky because
internal error can be more than just this reason. Still taking the
earlier suggestion to change to VIR_ERR_OPERATION_UNSUPPORTED may help
in the detection. Using protocol also gave that extra reason since we
then know the device isn't local... Knowing that the error domain was
VIR_FROM_STORAGE and domain is not running lent more credence to the
"assumption" one could make about the error without checking the actual
message which is something we cannot do (think internationalization).
John
[...]
5:52 a.m.
New subject: [libvirt] [PATCH v2 3/3] news: add cmdDomblkinfo --all option
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Update news for cmdDomblkinfo --all option.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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
+ </summary>
+ <description>
+ Add option --all to show all block deviecs info in one command.
+ </description>
+ </change>
</section>
<section title="Bug fixes">
</section>
--
2.17.1
4:42 p.m.
New subject: [libvirt] [PATCH v2 3/3] news: add cmdDomblkinfo --all option
On 06/11/2018 06:52 AM, Chen Hanxiao wrote:
From: Chen Hanxiao <chenhanxiao(a)gmail.com>
Update news for cmdDomblkinfo --all option.
Signed-off-by: Chen Hanxiao <chenhanxiao(a)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>
2352
days inactive
2356
days old
8 comments
2 participants
participants (2)
-
Chen Hanxiao -
John Ferlan