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(-)

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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>]
>  
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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

[...]

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list