1. Patchew
  2. Libvirt
  3. View series

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

Han Han posted 1 patch 5 years, 7 months ago
Patches applied successfully (tree, apply log)
git fetch https://github.com/patchew-project/libvirt tags/patchew/20180821132342.539-1-hhan@redhat.com
Test syntax-check passed
There is a newer version of this series
tools/virsh-domain-monitor.c | 18 ++----------------
1 file changed, 2 insertions(+), 16 deletions(-)
[libvirt] [PATCH] virsh: Don't break loop of domblkinfo for disks
Posted by Han Han 5 years, 7 months ago 
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. Remove its 'goto cleanup' part in case it breaks
the loop of domblkinfo for all disks. Remove unnecessary variables and the
condition part after virDomainGetBlockInfo returning fail.

Signed-off-by: Han Han <hhan@redhat.com>
---
 tools/virsh-domain-monitor.c | 18 ++----------------
 1 file changed, 2 insertions(+), 16 deletions(-)

diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
index b9b4f9739b..ee926baae8 100644
--- a/tools/virsh-domain-monitor.c
+++ b/tools/virsh-domain-monitor.c
@@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
     size_t i;
     xmlNodePtr *disks = NULL;
     char *target = NULL;
-    char *protocol = NULL;
 
     if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
         return false;
@@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
     human = vshCommandOptBool(cmd, "human");
 
     if (all) {
-        bool active = virDomainIsActive(dom) == 1;
         int rc;
 
         if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0)
@@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
 
         for (i = 0; i < ndisks; i++) {
             ctxt->node = disks[i];
-            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;
-                }
+                memset(&info, 0, sizeof(info));
+                vshResetLibvirtError();
             }
 
             cmdDomblkinfoPrint(ctl, &info, target, human, false);
 
             VIR_FREE(target);
-            VIR_FREE(protocol);
         }
     } else {
         if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
@@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
  cleanup:
     virshDomainFree(dom);
     VIR_FREE(target);
-    VIR_FREE(protocol);
     VIR_FREE(disks);
     xmlXPathFreeContext(ctxt);
     xmlFreeDoc(xmldoc);
-- 
2.18.0

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Don't break loop of domblkinfo for disks
Posted by Peter Krempa 5 years, 7 months ago 
On Tue, Aug 21, 2018 at 21:23:42 +0800, 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. Remove its 'goto cleanup' part in case it breaks
> the loop of domblkinfo for all disks. Remove unnecessary variables and the
> condition part after virDomainGetBlockInfo returning fail.
> 
> Signed-off-by: Han Han <hhan@redhat.com>
> ---
>  tools/virsh-domain-monitor.c | 18 ++----------------
>  1 file changed, 2 insertions(+), 16 deletions(-)
> 
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> index b9b4f9739b..ee926baae8 100644
> --- a/tools/virsh-domain-monitor.c
> +++ b/tools/virsh-domain-monitor.c
> @@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>      size_t i;
>      xmlNodePtr *disks = NULL;
>      char *target = NULL;
> -    char *protocol = NULL;
>  
>      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
>          return false;
> @@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>      human = vshCommandOptBool(cmd, "human");
>  
>      if (all) {
> -        bool active = virDomainIsActive(dom) == 1;
>          int rc;
>  
>          if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0)
> @@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>  
>          for (i = 0; i < ndisks; i++) {
>              ctxt->node = disks[i];
> -            protocol = virXPathString("string(./source/@protocol)", ctxt);
>              target = virXPathString("string(./target/@dev)", ctxt);

Well, here you can check also whether the <source> element is present
...

>  
>              rc = virDomainGetBlockInfo(dom, target, &info, 0);

... and skip this.

>  
>              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;
> -                }
> +                memset(&info, 0, sizeof(info));
> +                vshResetLibvirtError();

Rather than skipping errors.

This solution may skip other errors as well. Depends whether we are okay
with that in such case.

On the other hand getting rid of the code above looks good.

>              }
>  
>              cmdDomblkinfoPrint(ctl, &info, target, human, false);
>  
>              VIR_FREE(target);
> -            VIR_FREE(protocol);
>          }
>      } else {
>          if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
> @@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
>   cleanup:
>      virshDomainFree(dom);
>      VIR_FREE(target);
> -    VIR_FREE(protocol);
>      VIR_FREE(disks);
>      xmlXPathFreeContext(ctxt);
>      xmlFreeDoc(xmldoc);
> -- 
> 2.18.0
> 
> --
> libvir-list mailing list
> libvir-list@redhat.com
> https://www.redhat.com/mailman/listinfo/libvir-list
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
Re: [libvirt] [PATCH] virsh: Don't break loop of domblkinfo for disks
Posted by Han Han 5 years, 7 months ago 
On Tue, Aug 21, 2018 at 11:26 PM, Peter Krempa <pkrempa@redhat.com> wrote:

> On Tue, Aug 21, 2018 at 21:23:42 +0800, 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. Remove its 'goto cleanup' part in case it
> breaks
> > the loop of domblkinfo for all disks. Remove unnecessary variables and
> the
> > condition part after virDomainGetBlockInfo returning fail.
> >
> > Signed-off-by: Han Han <hhan@redhat.com>
> > ---
> >  tools/virsh-domain-monitor.c | 18 ++----------------
> >  1 file changed, 2 insertions(+), 16 deletions(-)
> >
> > diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c
> > index b9b4f9739b..ee926baae8 100644
> > --- a/tools/virsh-domain-monitor.c
> > +++ b/tools/virsh-domain-monitor.c
> > @@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >      size_t i;
> >      xmlNodePtr *disks = NULL;
> >      char *target = NULL;
> > -    char *protocol = NULL;
> >
> >      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))
> >          return false;
> > @@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >      human = vshCommandOptBool(cmd, "human");
> >
> >      if (all) {
> > -        bool active = virDomainIsActive(dom) == 1;
> >          int rc;
> >
> >          if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0)
> > @@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >
> >          for (i = 0; i < ndisks; i++) {
> >              ctxt->node = disks[i];
> > -            protocol = virXPathString("string(./source/@protocol)",
> ctxt);
> >              target = virXPathString("string(./target/@dev)", ctxt);
>
> Well, here you can check also whether the <source> element is present
>
Agree. However, I think it is better not to skip virDomainGetBlockInfo for
empty cdrom.
I expect the empty cdrom output will be:

Target     Capacity        Allocation      Physical
-----------------------------------------------------
sda              -                      -                 -

> ...
>
> >
> >              rc = virDomainGetBlockInfo(dom, target, &info, 0);
>
> ... and skip this.
>
> >
> >              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) {
>
I will add one checking of source element here to skip the error from
cdrom.

> > -                    memset(&info, 0, sizeof(info));
> > -                    vshResetLibvirtError();
> > -                } else {
> > -                    goto cleanup;
> > -                }
> > +                memset(&info, 0, sizeof(info));
> > +                vshResetLibvirtError();
>
> Rather than skipping errors.
>
> This solution may skip other errors as well. Depends whether we are okay
> with that in such case.
>
> On the other hand getting rid of the code above looks good.
>
> >              }
> >
> >              cmdDomblkinfoPrint(ctl, &info, target, human, false);
> >
> >              VIR_FREE(target);
> > -            VIR_FREE(protocol);
> >          }
> >      } else {
> >          if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)
> > @@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)
> >   cleanup:
> >      virshDomainFree(dom);
> >      VIR_FREE(target);
> > -    VIR_FREE(protocol);
> >      VIR_FREE(disks);
> >      xmlXPathFreeContext(ctxt);
> >      xmlFreeDoc(xmldoc);
> > --
> > 2.18.0
> >
> > --
> > libvir-list mailing list
> > libvir-list@redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>



-- 
Best regards,
-----------------------------------
Han Han
Quality Engineer
Redhat.

Email: hhan@redhat.com
Phone: +861065339333
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

Patchew 2.1-devel-0870f84

© 2016 - 2024 Red Hat, Inc.