[libvirt] [PATCH v2 0/1] virsh: fixed handling of sourceless disks in 'domblkinfo' cmd
Integrating feedback to the original version. This changes the approach to fixing the problem - previously, we called virDomainGetBlockInfo() and if it failed, we mitigated the failure for CDROMs and floppies specifically. Now we first check for existence of a <source> element in the corresponding XML and if we find none, we avoid calling virDomainGetBlockInfo() altogether as we know it's bound to fail in that case. The benefit is that whereas the previous fix swallowed all errors concerning CDROMs and floppies - not just missing <source> - this one only handles missing <source> specifically and doesn't mask other problems that might come up. The patch itself is fairly simple, unfortunately some noise is caused by additional indentation of code related to the virDomainGetBlockInfo() call. That code should however be unchanged (apart from reformating a comment to keep line lengths under 80 chars). Pavel Mores (1): fixed handling of sourceless disks in 'domblkinfo' cmd tools/virsh-domain-monitor.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) -- 2.21.0
virDomainGetBlockInfo() returns error if called on a disk with no source (a sourceless disk might be a removable media drive with no media in it, for instance an empty CDROM or floppy drive). So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes the problem by first checking for existence of a <source> element in the corresponding XML. If none is found, we avoid calling virDomainGetBlockInfo() altogether as we know it's bound to fail in that case. https://bugzilla.redhat.com/show_bug.cgi?id=1619625 Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index 0e2c4191d7..b69283b2da 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -507,20 +507,27 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) protocol = virXPathString("string(./source/@protocol)", ctxt); target = virXPathString("string(./target/@dev)", ctxt); - rc = virDomainGetBlockInfo(dom, target, &info, 0); - - if (rc < 0) { - /* 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. */ - if (protocol && !active && - virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR && - virGetLastErrorDomain() == VIR_FROM_STORAGE) { - memset(&info, 0, sizeof(info)); - vshResetLibvirtError(); - } else { - goto cleanup; + if (virXPathBoolean("boolean(./source)", ctxt) == 1) { + + rc = virDomainGetBlockInfo(dom, target, &info, 0); + + if (rc < 0) { + /* 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. */ + if (protocol && !active && + virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR && + virGetLastErrorDomain() == VIR_FROM_STORAGE) { + memset(&info, 0, sizeof(info)); + vshResetLibvirtError(); + } else { + goto cleanup; + } } + } else { + /* if we don't call virDomainGetBlockInfo() who clears 'info' + * we have to do it manually */ + memset(&info, 0, sizeof(info)); } if (!cmdDomblkinfoGet(ctl, &info, &cap, &alloc, &phy, human)) -- 2.21.0
On Fri, Oct 04, 2019 at 01:35:09PM +0200, Pavel Mores wrote:
virDomainGetBlockInfo() returns error if called on a disk with no source (a sourceless disk might be a removable media drive with no media in it, for instance an empty CDROM or floppy drive).
So far this caused the virsh domblkinfo --all command to abort and ignore any remaining (not yet displayed) disk devices. This patch fixes the problem by first checking for existence of a <source> element in the corresponding XML. If none is found, we avoid calling virDomainGetBlockInfo() altogether as we know it's bound to fail in that case.
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
Signed-off-by: Pavel Mores <pmores@redhat.com> --- tools/virsh-domain-monitor.c | 33 ++++++++++++++++++++------------- 1 file changed, 20 insertions(+), 13 deletions(-)
Reviewed-by: Ján Tomko <jtomko@redhat.com> Jano
participants (2)
-
Ján Tomko -
Pavel Mores