On Wed, Oct 02, 2019 at 01:15:12PM +0200, Ján Tomko wrote: > Real estate in the commit message summary is precious, so the e.g. > part belongs in the commit message. Right. > Also, (unless some strange usage sneaked into our XML), <source> deals > with the host-side where <target> says how the device appears in the > guest, so 'targetless' is wrong here. > > On Tue, Oct 01, 2019 at 04:45:02PM +0200, Pavel Mores wrote: > > virDomainGetBlockInfo() returns error if called on a disk with no target > > s/target/source/ Yes, sorry for the mixup. > > (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 > > Most of virsh commands correspond 1:1 to a libvirt API. It would be > nicer if that was the case here, since now we have to guess what > happened in the daemon, because we try to treat --all as > --all-that-are-sensible-and-supported Could you please elaborate? I don't have enough context yet, I'm not sure what in particular the above means for this patch. > > 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. > > > > I don't think that's an approach that should be copied, see below > > > https://bugzilla.redhat.com/show_bug.cgi?id=1619625 > > > > Signed-off-by: Pavel Mores <pmores@xxxxxxxxxx> > > --- > > 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; > > 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. */ > > This comment is misleading, we should be capable of getting statistics > from gluster even for inactive domains. Which is probably the reason we > only look for the presence of protocol if the API failed. > > > @@ -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(); > > What if the cdrom is not empty and the error was something else? Whatever error occurs, you get dashes instead of actual numbers. > To preserve that, doing a virXPathBoolean query on the source element should > say whether it makes sense to invoke the API or not. (Maybe fill in > dashes instead of zeroes as proposed in the discussion last time [0]) Don't know if that's what you're talking about but I already get a dashed output. For instance: virsh # domblkinfo --all archlinux Target Capacity Allocation Physical -------------------------------------------------- vda 16106127360 6850265088 16108814336 sda - - - fda - - - If querying <source> beforehand is the right way I'm happy to fix the patch. > Alternatively, we can resurrect that patch [1] that optimistically > ignored all errors and save ourselves the trouble of having to add > another exception here later. > > Jano > > > } else { > > goto cleanup; > > } > > + > > + VIR_FREE(device_type); > > } > > > > if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) > > [0] https://www.redhat.com/archives/libvir-list/2018-August/msg01332.html > [1] https://www.redhat.com/archives/libvir-list/2018-August/msg01300.html > > > @@ -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; > > -- > > 2.21.0 > > > > -- > > libvir-list mailing list > > libvir-list@xxxxxxxxxx > > https://www.redhat.com/mailman/listinfo/libvir-list -- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
