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