On 11/19/2010 03:57 PM, Eric Blake wrote:
> 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@xxxxxxxxxx>
>> ---
>> 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?
>
This function is basically a copy of virBufferEscapeString. I'll add a
patch to cleanup the original function before duplicating.
>> 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.
>
Okay, i'll fold in these changes.
>> 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://&""download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os </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?
>
Actually the BZ prompting this work was for a URL with & in it:
https://bugzilla.redhat.com/show_bug.cgi?id=472437
I'll format the URL in a more sensible way though.
Thanks,
Cole
--
libvir-list mailing list
libvir-list@xxxxxxxxxx
https://www.redhat.com/mailman/listinfo/libvir-list