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

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