Re: [Libvir] Support graphical console for PV Xen guests

[Jeremy Katz <katzj redhat com>]

On Sat, 2006-08-26 at 10:18 -0400, Daniel Veillard wrote:
> On Sat, Aug 26, 2006 at 09:49:05AM -0400, Jeremy Katz wrote:
> > With the paravirt framebuffer patches for Xen, we can support having a
> > graphical console for PV as well as HVM guests.  The attached adds
> > support to libvirt by breaking out the <graphics> parsing into its own
> > function that can be used by both PV and HVM guests.  It also adds
> > support for looking at the appropriate sexpr for the vnc/sdl console for
> > PV guests.
> > 
> > One thing that is potentially not okay is that I've changed sexpr_node
> > to take a format string instead of just a const char *, but since it's
> > not public API and doesn't actually change anything for callers I think
> > it should be fine.  If not, let me know and I can change it.
> 
>   Honnestly I would prefer to add a new sexpr_fmt_node() routine with
> the snprintf call which would then call the old sexpr_node() and replace
> the few places where you need that functionality with call to sexpr_fmt_node.
> I'm just a bit afraid that people may not realize it's expanded that way
> and pass uncheck strings.
> 
>   Otherwise it just look fine, I can do the change or you can do it, either
> ways.

Simple enough, attached

Jeremy

Index: src/sexpr.c
===================================================================
RCS file: /data/cvs/libvirt/src/sexpr.c,v
retrieving revision 1.3
diff -u -u -r1.3 sexpr.c
--- src/sexpr.c	15 Mar 2006 12:13:25 -0000	1.3
+++ src/sexpr.c	26 Aug 2006 15:21:05 -0000
@@ -486,3 +486,27 @@
 
     return (n && n->car->kind == SEXPR_VALUE) ? n->car->value : NULL;
 }
+
+/**
+ * sexpr_fmt_node:
+ * @sexpr: a pointer to a parsed S-Expression
+ * @fmt: a path for the node to lookup in the S-Expression
+ * @... extra data to build the path
+ *
+ * Search a node value in the S-Expression based on its path
+ * NOTE: path are limited to 4096 bytes.
+ *
+ * Returns the value of the node or NULL if not found.
+ */
+const char *
+sexpr_fmt_node(struct sexpr *sexpr, const char *fmt, ...)
+{
+    va_list ap;
+    char node[4096];
+
+    va_start(ap, fmt);
+    vsnprintf(node, sizeof(node), fmt, ap);
+    va_end(ap);
+
+    return sexpr_node(sexpr, node);
+}
Index: src/sexpr.h
===================================================================
RCS file: /data/cvs/libvirt/src/sexpr.h,v
retrieving revision 1.2
diff -u -u -r1.2 sexpr.h
--- src/sexpr.h	15 Mar 2006 12:13:25 -0000	1.2
+++ src/sexpr.h	26 Aug 2006 15:21:05 -0000
@@ -45,5 +45,6 @@
 
 /* lookup in S-Expressions */
 const char *sexpr_node(struct sexpr *sexpr, const char *node);
+const char *sexpr_fmt_node(struct sexpr *sexpr, const char *fmt, ...);
 struct sexpr *sexpr_lookup(struct sexpr *sexpr, const char *node);
 #endif
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.57
diff -u -u -r1.57 xend_internal.c
--- src/xend_internal.c	24 Aug 2006 15:05:19 -0000	1.57
+++ src/xend_internal.c	26 Aug 2006 15:21:07 -0000
@@ -1618,23 +1618,23 @@
             virBufferAdd(&buf, "      <readonly/>\n", 18);
             virBufferAdd(&buf, "    </disk>\n", 12);
         }
+    }
         
-        /* Graphics device */
-        tmp = sexpr_node(root, "domain/image/hvm/vnc");
-        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 */
+    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_node(root, "domain/image/hvm/sdl");
-        if (tmp != NULL) {
-           if (tmp[0] == '1')
-               virBufferAdd(&buf, "    <graphics type='sdl'/>\n", 27 );
-        }
+    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 );
     }
     
     tty = xenStoreDomainGetConsolePath(conn, domid);
Index: src/xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.36
diff -u -u -r1.36 xml.c
--- src/xml.c	18 Aug 2006 20:20:50 -0000	1.36
+++ src/xml.c	26 Aug 2006 15:21:08 -0000
@@ -571,6 +571,40 @@
 
 #ifndef PROXY
 /**
+ * virtDomainParseXMLGraphicsDesc:
+ * @node: node containing graphics description
+ * @buf: a buffer for the result S-Expr
+ *
+ * Parse the graphics part of the XML description and add it to the S-Expr 
+ * in buf.  This is a temporary interface as the S-Expr interface will be 
+ * replaced by XML-RPC in the future. However the XML format should stay 
+ * valid over time.
+ *
+ * Returns 0 in case of success, -1 in case of error
+ */
+static int virDomainParseXMLGraphicsDesc(xmlNodePtr node, virBufferPtr buf)
+{
+    xmlChar *graphics_type = NULL;
+
+    graphics_type = xmlGetProp(node, BAD_CAST "type");
+    if (graphics_type != NULL) {
+        if (xmlStrEqual(graphics_type, BAD_CAST "sdl")) {
+            virBufferAdd(buf, "(sdl 1)", 7);
+            // TODO:
+            // Need to understand sdl options
+            //
+            //virBufferAdd(buf, "(display localhost:10.0)", 24);
+            //virBufferAdd(buf, "(xauthority /root/.Xauthority)", 30);
+        }
+        else if (xmlStrEqual(graphics_type, BAD_CAST "vnc"))
+            virBufferAdd(buf, "(vnc 1)", 7);
+        xmlFree(graphics_type);
+    }
+    return 0;
+}
+
+
+/**
  * virDomainParseXMLOSDescHVM:
  * @node: node containing HVM OS description
  * @buf: a buffer for the result S-Expr
@@ -591,7 +625,7 @@
     const xmlChar *type = NULL;
     const xmlChar *loader = NULL;
     const xmlChar *boot_dev = NULL;
-    xmlChar *graphics_type = NULL;
+    int res;
 
     cur = node->children;
     while (cur != NULL) {
@@ -733,24 +767,11 @@
     /* Is a graphics device specified? */
     obj = xmlXPathEval(BAD_CAST "/domain/devices/graphics[1]", ctxt);
     if ((obj != NULL) && (obj->type == XPATH_NODESET) &&
-        (obj->nodesetval != NULL) && (obj->nodesetval->nodeNr = 0)) {
-        virXMLError(VIR_ERR_NO_OS, "", 0); /* TODO: error */
-        goto error;
-    }
-
-    graphics_type = xmlGetProp(obj->nodesetval->nodeTab[0], BAD_CAST "type");
-    if (graphics_type != NULL) {
-        if (xmlStrEqual(graphics_type, BAD_CAST "sdl")) {
-            virBufferAdd(buf, "(sdl 1)", 7);
-            // TODO:
-            // Need to understand sdl options
-            //
-            //virBufferAdd(buf, "(display localhost:10.0)", 24);
-            //virBufferAdd(buf, "(xauthority /root/.Xauthority)", 30);
+        (obj->nodesetval != NULL) && (obj->nodesetval->nodeNr > 0)) {
+        res = virDomainParseXMLGraphicsDesc(obj->nodesetval->nodeTab[0], buf);
+        if (res != 0) {
+            goto error;
         }
-        else if (xmlStrEqual(graphics_type, BAD_CAST "vnc"))
-            virBufferAdd(buf, "(vnc 1)", 7);
-        xmlFree(graphics_type);
     }
     xmlXPathFreeObject(obj);
 
@@ -767,6 +788,7 @@
  * virDomainParseXMLOSDescPV:
  * @node: node containing PV OS description
  * @buf: a buffer for the result S-Expr
+ * @ctxt: a path context representing the XML description
  *
  * Parse the OS part of the XML description for a paravirtualized domain
  * and add it to the S-Expr in buf.  This is a temporary interface as the
@@ -776,14 +798,16 @@
  * Returns 0 in case of success, -1 in case of error.
  */
 static int
-virDomainParseXMLOSDescPV(xmlNodePtr node, virBufferPtr buf)
+virDomainParseXMLOSDescPV(xmlNodePtr node, virBufferPtr buf, xmlXPathContextPtr ctxt)
 {
     xmlNodePtr cur, txt;
+    xmlXPathObjectPtr obj = NULL;
     const xmlChar *type = NULL;
     const xmlChar *root = NULL;
     const xmlChar *kernel = NULL;
     const xmlChar *initrd = NULL;
     const xmlChar *cmdline = NULL;
+    int res;
 
     cur = node->children;
     while (cur != NULL) {
@@ -840,6 +864,19 @@
         virBufferVSprintf(buf, "(root '%s')", (const char *) root);
     if (cmdline != NULL)
         virBufferVSprintf(buf, "(args '%s')", (const char *) cmdline);
+
+    /* Is a graphics device specified? */
+    obj = xmlXPathEval(BAD_CAST "/domain/devices/graphics[1]", ctxt);
+    if ((obj != NULL) && (obj->type == XPATH_NODESET) &&
+        (obj->nodesetval != NULL) && (obj->nodesetval->nodeNr > 0)) {
+        res = virDomainParseXMLGraphicsDesc(obj->nodesetval->nodeTab[0], buf);
+        if (res != 0) {
+            goto error;
+        }
+    }
+    xmlXPathFreeObject(obj);
+
+ error:
     virBufferAdd(buf, "))", 2);
     return (0);
 }
@@ -1177,7 +1214,7 @@
 	}
 
 	if ((tmpobj == NULL) || !xmlStrEqual(tmpobj->stringval, BAD_CAST "hvm")) {
-	    res = virDomainParseXMLOSDescPV(obj->nodesetval->nodeTab[0], &buf);
+	    res = virDomainParseXMLOSDescPV(obj->nodesetval->nodeTab[0], &buf, ctxt);
 	} else {
 	    hvm = 1;
 	    res = virDomainParseXMLOSDescHVM(obj->nodesetval->nodeTab[0], &buf, ctxt);