Re: [libvirt] [PATCH 4/4] xend: Escape reserved sexpr characters

On 11/19/2010 09:15 AM, Cole Robinson wrote: > If we don't escape ' or \ xend can't parse the generated sexpr. This > might over apply the EscapeSexpr routine, but it shouldn't hurt. > > Signed-off-by: Cole Robinson <crobinso(a)redhat.com&gt; > --- > src/util/buf.c | 79 +++++++++++++++++++ > src/util/buf.h | 1 + > src/xen/xend_internal.c | 115 +++++++++++++++------------- > tests/xml2sexprdata/xml2sexpr-escape.sexpr | 1 + > tests/xml2sexprdata/xml2sexpr-escape.xml | 24 ++++++ > tests/xml2sexprtest.c | 1 + > 6 files changed, 169 insertions(+), 52 deletions(-) > create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.sexpr > create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.xml > > diff --git a/src/util/buf.c b/src/util/buf.c > index 553e2a0..90034ad 100644 > --- a/src/util/buf.c > +++ b/src/util/buf.c > @@ -379,6 +379,85 @@ err: > } > > /** > + * virBufferEscapeSexpr: > + * @buf: the buffer to dump > + * @format: a printf like format string but with only one %s parameter > + * @str: the string argument which need to be escaped > + * > + * Do a formatted print with a single string to an sexpr buffer. The string > + * is escaped to avoid generating a sexpr that xen will choke on. This > + * doesn't fully escape the sexpr, just enough for our code to work. > + */ > +void > +virBufferEscapeSexpr(const virBufferPtr buf, > + const char *format, > + const char *str) > +{ > + int size, count, len, grow_size; > + char *escaped, *out; > + const char *cur; > + > + if ((format == NULL) || (buf == NULL) || (str == NULL)) > + return; > + > + if (buf->error) > + return; > + > + len = strlen(str); Right here, is it worth adding: if (strcspn(str, "\\'") == len) { virBufferVSprintf(buf, format, str); return; } > + if (VIR_ALLOC_N(escaped, 2 * len + 1) < 0) { > + virBufferNoMemory(buf); > + return; > + } so as to avoid the alloc in the common case of nothing to escape? > + *out = 0; > + > + if ((buf->use >= buf->size) && > + virBufferGrow(buf, 100) < 0) { > + goto err; > + } That 100 looks wrong; shouldn't it instead be max(100,out-escaped)? For that matter, why do all this low level stuff; wouldn't it be easier after *out = 0 to just do: virBufferVSpring(buf, format, escaped); VIR_FREE(escaped); return; and leave all the resizing and snprintf stuff to the already written function?

> diff --git a/src/util/buf.h b/src/util/buf.h > index 6616898..54f4de5 100644 > --- a/src/util/buf.h > @@ -5400,39 +5401,42 @@ xenDaemonFormatSxprDisk(virConnectPtr conn ATTRIBUTE_UNUSED, > > if (hvm) { > /* Xend <= 3.0.2 wants a ioemu: prefix on devices for HVM */ > - if (xendConfigVersion == 1) > - virBufferVSprintf(buf, "(dev 'ioemu:%s')", def->dst); > - else /* But newer does not */ > - virBufferVSprintf(buf, "(dev '%s:%s')", def->dst, > - def->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? > - "cdrom" : "disk"); > + if (xendConfigVersion == 1) { > + virBufferEscapeSexpr(buf, "(dev 'ioemu:%s')", def->dst); > + } else { > + /* But newer does not */ > + virBufferEscapeSexpr(buf, "(dev '%s:", def->dst); > + virBufferEscapeSexpr(buf, "%s')", > + def->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? > + "cdrom" : "disk"); This can be virBufferVSprintf(buf, "%s')",...), given that the only two possible strings for %s don't need escaping. > @@ -5680,7 +5685,8 @@ xenDaemonFormatSxprSound(virDomainDefPtr def, > def->sounds[i]->model); > return -1; > } > - virBufferVSprintf(buf, "%s%s", i ? "," : "", str); > + virBufferVSprintf(buf, "%s", i ? "," : ""); I'd write this as 'if (i) virBufferAddChar(buf, ',')' rather than a complicated VSprintf.

> diff --git a/tests/xml2sexprdata/xml2sexpr-escape.xml b/tests/xml2sexprdata/xml2sexpr-escape.xml > new file mode 100644 > index 0000000..73beb6b > --- /dev/null > +++ b/tests/xml2sexprdata/xml2sexpr-escape.xml > @@ -0,0 +1,24 @@ > +<domain type='xen' id='15'> > + <name>fvtest</name> > + <uuid>596a5d2171f48fb2e068e2386a5c413e</uuid> > + <os> > + <type>hvm</type> > + <kernel>/var/lib/xen/vmlinuz.2Dn2YT</kernel> > + <initrd>/var/lib/xen/initrd.img.0u-Vhq</initrd> > + <cmdline> method=http://&amp;""download.fedora.devel.redhat.com/pub/f... </cmdline> Seems unusual to have & and " in a URL; but the point of this test was not so much a real configuration as a way to expose the sexpr escaping code path. Is there any better place to stick this where it won't be quite so confusing?