On Tue, Jan 27, 2009 at 09:23:42PM -0800, john.levon@sun.com wrote:
Fixes for VNC port handling
When parsing sexpr, the VNC port should not be ignored, even when vncunused is set. Fix the parsing of vncdisplay, which was broken due to strtol() (never use this function!).
Signed-off-by: John Levon <john.levon@sun.com>
diff --git a/src/xend_internal.c b/src/xend_internal.c --- a/src/xend_internal.c +++ b/src/xend_internal.c @@ -612,7 +612,9 @@ sexpr_get(virConnectPtr xend, const char * * convenience function to lookup an int value in the S-Expression * - * Returns the value found or 0 if not found (but may not be an error) + * Returns the value found or 0 if not found (but may not be an error). + * This function suffers from the flaw that zero is both a correct + * return value and an error indicator: careful! */ static int sexpr_int(const struct sexpr *sexpr, const char *name) @@ -2091,15 +2093,16 @@ xenDaemonParseSxprGraphicsNew(virConnect port = xenStoreDomainGetVNCPort(conn, def->id); xenUnifiedUnlock(priv);
+ // Didn't find port entry in xenstore if (port == -1) { - // Didn't find port entry in xenstore - port = sexpr_int(node, "device/vfb/vncdisplay"); + const char *value = sexpr_node(node, + "device/vfb/vncdisplay"); + if (value != NULL) + port = strtol(value, NULL, 0);
This isn't checking the return value of strtol, so it could have parsed garbage from XenD's SEXPR. Prefer to use virStrToLong_i() and initialize port back to -1 upon failure.
- if ((unused && STREQ(unused, "1")) || port == -1) { + if ((unused && STREQ(unused, "1")) || port == -1) graphics->data.vnc.autoport = 1; - port = -1; - }
if (port >= 0 && port < 5900) port += 5900;
The general idea of the patch seems correct. A question about the test case though - the code above is dealing with correctly handling the 'device/vfb/vncdisplay' field in the SEXPR, but your test SEXPR data doesn't have a 'vncdisplay' field:
+ (device + (vfb + (vncunused 1) + (keymap en-us) + (type vnc) + (uuid 09666ad1-0c94-d79c-1439-99e05394ee51) + (location localhost:5900) + ) + )
And I've not seen this 'location' field before - guess that's something new in XenD we've not handled before. Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|