[libvirt] [PATCH] virsh: fixed domdisplay command

[Doug Goldstein]

On Mon, Jul 23, 2012 at 1:51 PM, Martin Kletzander <mkletzan at redhat.com> wrote:
> The 'domdisplay' command didn't properly evaluate '--include-password'
> option.
> ---
>  tools/virsh.c |   35 +++++++++++++++++++++++------------
>  1 files changed, 23 insertions(+), 12 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 5888d6c..e0765ba 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -66,6 +66,7 @@
>  #include "virtypedparam.h"
>  #include "intprops.h"
>  #include "conf/virdomainlist.h"
> +#include "datatypes.h"

Why exactly is this necessary? No new types are being used that aren't
used throughout this whole file.


>
>  static char *progname;
>
> @@ -13882,7 +13883,16 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>          goto cleanup;
>      }
>
> -    doc = virDomainGetXMLDesc(dom, 0);
> +    if (!vshCommandOptBool(cmd, "include-password"))
> +        doc = virDomainGetXMLDesc(dom, 0);
> +    else {
> +        if (ctl->conn->flags & VIR_DOMAIN_XML_SECURE) {
> +            vshError(ctl, _("Cannot get password with read-only connection"));
> +            goto cleanup;
> +        }
> +        doc = virDomainGetXMLDesc(dom, VIR_DOMAIN_XML_SECURE);
> +    }
> +

I would do like all the other commands and define unsigned int flags =
0; at the top and if include-password is set just do flags |=
VIR_DOMAIN_XML_SECURE; and always call virDomainGetXMLDesc() with
flags passed. Look at cmdSnapshotCurrent for an example.



>      if (!doc)
>          goto cleanup;
>
> @@ -13944,19 +13954,20 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>              if (tmp)
>                  tls_port = 0;
>
> -            if (vshCommandOptBool(cmd, "daemon")) {
> -                /* Create our XPATH lookup for the SPICE password */
> -                virAsprintf(&xpath, "string(/domain/devices/graphics"
> +            /* Create our XPATH lookup for the SPICE password */
> +            virAsprintf(&xpath, "string(/domain/devices/graphics"
>                          "[@type='%s']/@passwd)", scheme[iter]);
> -                if (!xpath) {
> -                    virReportOOMError();
> -                    goto cleanup;
> -                }
> -
> -                /* Attempt to get the SPICE password */
> -                passwd = virXPathString(xpath, ctxt);
> -                VIR_FREE(xpath);
> +            if (!xpath) {
> +                virReportOOMError();
> +                goto cleanup;
>              }
> +
> +            /* Attempt to get the SPICE password
> +             *
> +             * This will return NULL automatically if the
> +             * virDomainGetXMLDesc wasn't secure */
> +            passwd = virXPathString(xpath, ctxt);
> +            VIR_FREE(xpath);
>          }
>
>          /* Build up the full URI, starting with the scheme */
> --
> 1.7.8.6
>

Getting rid of the conditional on include-password (which was
incorrectly stated as 'daemon') doesn't seem correct. Leave the logic
as is and just fix 'daemon' to say 'include-password'.


-- 
Doug Goldstein