[libvirt] [PATCH] virsh: Clean up and refactor cmdDomDisplay() and fix some possible leaks

[Osier Yang]

On 2012年07月25日 17:41, Peter Krempa wrote:
> This patch refactors cmdDomDisplay to update coding style and remove
> some possible memory leaks on error paths. This patch also fixes flag
> variable type to unsigned.
> ---
>   tools/virsh.c |   76 ++++++++++++++++++++++++++------------------------------
>   1 files changed, 35 insertions(+), 41 deletions(-)
>
> diff --git a/tools/virsh.c b/tools/virsh.c
> index 6d65036..eea580d 100644
> --- a/tools/virsh.c
> +++ b/tools/virsh.c
> @@ -13861,16 +13861,15 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>       virDomainPtr dom;
>       virBuffer buf = VIR_BUFFER_INITIALIZER;
>       bool ret = false;
> -    char *doc;
> -    char *xpath;
> -    char *listen_addr;
> +    char *doc = NULL;
> +    char *xpath = NULL;
> +    char *listen_addr = NULL;
>       int port, tls_port = 0;
>       char *passwd = NULL;
>       char *output = NULL;
>       const char *scheme[] = { "vnc", "spice", "rdp", NULL };
>       int iter = 0;
> -    int tmp;
> -    int flags = 0;
> +    unsigned int flags = 0;
>
>       if (!vshConnectionUsability(ctl, ctl->conn))
>           return false;
> @@ -13886,40 +13885,34 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>       if (vshCommandOptBool(cmd, "include-password"))
>           flags |= VIR_DOMAIN_XML_SECURE;
>
> -    doc = virDomainGetXMLDesc(dom, flags);
> -
> -    if (!doc)
> +    if (!(doc = virDomainGetXMLDesc(dom, flags)))
>           goto cleanup;
>
> -    xml = virXMLParseStringCtxt(doc, _("(domain_definition)"),&ctxt);
> -    VIR_FREE(doc);
> -    if (!xml)
> +    if (!(xml = virXMLParseStringCtxt(doc, _("(domain_definition)"),&ctxt)))
>           goto cleanup;
>
>       /* Attempt to grab our display info */
>       for (iter = 0; scheme[iter] != NULL; iter++) {
>           /* Create our XPATH lookup for the current display's port */
> -        virAsprintf(&xpath, "string(/domain/devices/graphics[@type='%s']"
> -                "/@port)", scheme[iter]);
> -        if (!xpath) {
> +        VIR_FREE(xpath);
> +        if (virAsprintf(&xpath,
> +                        "string(/domain/devices/graphics[@type='%s']/@port)",
> +                        scheme[iter])<  0) {
>               virReportOOMError();
>               goto cleanup;
>           }
>
> -        /* Attempt to get the port number for the current graphics scheme */
> -        tmp = virXPathInt(xpath, ctxt,&port);
> -        VIR_FREE(xpath);
> -
> -        /* If there is no port number for this type, then jump to the next
> -         * scheme
> +        /* Attempt to get the port number for the current graphics scheme.
> +         * If there is no port number for this type, then jump to the next scheme.
>            */
> -        if (tmp)
> +        if (virXPathInt(xpath, ctxt,&port)<  0)
>               continue;
>
>           /* Create our XPATH lookup for the current display's address */
> -        virAsprintf(&xpath, "string(/domain/devices/graphics[@type='%s']"
> -                "/@listen)", scheme[iter]);
> -        if (!xpath) {
> +        VIR_FREE(xpath);
> +        if (virAsprintf(&xpath,
> +                        "string(/domain/devices/graphics[@type='%s']/@listen)",
> +                        scheme[iter])<  0) {
>               virReportOOMError();
>               goto cleanup;
>           }
> @@ -13928,7 +13921,6 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>            * graphics scheme
>            */
>           listen_addr = virXPathString(xpath, ctxt);
> -        VIR_FREE(xpath);
>
>           /* Per scheme data mangling */
>           if (STREQ(scheme[iter], "vnc")) {
> @@ -13936,31 +13928,32 @@ cmdDomDisplay(vshControl *ctl, const vshCmd *cmd)
>               port -= 5900;
>           } else if (STREQ(scheme[iter], "spice")) {
>               /* Create our XPATH lookup for the SPICE TLS Port */
> -            virAsprintf(&xpath, "string(/domain/devices/graphics[@type='%s']"
> -                    "/@tlsPort)", scheme[iter]);
> -            if (!xpath) {
> +            VIR_FREE(xpath);
> +            if (virAsprintf(&xpath,
> +                            "string(/domain/devices/graphics[@type='%s']"
> +                             "/@tlsPort)",

One space nit. ACK.

Regards,
Osier