[libvirt] [PATCH] virsh: Use virXPath wrappers for vncdisplay cmd
Peter Krempa
pkrempa at redhat.com
Mon Jun 25 09:26:58 UTC 2012
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 at cardoe.com>
> ---
> 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
More information about the libvir-list
mailing list