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

[Pavel Mores]

On Wed, Oct 02, 2019 at 08:34:10AM -0300, Daniel Henrique Barboza wrote:
> 
> 
> 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 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;
> 
> 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.

Was that the consensus from the migration debate?  If so I'm happy to fix my
patch.

	pvl

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