Re: [Libvir] [PATCH] Support parsing of new SEXPR for PVFB
On Tue, Dec 05, 2006 at 05:16:54PM -0500, Daniel Veillard wrote:
On Tue, Dec 05, 2006 at 07:57:33PM +0000, Daniel P. Berrange wrote:
> The paravirt framebuffer patches for Xen finally got merged into upstream
> xen-devel. In doing so, however, the syntax of the SEXPR for configuring
> them changed completely. The existing framebuffer suppoirt in libvirt thus
> only works with the 3.0.3 tree + PVFB patches. For 3.0.4 we need to work
> with the new scheme. Fortunately, in 3.0.4 the xend_config_version flag
> is bumped from 2 -> 3, so we have an easy value to hook off.
>
> The attached patch thus adds support for parsing an SEXPR looking like
>
> (device
> (vfb
> (vncunused 1)
> (vnclisten 127.0.0.1)
> (vncpasswd 123456)
> (type vnc)
> )
> )
>
> And generating the <graphics type='vnc' port='5903'/> XML tag
to match.
> Index: src/xend_internal.c
> ===================================================================
> RCS file: /data/cvs/libvirt/src/xend_internal.c,v
> retrieving revision 1.78
> diff -u -r1.78 xend_internal.c
> --- src/xend_internal.c 22 Nov 2006 17:48:29 -0000 1.78
> +++ src/xend_internal.c 5 Dec 2006 20:47:19 -0000
> @@ -1736,6 +1736,19 @@
> tmp2);
>
> virBufferAdd(&buf, " </interface>\n", 17);
> + } else if (!hvm &&
> + sexpr_lookup(node, "device/vfb")) {
I assume we don't need to check here for (xendConfigVersion >= 3) because
that construct positively never happened before.
Yeah, and in fact in RHEL / Fedora we explicitly don't want the version
check, because we're planning to backport the new style PVFB config to
the Xen 3.0.3 build we have.
> + /* New style graphics config for PV guests only in
3.0.4 */
> + tmp = sexpr_node(node, "device/vfb/type");
> +
> + if (tmp && !strcmp(tmp, "sdl")) {
> + virBufferAdd(&buf, " <graphics
type='sdl'/>\n", 27);
> + } else if (tmp && !strcmp(tmp, "vnc")) {
> + int port = xenStoreDomainGetVNCPort(conn, domid);
> + if (port == -1)
> + port = 5900 + domid;
> + virBufferVSprintf(&buf, " <graphics
type='vnc' port='%d'/>\n", port);
> + }
Obviously the SEXPR contains more data, but the only one we used to export
is the port number (which we have to extract in different way), do we want to
make those extra informations about the IP listening and password part of
the dumped XML ? I guess the answer is no because the XML is provided to
non-root users via the proxy and this is a security information leak, right ?
Definitely don't want to include the password. We should add the vnclisten
data there though at some point in future - if only because it'll make it
easier for apps to sanity check. Adding the vnclisten data isn't a data leak
because you can already trivially see if with 'netstat'.
> - tmp = sexpr_fmt_node(root, "domain/image/%s/sdl",
hvm ? "hvm" : "linux");
> - if (tmp != NULL) {
> - if (tmp[0] == '1')
> - virBufferAdd(&buf, " <graphics
type='sdl'/>\n", 27 );
> + /* Graphics device (HVM, or old (pre-3.0.4) style PV sdl config) */
> + if (hvm || xendConfigVersion < 3){
> + tmp = sexpr_fmt_node(root, "domain/image/%s/sdl", hvm ?
"hvm" : "linux");
> + if (tmp != NULL) {
> + if (tmp[0] == '1')
> + virBufferAdd(&buf, " <graphics
type='sdl'/>\n", 27 );
> + }
> }
Could we potentially have both sdl and vnc ? I'm wondering why this need to
be limited it xendConfigVersion < 3 ...
I guess I could simply leave out that chunk /change - since that old style
config for PVFB simply won't ever appear in XenD SEXPR from 3.0.4 onwards.
Regards,
Dan.
--
|=- Red Hat, Engineering, Emerging Technologies, Boston. +1 978 392 2496 -=|
|=- Perl modules: http://search.cpan.org/~danberr/ -=|
|=- Projects: http://freshmeat.net/~danielpb/ -=|
|=- GnuPG: 7D3B9505 F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 -=|