[libvirt] [PATCH v2] virsh: Don't break loop of domblkinfo for disks

https://bugzilla.redhat.com/show_bug.cgi?id=1619625 --all option is added to cmdDomblkinfo since commit 62c39193 allowing to show all block devices info. Reset error when empty source in case error breaks the loop of domblkinfo for disks. Signed-off-by: Han Han <hhan@redhat.com> --- tools/virsh-domain-monitor.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b4f9739b..576610f005 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) int ndisks; size_t i; xmlNodePtr *disks = NULL; + char *source = NULL; char *target = NULL; char *protocol = NULL; @@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; + source = virXPathString("string(./source)", ctxt); 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 && + /* For the case of empty cdrom, networked disk which cannot + * provide statistics, generate 0 based data and get the next + * disk. + */ + if (!source && protocol && !active && virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR && virGetLastErrorDomain() == VIR_FROM_STORAGE) { memset(&info, 0, sizeof(info)); @@ -526,6 +529,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) cmdDomblkinfoPrint(ctl, &info, target, human, false); + VIR_FREE(source); VIR_FREE(target); VIR_FREE(protocol); } @@ -540,6 +544,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) cleanup: virshDomainFree(dom); + VIR_FREE(source); VIR_FREE(target); VIR_FREE(protocol); VIR_FREE(disks); -- 2.18.0

On 08/22/2018 11:46 AM, Han Han wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
--all option is added to cmdDomblkinfo since commit 62c39193 allowing to show all block devices info. Reset error when empty source in case error breaks the loop of domblkinfo for disks.
Signed-off-by: Han Han <hhan@redhat.com> --- tools/virsh-domain-monitor.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b4f9739b..576610f005 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) int ndisks; size_t i; xmlNodePtr *disks = NULL; + char *source = NULL; char *target = NULL; char *protocol = NULL;
@@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; + source = virXPathString("string(./source)", ctxt); 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 && + /* For the case of empty cdrom, networked disk which cannot + * provide statistics, generate 0 based data and get the next + * disk. + */ + if (!source && protocol && !active &&
Shouldn't this look like: if ((!source || protocol) && !active && I guess we want this to be: - either source is missing, or - disk is a network one Michal

On 09/21/2018 08:32 AM, Michal Privoznik wrote:
On 08/22/2018 11:46 AM, Han Han wrote:
https://bugzilla.redhat.com/show_bug.cgi?id=1619625
--all option is added to cmdDomblkinfo since commit 62c39193 allowing to show all block devices info. Reset error when empty source in case error breaks the loop of domblkinfo for disks.
Signed-off-by: Han Han <hhan@redhat.com> --- tools/virsh-domain-monitor.c | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c index b9b4f9739b..576610f005 100644 --- a/tools/virsh-domain-monitor.c +++ b/tools/virsh-domain-monitor.c @@ -475,6 +475,7 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd) int ndisks; size_t i; xmlNodePtr *disks = NULL; + char *source = NULL; char *target = NULL; char *protocol = NULL;
@@ -505,16 +506,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
for (i = 0; i < ndisks; i++) { ctxt->node = disks[i]; + source = virXPathString("string(./source)", ctxt); 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 && + /* For the case of empty cdrom, networked disk which cannot + * provide statistics, generate 0 based data and get the next + * disk. + */ + if (!source && protocol && !active &&
Shouldn't this look like:
if ((!source || protocol) && !active &&
I guess we want this to be:
- either source is missing, or - disk is a network one
Michal
There's a v3 already: https://www.redhat.com/archives/libvir-list/2018-August/msg01418.html John
participants (3)
-
Han Han
-
John Ferlan
-
Michal Privoznik