Re: [PATCH] virsh: Use virXPath wrappers for vncdisplay cmd

[Peter Krempa <pkrempa@xxxxxxxxxx>]

On 06/24/12 23:36, Doug Goldstein wrote:
> Update the vncdisplay command to use the virXPath wrappers as well as
> check if the domain is up rather than using the port set to -1 to mean
> the domain is not up.
> 
> Signed-off-by: Doug Goldstein <cardoe@xxxxxxxxxx>
> ---
>   tools/virsh.c |   30 +++++++++++++++---------------
>   1 files changed, 15 insertions(+), 15 deletions(-)
> 
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 0354822..a6649f4 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -13844,12 +13844,12 @@ static bool
>   cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
>   {
>       xmlDocPtr xml = NULL;
> -    xmlXPathObjectPtr obj = NULL;
>       xmlXPathContextPtr ctxt = NULL;
>       virDomainPtr dom;
>       bool ret = false;
>       int port = 0;
>       char *doc;
> +    char *listen_addr;
>   
>       if (!vshConnectionUsability(ctl, ctl->conn))
>           return false;
> @@ -13857,6 +13857,12 @@ cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
>       if (!(dom = vshCommandOptDomain(ctl, cmd, NULL)))
>           return false;
>   
> +    /* Check if the domain is active and don't rely on -1 for this */
> +    if (!virDomainIsActive(dom)) {
> +        vshError(ctl, _("Domain is not running"));
> +        goto cleanup;
> +    }
> +
>       doc = virDomainGetXMLDesc(dom, 0);
>       if (!doc)
>           goto cleanup;
> @@ -13866,29 +13872,23 @@ cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
>       if (!xml)
>           goto cleanup;
>   
> -    obj = xmlXPathEval(BAD_CAST "string(/domain/devices/graphics[@type='vnc']/@port)", ctxt);
> -    if (obj == NULL || obj->type != XPATH_STRING ||
> -        obj->stringval == NULL || obj->stringval[0] == 0) {
> +    /* Get the VNC port */
> +    if (virXPathInt("string(/domain/devices/graphics[@type='vnc']/@port)",
> +        ctxt, &port)) {

We align arguments that continue on the next line with the first argument and not the function name.

> +        vshError(ctl, _("Failed to get VNC port. Is this domain using VNC?"));
>           goto cleanup;
>       }
> -    if (virStrToLong_i((const char *)obj->stringval, NULL, 10, &port) || port < 0)
> -        goto cleanup;
> -    xmlXPathFreeObject(obj);
>   
> -    obj = xmlXPathEval(BAD_CAST "string(/domain/devices/graphics[@type='vnc']/@listen)", ctxt);
> -    if (obj == NULL || obj->type != XPATH_STRING ||
> -        obj->stringval == NULL || obj->stringval[0] == 0 ||
> -        STREQ((const char*)obj->stringval, "0.0.0.0")) {
> +    listen_addr = virXPathString("string(/domain/devices/graphics"
> +            "[@type='vnc']/@listen)", ctxt);

Same with continued strings. (As long as it's possible)

> +    if (listen_addr == NULL || STREQ((const char *)listen_addr, "0.0.0.0")) {

We don't typecast non-const strings to const, the original code is a fallout from using libxml2's data types, that are unsigned.
Block braces are not required for one line bodies.

>           vshPrint(ctl, ":%d\n", port-5900);
>       } else {
> -        vshPrint(ctl, "%s:%d\n", (const char *)obj->stringval, port-5900);
> +        vshPrint(ctl, "%s:%d\n", (const char *)listen_addr, port-5900);
>       }
> -    xmlXPathFreeObject(obj);
> -    obj = NULL;
>       ret = true;
>   
>    cleanup:

Memory leak: You don't free listen_addr after virXPathString() allocated the result for you.

> -    xmlXPathFreeObject(obj);
>       xmlXPathFreeContext(ctxt);
>       xmlFreeDoc(xml);
>       virDomainFree(dom);
> 

Thanks for a nice cleanup. I'm pushing this with following changes squashed in as styling nits 
and the one memleak are not worth for doing a v2:

diff --git a/tools/virsh.c b/tools/virsh.c
index a6649f4..7d6b52a 100644
--- a/tools/virsh.c
+++ b/tools/virsh.c
@@ -13848,8 +13848,8 @@ cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
     virDomainPtr dom;
     bool ret = false;
     int port = 0;
-    char *doc;
-    char *listen_addr;
+    char *doc = NULL;
+    char *listen_addr = NULL;

     if (!vshConnectionUsability(ctl, ctl->conn))
         return false;
@@ -13863,32 +13863,31 @@ cmdVNCDisplay(vshControl *ctl, const vshCmd *cmd)
         goto cleanup;
     }

-    doc = virDomainGetXMLDesc(dom, 0);
-    if (!doc)
+    if (!(doc = virDomainGetXMLDesc(dom, 0)))
         goto cleanup;

-    xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt);
-    VIR_FREE(doc);
-    if (!xml)
+    if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"), &ctxt)))
         goto cleanup;

     /* Get the VNC port */
     if (virXPathInt("string(/domain/devices/graphics[@type='vnc']/@port)",
-        ctxt, &port)) {
+                    ctxt, &port)) {
         vshError(ctl, _("Failed to get VNC port. Is this domain using VNC?"));
         goto cleanup;
     }

     listen_addr = virXPathString("string(/domain/devices/graphics"
-            "[@type='vnc']/@listen)", ctxt);
-    if (listen_addr == NULL || STREQ((const char *)listen_addr, "0.0.0.0")) {
+                                 "[@type='vnc']/@listen)", ctxt);
+    if (listen_addr == NULL || STREQ(listen_addr, "0.0.0.0"))
         vshPrint(ctl, ":%d\n", port-5900);
-    } else {
-        vshPrint(ctl, "%s:%d\n", (const char *)listen_addr, port-5900);
-    }
+    else
+        vshPrint(ctl, "%s:%d\n", listen_addr, port-5900);
+
     ret = true;

  cleanup:
+    VIR_FREE(doc);
+    VIR_FREE(listen_addr);
     xmlXPathFreeContext(ctxt);
     xmlFreeDoc(xml);
     virDomainFree(dom);

Peter

--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list