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

"Daniel P. Berrange" <berrange(a)redhat.com&gt; 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.

> + } 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.

> 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.

> +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.