4:26 a.m.
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(a)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