[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



On Thu, Apr 24, 2008 at 10:42:34PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange redhat com> wrote:
> > +    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 defined a new convenience function STRPREFIX(str, prefix)
which applies strlen(prefix) so we avoid the hardcoding.
I didn't use sizeof(prefix) because someone might pass a
non-literal string as the prefix and thta would endup as
sizeof(char*) rather than the real length.

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

Fix this to chcek offset for NULL.

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

As discussed, posted new API to make virBuffer* safer.

Regards,
Daniel
-- 
|: 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 :|


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