Re: [Libvir] PATCH 2/4: Xen driver support for serial/paralle

24 Apr 2008

"Daniel P. Berrange" <berrange@redhat.com> wrote:
...
On Fri, Apr 18, 2008 at 09:27:05PM +0100, Daniel P. Berrange wrote:
...
This patch updates Xen HVM to allow use of serial ¶llel ports, though
XenD limits you to  single one of each even though QEMU supports many.
It also updates the <console> tag to support new syntax extensions.
This version fixes a few memory leaks in the previous code...
Hi Dan,
...
Index: src/xend_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.c,v
retrieving revision 1.180
diff -u -p -r1.180 xend_internal.c

--- src/xend_internal.c	10 Apr 2008 16:54:54 -0000	1.180
+++ src/xend_internal.c	24 Apr 2008 15:10:00 -0000
@@ -1376,6 +1376,243 @@ xend_parse_sexp_desc_os(virConnectPtr xe
     return(0);
 }
+
+int
+xend_parse_sexp_desc_char(virConnectPtr conn,
+                          virBufferPtr buf,
+                          const char *devtype,
+                          int portNum,
+                          const char *value,
+                          const char *tty)
+{
+    const char *type;
+    int telnet = 0;
+    char *bindPort = NULL;
+    char *bindHost = NULL;
+    char *connectPort = NULL;
+    char *connectHost = NULL;
+    char *path = NULL;
+    int ret = -1;
+
+    if (value[0] == '/') {
+        type = "dev";
+    } else if (STREQLEN(value, "null", 4)) {
+        type = "null";
+        value = NULL;
+    } else if (STREQLEN(value, "vc", 2)) {
+        type = "vc";
+        value = NULL;
+    } else if (STREQLEN(value, "pty", 3)) {
+        type = "pty";
+        value = NULL;
+    } else if (STREQLEN(value, "stdio", 5)) {
+        type = "stdio";
+        value = NULL;
+    } else if (STREQLEN(value, "file:", 5)) {
+        type = "file";
+        value += 5;
+    } else if (STREQLEN(value, "pipe:", 5)) {
+        type = "pipe";
+        value += 5;
+    } else if (STREQLEN(value, "tcp:", 4)) {
+        type = "tcp";
+        value += 4;
+    } else if (STREQLEN(value, "telnet:", 4)) {
That "4" should be 7.

It'd be nice to avoid the riskily redundant length,
by using a STR* macro that requires a literal string S
as argument #2, and uses sizeof(S)-1 as the length argument.

This code could be factored even more with a loop.
The only special cases would be value[0]=='/' and
"telnet:" having type of "tcp" and setting telnet=1.
...
+        type = "tcp";
+        value += 7;
+        telnet = 1;
+    } else if (STREQLEN(value, "udp:", 4)) {
+        type = "udp";
+        value += 4;
+    } else if (STREQLEN(value, "unix:", 5)) {
+        type = "unix";
+        value += 5;
+    } else {
+        virXendError(conn, VIR_ERR_INTERNAL_ERROR,
+                     _("Unknown char device type"));
+        return -1;
+    }
+
...
...
+    } else if (STREQ(type, "unix")) {
+        const char *offset = strchr(value, ',');
+        int dolisten = 0;
+        if (offset)
+            path = strndup(value, (offset - value));
+        else
+            path = strdup(value);
+        if (path == NULL)
+            goto no_memory;
+
+        if (strstr(offset, ",listen") != NULL)
If the strchr call above finds no comma, then
"offset" will be NULL and strstr call will segfault.

...
...
+        tmp = sexpr_node(root, "domain/image/hvm/parallel");
+        if (tmp && STRNEQ(tmp, "none")) {
+            /* XXX does XenD stuff parallel port tty info into xenstore somewhere ? */
+            if (xend_parse_sexp_desc_char(conn, &buf, "parallel", 0, tmp, NULL) < 0)
+                goto error;
+        }
+    } else {
+        /* Paravirt always has a console */
+        if (tty) {
+            virBufferVSprintf(&buf, "    <console type='pty' tty='%s'>\n", tty);
+            virBufferVSprintf(&buf, "      <source path='%s'/>\n", tty);
+        } else {
+            virBufferAddLit(&buf, "    <console type='pty'>\n");
+        }
+        virBufferAddLit(&buf, "      <target port='0'/>\n");
+        virBufferAddLit(&buf, "    </console>\n");
     }
+    free(tty);
virBufferAddLit(&buf, "  </devices>\n");
     virBufferAddLit(&buf, "</domain>\n");
All of the virBufferAddLit and virBufferVSprintf calls
above can fail with ENOMEM.  I see that there are *many* more
virBufferAddLit and virBufferVSprintf calls that ignore their
return values (over 300), so maybe I'm missing something.
...
Index: src/xend_internal.h
===================================================================
RCS file: /data/cvs/libvirt/src/xend_internal.h,v
retrieving revision 1.40
diff -u -p -r1.40 xend_internal.h
--- src/xend_internal.h	10 Apr 2008 16:54:54 -0000	1.40
+++ src/xend_internal.h	24 Apr 2008 15:10:00 -0000
@@ -20,12 +20,12 @@
#include "libvirt/libvirt.h"
 #include "capabilities.h"
+#include "buf.h"
#ifdef __cplusplus
 extern "C" {
 #endif
-
 /**
  * \brief Setup the connection to a xend instance via TCP
  * \param host The host name to connect to
@@ -180,6 +180,13 @@ char *xenDaemonDomainDumpXMLByName(virCo
  */
     int xend_log(virConnectPtr xend, char *buffer, size_t n_buffer);
+    int xend_parse_sexp_desc_char(virConnectPtr conn,
+                                  virBufferPtr buf,
+                                  const char *devtype,
+                                  int portNum,
+                                  const char *value,
+                                  const char *tty);
+
   char *xend_parse_domain_sexp(virConnectPtr conn,  char *root, int xendConfigVersion);
/* refactored ones */
Index: src/xm_internal.c
===================================================================
RCS file: /data/cvs/libvirt/src/xm_internal.c,v
retrieving revision 1.70
diff -u -p -r1.70 xm_internal.c
--- src/xm_internal.c	10 Apr 2008 16:54:54 -0000	1.70
+++ src/xm_internal.c	24 Apr 2008 15:10:01 -0000
@@ -1025,11 +1025,22 @@ char *xenXMDomainFormatXML(virConnectPtr
     }
if (hvm) {
-        if (xenXMConfigGetString(conf, "serial", &str) == 0 && !strcmp(str, "pty")) {
-            virBufferAddLit(buf, "    <console/>\n");
+        if (xenXMConfigGetString(conf, "parallel", &str) == 0) {
+            if (STRNEQ(str, "none"))
+                xend_parse_sexp_desc_char(conn, buf, "parallel", 0, str, NULL);
+        }
+        if (xenXMConfigGetString(conf, "serial", &str) == 0) {
+            if (STRNEQ(str, "none")) {
+                xend_parse_sexp_desc_char(conn, buf, "serial", 0, str, NULL);
+                /* Add back-compat console tag for primary console */
+                xend_parse_sexp_desc_char(conn, buf, "console", 0, str, NULL);
+            }
         }
-    } else { /* Paravirt implicitly always has a console */
-        virBufferAddLit(buf, "    <console/>\n");
+    } else {
+        /* Paravirt implicitly always has a single console */
+        virBufferAddLit(buf, "    <console type='pty'>\n");
+        virBufferAddLit(buf, "      <target port='0'/>\n");
+        virBufferAddLit(buf, "    </console>\n");
     }
virBufferAddLit(buf, "  </devices>\n");
@@ -2267,14 +2278,38 @@ virConfPtr xenXMParseXMLToConfig(virConn
     obj = NULL;
if (hvm) {
-        obj = xmlXPathEval(BAD_CAST "count(/domain/devices/console) > 0", ctxt);
-        if ((obj != NULL) && (obj->type == XPATH_BOOLEAN) &&
-            (obj->boolval)) {
-            if (xenXMConfigSetString(conf, "serial", "pty") < 0)
+        xmlNodePtr cur;
+        cur = virXPathNode("/domain/devices/parallel[1]", ctxt);
+        if (cur != NULL) {
+            char scratch[PATH_MAX];
+
+            if (virDomainParseXMLOSDescHVMChar(conn, scratch, sizeof(scratch), cur) < 0) {
+                goto error;
+            }
+
+            if (xenXMConfigSetString(conf, "parallel", scratch) < 0)
+                goto error;
+        } else {
+            if (xenXMConfigSetString(conf, "parallel", "none") < 0)
                 goto error;
         }
-        xmlXPathFreeObject(obj);
-        obj = NULL;
+
+        cur = virXPathNode("/domain/devices/serial[1]", ctxt);
+        if (cur != NULL) {
+            char scratch[PATH_MAX];
+            if (virDomainParseXMLOSDescHVMChar(conn, scratch, sizeof(scratch), cur) < 0)
+                goto error;
+            if (xenXMConfigSetString(conf, "serial", scratch) < 0)
+                goto error;
+        } else {
+            if (virXPathBoolean("count(/domain/devices/console) > 0", ctxt)) {
+                if (xenXMConfigSetString(conf, "serial", "pty") < 0)
+                    goto error;
+            } else {
+                if (xenXMConfigSetString(conf, "serial", "none") < 0)
+                    goto error;
+            }
+        }
     }
xmlFreeDoc(doc);
Index: src/xml.c
===================================================================
RCS file: /data/cvs/libvirt/src/xml.c,v
retrieving revision 1.117
diff -u -p -r1.117 xml.c
--- src/xml.c	10 Apr 2008 16:54:54 -0000	1.117
+++ src/xml.c	24 Apr 2008 15:10:01 -0000
@@ -673,6 +673,178 @@ virDomainParseXMLGraphicsDescVFB(virConn
 }
+int
+virDomainParseXMLOSDescHVMChar(virConnectPtr conn,
+                               char *buf,
+                               size_t buflen,
+                               xmlNodePtr node)
+{
+    xmlChar *type = NULL;
Can you use "const char *" above, instead of "xmlChar *"?
If so, that'd let you remove most of those risky and
ugly (const char*) casts below.

Ditto for all of these others.
...
+    xmlChar *path = NULL;
+    xmlChar *bindHost = NULL;
+    xmlChar *bindService = NULL;
+    xmlChar *connectHost = NULL;
+    xmlChar *connectService = NULL;
+    xmlChar *mode = NULL;
+    xmlChar *protocol = NULL;
+    xmlNodePtr cur;
+
+    type = xmlGetProp(node, BAD_CAST "type");
+
+    if (type != NULL) {
+        cur = node->children;
+        while (cur != NULL) {
+            if (cur->type == XML_ELEMENT_NODE) {
+                if (xmlStrEqual(cur->name, BAD_CAST "source")) {
+                    if (mode == NULL)
+                        mode = xmlGetProp(cur, BAD_CAST "mode");
+
+                    if (STREQ((const char *)type, "dev") ||
+                        STREQ((const char *)type, "file") ||
+                        STREQ((const char *)type, "pipe") ||
+                        STREQ((const char *)type, "unix")) {
+                        if (path == NULL)
+                            path = xmlGetProp(cur, BAD_CAST "path");
+
+                    } else if (STREQ((const char *)type, "udp") ||
+                               STREQ((const char *)type, "tcp")) {
+                        if (mode == NULL ||
+                            STREQ((const char *)mode, "connect")) {
+
+                            if (connectHost == NULL)
+                                connectHost = xmlGetProp(cur, BAD_CAST "host");
+                            if (connectService == NULL)
+                                connectService = xmlGetProp(cur, BAD_CAST "service");
+                        } else {
+                            if (bindHost == NULL)
+                                bindHost = xmlGetProp(cur, BAD_CAST "host");
+                            if (bindService == NULL)
+                                bindService = xmlGetProp(cur, BAD_CAST "service");
+                        }
+
+                        if (STREQ((const char*)type, "udp")) {
+                            xmlFree(mode);
+                            mode = NULL;
+                        }
+                    }
+                } else if (xmlStrEqual(cur->name, BAD_CAST "protocol")) {
+                    if (protocol == NULL)
+                        protocol = xmlGetProp(cur, BAD_CAST "type");
+                }
+            }
+            cur = cur->next;
+        }
+    }
+
+    if (type == NULL ||
+        STREQ((const char *)type, "pty")) {
+        strncpy(buf, "pty", buflen);
+    } else if (STREQ((const char *)type, "null") ||
+               STREQ((const char *)type, "stdio") ||
+               STREQ((const char *)type, "vc")) {
+        snprintf(buf, buflen, "%s", type);
+    } else if (STREQ((const char *)type, "file") ||
+               STREQ((const char *)type, "dev") ||
+               STREQ((const char *)type, "pipe")) {
...

    

Re: [Libvir] PATCH 2/4: Xen driver support for serial/paralle

Jim Meyering