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

[Daniel P. Berrange]

On Thu, Apr 24, 2008 at 10:42:34PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange at 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.

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