[libvirt] [PATCH] virsh: fixed handling of targetless disks (e.g. empty CDROM) in 'domblkinfo' cmd

[Ján Tomko]

Real estate in the commit message summary is precious, so the e.g.
part belongs in the commit message.

Also, (unless some strange usage sneaked into our XML), <source> deals
with the host-side where <target> says how the device appears in the
guest, so 'targetless' is wrong here.

On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote:
>virDomainGetBlockInfo() returns error if called on a disk with no target

s/target/source/

>(a targetless disk might be a removable media drive with no media in it,
>for instance an empty CDROM drive).
>
>So far this caused the virsh domblkinfo --all command to abort and ignore

Most of virsh commands correspond 1:1 to a libvirt API. It would be
nicer if that was the case here, since now we have to guess what
happened in the daemon, because we try to treat --all as
--all-that-are-sensible-and-supported

>any remaining (not yet displayed) disk devices.  This patch fixes it by
>ignoring virDomainGetBlockInfo() errors for CDROM and floppy drives,
>similar to how it's done for network drives.
>

I don't think that's an approach that should be copied, see below

>https://bugzilla.redhat.com/show_bug.cgi?id=1619625
>
>Signed-off-by: Pavel Mores <pmores at redhat.com>
>---
> tools/virsh-domain-monitor.c | 11 +++++++++++
> 1 file changed, 11 insertions(+)
>
>diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
>index 0e2c4191d7..0f495c1a3f 100644
>--- a/tools/virsh-domain-monitor.c
>+++ b/tools/virsh-domain-monitor.c
>@@ -473,6 +473,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>     char *cap = NULL;
>     char *alloc = NULL;
>     char *phy = NULL;
>+    char *device_type = NULL;
>     vshTablePtr table = NULL;
>
>     if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>@@ -510,6 +511,8 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>             rc = virDomainGetBlockInfo(dom, target, &info, 0);
>
>             if (rc < 0) {
>+                device_type = virXPathString("string(./@device)", ctxt);
>+
>                 /* If protocol is present that's an indication of a networked
>                  * storage device which cannot provide statistics, so generate
>                  * 0 based data and get the next disk. */

This comment is misleading, we should be capable of getting statistics
from gluster even for inactive domains. Which is probably the reason we
only look for the presence of protocol if the API failed.

>@@ -518,9 +521,16 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>                     virGetLastErrorDomain() == VIR_FROM_STORAGE) {
>                     memset(&info, 0, sizeof(info));
>                     vshResetLibvirtError();
>+                } else if (device_type != NULL &&
>+                        (STRCASEEQ(device_type, "cdrom") ||
>+                        STRCASEEQ(device_type, "floppy"))) {
>+                    memset(&info, 0, sizeof(info));
>+                    vshResetLibvirtError();

What if the cdrom is not empty and the error was something else?

To preserve that, doing a virXPathBoolean query on the source element should
say whether it makes sense to invoke the API or not. (Maybe fill in
dashes instead of zeroes as proposed in the discussion last time [0])

Alternatively, we can resurrect that patch [1] that optimistically
ignored all errors and save ourselves the trouble of having to add
another exception here later.

Jano

>                 } else {
>                     goto cleanup;
>                 }
>+
>+                VIR_FREE(device_type);
>             }
>
>             if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human))

[0] https://www.redhat.com/archives/libvir-list/2018-August/msg01332.html
[1] https://www.redhat.com/archives/libvir-list/2018-August/msg01300.html

>@@ -556,6 +566,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>     VIR_FREE(target);
>     VIR_FREE(protocol);
>     VIR_FREE(disks);
>+    VIR_FREE(device_type);
>     xmlXPathFreeContext(ctxt);
>     xmlFreeDoc(xmldoc);
>     return ret;
>-- 
>2.21.0
>
>--
>libvir-list mailing list
>libvir-list at redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 488 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20191002/52060291/attachment-0001.sig>