[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

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



"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 &parallel 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")) {
...


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]