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

[Pavel Mores]

On Thu, Oct 03, 2019 at 06:25:40PM +0200, Ján Tomko wrote:
> On Thu, Oct 03, 2019 at 02:45:36PM +0200, Pavel Mores wrote:
> > On Thu, Oct 03, 2019 at 01:09:55PM +0200, Pavel Mores wrote:
> > > On Thu, Oct 03, 2019 at 11:54:44AM +0200, Ján Tomko wrote:
> > > > On Wed, Oct 02, 2019 at 04:21:52PM +0200, Pavel Mores wrote:
> > > > > 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.
> > > >
> > > > IIUC the consensus was that we will switch to g_auto eventually,
> > > > but it's a simple string replace that will be dealt with by whoever
> > > > pushes the patch.
> > > >
> > > > Also, if you go with any of the two approaches I suggested (ignoring all
> > > > errors or using the XPathBoolean), you don't need to introduce a new
> > > > allocated variable
> > > 
> > > Cool, so if there aren't any objections, I'll implement the XPathBoolean
> > > variant and submit v2.
> > 
> > As it turns out, the thing with the virDomainGetBlockInfo() call is that even
> > if it fails, it does initialise the virDomainBlockInfo structure enough to be
> > displayed in the table (even if just as a target name and dashes).
> 
> The https://libvirt.org/html/libvirt-libvirt-domain.html#virDomainBlockInfo
> only contains numbers, not the target name.
> 
> The target name is output by the 'vshTableRowAppend' command.
> 
> > 
> > It follows apparently that if we go the virXPathBoolean() way and skip the
> > virDomainGetBlockInfo() call, we can't show the sourceless device at all, we
> > can't even indicate that it exists.
> > 
> > Is that acceptable?  Or is there another way to fill in virDomainBlockInfo?
> > 
> 
> cmdDomblkinfoGet happily handles a virDomainBlockInfo with all zeroes,
> so as long as info is reset to zeroes on every loop iteration, it's okay
> to execute the rest of the loop even if we did not call virDomainGetBlockInfo

You're right, the problem wasn't actually caused by the virDomainBlockInfo
instance.

I'll post a v2 shortly.

Thanks!

	pvl