On Tue, Jan 27, 2009 at 09:23:42PM -0800, john.levon(a)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(a)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 :|