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

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