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 -- Libvir-list mailing list Libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list