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

[John Levon <john levon sun com>]

On Wed, Jan 28, 2009 at 11:29:16AM +0000, Daniel P. Berrange wrote:

> > +                        "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.

OK.

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

I screwed up the test. It was working on a live system, but of course
there's no xenstore in the test suite, so it wasn't testing what I
thought it was. I've modified the test to specify a vncdisplay.

> > +            (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.

Apparently so. However, handling it is strictly pointless: like
vncdisplay, it's only useful for static test cases. In all other cases
we use the authoritative xenstore results...

regards
john