On Thu, Apr 24, 2008 at 10:42:34PM +0200, Jim Meyering wrote:
"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.
I'll see about adding a suitable macro for that.
+ } 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.
Yes, that needs a check for offset != NULL in the conditional before strstr.
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.
Yes, the entire set of Xen drivers is basically fubar wrt to checking virBufferXXX return values. I felt that needed addresing independantly changing everything in one go, so didn't try to address that in this patch.
+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.
That'd just mean I needed to cast the to const char * and back to xmlChar * in different places. eg all the xmlGetProp and xmlFree calls. Its pretty unpleasant either way around, so I kept the style the rest of the file uses. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|