On Tue, Aug 21, 2018 at 11:26 PM, Peter Krempa <pkre...@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 <h...@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: h...@redhat.com Phone: +861065339333
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
