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@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