[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