On Tue, Jan 27, 2009 at 09:23:42PM -0800, john.levon@xxxxxxx 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@xxxxxxx>
>
> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list