On Thu, Apr 24, 2008 at 10:42:34PM +0200, Jim Meyering wrote:
> "Daniel P. Berrange" <berrange@xxxxxxxxxx> 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 :|
--
Libvir-list mailing list
Libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list