Fix logical error in v2: https://www.redhat.com/archives/libvir-list/2018-August/msg01358.html

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 | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..ecb98d4128 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,18 +506,20 @@ 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) {
+                    virGetLastErrorDomain() == VIR_FROM_STORAGE)) {
                     memset(&info, 0, sizeof(info));
                     vshResetLibvirtError();
                 } else {
@@ -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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

On 08/23/2018 03:27 AM, Han Han wrote:
> Fix logical error in v2: https://www.redhat.com/archives/libvir-list/2018-August/msg01358.html
> 

^^^ This would go under the --- below so it's not part of the history.

> 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 | 15 ++++++++++-----
>  1 file changed, 10 insertions(+), 5 deletions(-)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index b9b4f9739b..ecb98d4128 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,18 +506,20 @@ 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);

I *think* what Peter was inferring from his v1 review is that if
!source, then don't make that virDomainGetBlockInfo call since it's just
going to fail since an empty cdrom will have no <source>. Thus avoiding
the error from qemuDomainGetBlockInfo:

    if (virStorageSourceIsEmpty(disk->src)) {
        virReportError(VIR_ERR_INVALID_ARG,
                       _("disk '%s' does not currently have a source
assigned"),
                       path);
        goto endjob;
    }

How about:

    /* Skip GetBlockInfo if no <source>, it's an indication of an
     * empty CDROM device. */
    rc = 0;
    if (source)
        rc = virDomainGetBlockInfo(dom, target, &info, 0);
    else
        memset(&info, 0, sizeof(info));

This should be enough to satisfy the bz issue and not change other code
as well.

>  
>              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 &&

Don't change anything here. When you have a protocol and it's !active
and you have rc < 0, we more than likely have that specific existing
error scenario which would come from qemuStorageLimitsRefresh failure
through virStorageFileInitAs.

John

>                      virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&
> -                    virGetLastErrorDomain() == VIR_FROM_STORAGE) {
> +                    virGetLastErrorDomain() == VIR_FROM_STORAGE)) {
>                      memset(&info, 0, sizeof(info));
>                      vshResetLibvirtError();
>                  } else {
> @@ -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);
> 

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list