<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Aug 21, 2018 at 11:26 PM, Peter Krempa <span dir="ltr"><<a href="mailto:pkrempa@redhat.com" target="_blank">pkrempa@redhat.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">On Tue, Aug 21, 2018 at 21:23:42 +0800, Han Han wrote:<br>
> <a href="https://bugzilla.redhat.com/show_bug.cgi?id=1619625" rel="noreferrer" target="_blank">https://bugzilla.redhat.com/<wbr>show_bug.cgi?id=1619625</a><br>
> <br>
> --all option is added to cmdDomblkinfo since commit 62c39193 allowing to<br>
> show all block devices info. Remove its 'goto cleanup' part in case it breaks<br>
> the loop of domblkinfo for all disks. Remove unnecessary variables and the<br>
> condition part after virDomainGetBlockInfo returning fail.<br>
> <br>
> Signed-off-by: Han Han <<a href="mailto:hhan@redhat.com">hhan@redhat.com</a>><br>
> ---<br>
>  tools/virsh-domain-monitor.c | 18 ++----------------<br>
>  1 file changed, 2 insertions(+), 16 deletions(-)<br>
> <br>
> diff --git a/tools/virsh-domain-monitor.c b/tools/virsh-domain-monitor.c<br>
> index b9b4f9739b..ee926baae8 100644<br>
> --- a/tools/virsh-domain-monitor.c<br>
> +++ b/tools/virsh-domain-monitor.c<br>
> @@ -476,7 +476,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)<br>
>      size_t i;<br>
>      xmlNodePtr *disks = NULL;<br>
>      char *target = NULL;<br>
> -    char *protocol = NULL;<br>
>  <br>
>      if (!(dom = virshCommandOptDomain(ctl, cmd, NULL)))<br>
>          return false;<br>
> @@ -490,7 +489,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)<br>
>      human = vshCommandOptBool(cmd, "human");<br>
>  <br>
>      if (all) {<br>
> -        bool active = virDomainIsActive(dom) == 1;<br>
>          int rc;<br>
>  <br>
>          if (virshDomainGetXML(ctl, cmd, 0, &xmldoc, &ctxt) < 0)<br>
> @@ -505,29 +503,18 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)<br>
>  <br>
>          for (i = 0; i < ndisks; i++) {<br>
>              ctxt->node = disks[i];<br>
> -            protocol = virXPathString("string(./<wbr>source/@protocol)", ctxt);<br>
>              target = virXPathString("string(./<wbr>target/@dev)", ctxt);<br>
<br>
</div></div>Well, here you can check also whether the <source> element is present<br></blockquote><div>Agree. However, I think it is better not to skip <span class="gmail-">virDomainGetBlockInfo</span> for empty cdrom.</div><div>I expect the empty cdrom output will be:</div><div><br></div><div>Target     Capacity        Allocation      Physical       <br>-----------------------------------------------------<br>sda              -                      -                 -<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
...<br>
<span class="gmail-"><br>
>  <br>
>              rc = virDomainGetBlockInfo(dom, target, &info, 0);<br>
<br>
</span>... and skip this.<br>
<span class="gmail-"><br>
>  <br>
>              if (rc < 0) {<br>
> -                /* If protocol is present that's an indication of a networked<br>
> -                 * storage device which cannot provide statistics, so generate<br>
> -                 * 0 based data and get the next disk. */<br>
> -                if (protocol && !active &&<br>
> -                    virGetLastErrorCode() == VIR_ERR_INTERNAL_ERROR &&<br>
> -                    virGetLastErrorDomain() == VIR_FROM_STORAGE) {<br></span></blockquote><div>I will add one checking of source element here to skip the error from cdrom. <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">
> -                    memset(&info, 0, sizeof(info));<br>
> -                    vshResetLibvirtError();<br>
> -                } else {<br>
> -                    goto cleanup;<br>
> -                }<br>
> +                memset(&info, 0, sizeof(info));<br>
> +                vshResetLibvirtError();<br>
<br>
</span>Rather than skipping errors.<br>
<br>
This solution may skip other errors as well. Depends whether we are okay<br>
with that in such case.<br>
<br>
On the other hand getting rid of the code above looks good.<br>
<div class="gmail-HOEnZb"><div class="gmail-h5"><br>
>              }<br>
>  <br>
>              cmdDomblkinfoPrint(ctl, &info, target, human, false);<br>
>  <br>
>              VIR_FREE(target);<br>
> -            VIR_FREE(protocol);<br>
>          }<br>
>      } else {<br>
>          if (virDomainGetBlockInfo(dom, device, &info, 0) < 0)<br>
> @@ -541,7 +528,6 @@ cmdDomblkinfo(vshControl *ctl, const vshCmd *cmd)<br>
>   cleanup:<br>
>      virshDomainFree(dom);<br>
>      VIR_FREE(target);<br>
> -    VIR_FREE(protocol);<br>
>      VIR_FREE(disks);<br>
>      xmlXPathFreeContext(ctxt);<br>
>      xmlFreeDoc(xmldoc);<br>
> -- <br>
> 2.18.0<br>
> <br>
</div></div><span class="gmail-HOEnZb"><font color="#888888">> --<br>
> libvir-list mailing list<br>
> <a href="mailto:libvir-list@redhat.com">libvir-list@redhat.com</a><br>
> <a href="https://www.redhat.com/mailman/listinfo/libvir-list" rel="noreferrer" target="_blank">https://www.redhat.com/<wbr>mailman/listinfo/libvir-list</a><br>
</font></span></blockquote></div><br><br clear="all"><br>-- <br><div class="gmail_signature"><div dir="ltr"><div><div dir="ltr"><div><div dir="ltr">Best regards,</div><div dir="ltr">-----------------------------------<br></div><div dir="ltr">Han Han<br>Quality Engineer<br>Redhat.<br><br>Email: <a href="mailto:hhan@redhat.com" target="_blank">hhan@redhat.com</a><br>Phone: +861065339333<br></div></div></div></div></div></div>
</div></div>