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

Han Han hhan at redhat.com
Wed Aug 22 08:50:04 UTC 2018


On Tue, Aug 21, 2018 at 11:26 PM, Peter Krempa <pkrempa at 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 at 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 at redhat.com
> > https://www.redhat.com/mailman/listinfo/libvir-list
>



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

Email: hhan at redhat.com
Phone: +861065339333
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20180822/e47998f4/attachment-0001.htm>


More information about the libvir-list mailing list