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.

+ /* 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 ?

}

@@ -1769,21 +1782,26 @@ } }

- /* Graphics device */ - tmp = sexpr_fmt_node(root, "domain/image/%s/vnc", hvm ? "hvm" : "linux"); - if (tmp != NULL) { - if (tmp[0] == '1') { - int port = xenStoreDomainGetVNCPort(conn, domid); - if (port == -1) - port = 5900 + domid; - virBufferVSprintf(&buf, " <graphics type='vnc' port='%d'/>\n", port); + /* Graphics device (HVM, or old (pre-3.0.4) style PV vnc config) */ + if (hvm || xendConfigVersion < 3) { + tmp = sexpr_fmt_node(root, "domain/image/%s/vnc", hvm ? "hvm" : "linux"); + if (tmp != NULL) { + if (tmp[0] == '1') { + int port = xenStoreDomainGetVNCPort(conn, domid); + if (port == -1) + port = 5900 + domid; + virBufferVSprintf(&buf, " <graphics type='vnc' port='%d'/>\n", port); + } } }

- 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 ... But those are just side questions, feel free to commit, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/