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

On 10/1/19 11:45 AM, Pavel Mores wrote: > virDomainGetBlockInfo() returns error if called on a disk with no target > (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 > 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. > > https://bugzilla.redhat.com/show_bug.cgi?id=1619625 > > Signed-off-by: Pavel Mores <pmores(a)redhat.com&gt; > --- > 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; I believe you need to use VIR_AUTO(char *) here. Even considering that the macro will be deprecated in the near future for glib stuff, by using VIR_AUTO here: - you'll make it easier to introduce the glib replacement, since s/VIR_AUTO/<g_lib_macro> is a trivial change; - you won't be adding more VIR_FREE() on top of existing cases that will need to be addressed in the future.

Thanks, DHB > 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. */ > @@ -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(); > } else { > goto cleanup; > } > + > + VIR_FREE(device_type); > } > if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) > @@ -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;