Re: [libvirt] [PATCH] Fixes for VNC port handling

["Daniel P. Berrange" <berrange redhat com>]

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