[libvirt] [PATCH 0/6 v2] xen: parsing and sexpr escaping fixes

This series fixes some xen XM parsing and sexpr generation issues. The first three patches fix parsing /etc/xen files that use python style triple quotes (which libvirt will actually generate in certain situations). The last three patches fix generating xend sexpr with the reserved characters & \ or ' v2: Tweak domain schema path usage Cleanup virBufferEscape* Small improvements in xend sexpr formatting More sensible values in sexpr escape tests Cole Robinson (6): schemas: domain: Add more valid file path chars conf: Convert ParseString to use STRPREFIX conf: Fix parsing python style triple quotes xend: urlencode: Properly escape '&' buf: Simplify virBufferEscapeString xend: Escape reserved sexpr characters docs/schemas/domain.rng | 23 ++---- src/util/buf.c | 79 ++++++++++++++------ src/util/buf.h | 1 + src/util/conf.c | 20 +++-- src/xen/xend_internal.c | 113 +++++++++++++++------------ tests/xmconfigdata/test-escape-paths.cfg | 2 +- tests/xmconfigdata/test-escape-paths.xml | 5 + tests/xml2sexprdata/xml2sexpr-escape.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-escape.xml | 24 ++++++ tests/xml2sexprtest.c | 1 + 10 files changed, 172 insertions(+), 97 deletions(-) create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.xml -- 1.7.3.2

Also, standardize path usage on 'filePath' and 'absFilePath' Signed-off-by: Cole Robinson <crobinso@redhat.com> --- docs/schemas/domain.rng | 23 +++++++++-------------- 1 files changed, 9 insertions(+), 14 deletions(-) diff --git a/docs/schemas/domain.rng b/docs/schemas/domain.rng index bbbc846..fb44335 100644 --- a/docs/schemas/domain.rng +++ b/docs/schemas/domain.rng @@ -497,7 +497,7 @@ </optional> <optional> <element name="root"> - <ref name="devicePath"/> + <ref name="absFilePath"/> </element> </optional> <optional> @@ -588,7 +588,7 @@ <optional> <element name="source"> <attribute name="dev"> - <ref name="deviceName"/> + <ref name="absFilePath"/> </attribute> <empty/> </element> @@ -766,7 +766,7 @@ <interleave> <element name="source"> <attribute name="dev"> - <ref name="deviceName"/> + <ref name="absFilePath"/> </attribute> <empty/> </element> @@ -1338,7 +1338,7 @@ <ref name="qemucdevSrcType"/> <optional> <attribute name="tty"> - <ref name="devicePath"/> + <ref name="absFilePath"/> </attribute> </optional> <interleave> @@ -1430,7 +1430,7 @@ <group> <optional> <attribute name="tty"> - <ref name="devicePath"/> + <ref name="absFilePath"/> </attribute> </optional> <empty/> @@ -2028,27 +2028,22 @@ </define> <define name="filePath"> <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.\+\-&/%]+</param> + <param name="pattern">[a-zA-Z0-9_\.\+\-\\&"'<>/%]+</param> </data> </define> <define name="absFilePath"> <data type="string"> - <param name="pattern">/[a-zA-Z0-9_\.\+\-&/%]+</param> + <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]+</param> </data> </define> <define name="absDirPath"> <data type="string"> - <param name="pattern">/[a-zA-Z0-9_\.\+\-&/%]*</param> - </data> - </define> - <define name="devicePath"> - <data type="string"> - <param name="pattern">/[a-zA-Z0-9_\+\-/%]+</param> + <param name="pattern">/[a-zA-Z0-9_\.\+\-\\&"'<>/%]*</param> </data> </define> <define name="deviceName"> <data type="string"> - <param name="pattern">[a-zA-Z0-9_\.\-:/]+</param> + <param name="pattern">[a-zA-Z0-9_\.\-\\:/]+</param> </data> </define> <define name="bridgeMode"> -- 1.7.3.2

Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/conf.c | 19 +++++++++++-------- 1 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index ba1a384..a31bbc4 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -394,17 +394,20 @@ virConfParseString(virConfParserCtxtPtr ctxt) return NULL; } NEXT; - } else if ((ctxt->cur + 6 < ctxt->end) && (ctxt->cur[0] == '"') && - (ctxt->cur[1] == '"') && (ctxt->cur[2] == '"')) { + } else if ((ctxt->cur + 6 < ctxt->end) && + (STRPREFIX(ctxt->cur, "\"\"\""))) { + /* String starts with python-style triple quotes """ */ ctxt->cur += 3; base = ctxt->cur; - while ((ctxt->cur + 2 < ctxt->end) && (ctxt->cur[0] == '"') && - (ctxt->cur[1] == '"') && (ctxt->cur[2] == '"')) { - if (CUR == '\n') ctxt->line++; - NEXT; + + while ((ctxt->cur + 2 < ctxt->end) && + (STRPREFIX(ctxt->cur, "\"\"\""))) { + if (CUR == '\n') + ctxt->line++; + NEXT; } - if ((ctxt->cur[0] != '"') || (ctxt->cur[1] != '"') || - (ctxt->cur[2] != '"')) { + + if (!STRPREFIX(ctxt->cur, "\"\"\"")) { virConfError(ctxt, VIR_ERR_CONF_SYNTAX, _("unterminated string")); return(NULL); } -- 1.7.3.2

An incorrect check broke matching the closing set of quotes. Update tests to cover this case for XM config files. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/conf.c | 3 ++- tests/xmconfigdata/test-escape-paths.cfg | 2 +- tests/xmconfigdata/test-escape-paths.xml | 5 +++++ 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/util/conf.c b/src/util/conf.c index a31bbc4..d9a7603 100644 --- a/src/util/conf.c +++ b/src/util/conf.c @@ -400,8 +400,9 @@ virConfParseString(virConfParserCtxtPtr ctxt) ctxt->cur += 3; base = ctxt->cur; + /* Find the ending triple quotes */ while ((ctxt->cur + 2 < ctxt->end) && - (STRPREFIX(ctxt->cur, "\"\"\""))) { + !(STRPREFIX(ctxt->cur, "\"\"\""))) { if (CUR == '\n') ctxt->line++; NEXT; diff --git a/tests/xmconfigdata/test-escape-paths.cfg b/tests/xmconfigdata/test-escape-paths.cfg index f9f2cb8..e3e6db9 100644 --- a/tests/xmconfigdata/test-escape-paths.cfg +++ b/tests/xmconfigdata/test-escape-paths.cfg @@ -19,7 +19,7 @@ vnc = 1 vncunused = 1 vnclisten = "127.0.0.1" vncpasswd = "123poi" -disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso&test,hdc:cdrom,r" ] +disk = [ "phy:/dev/HostVG/XenGuest2,hda,w", "file:/root/boot.iso&test,hdc:cdrom,r", """phy:/dev/HostVG/XenGuest'",hdb,w""" ] vif = [ "mac=00:16:3e:66:92:9c,bridge=xenbr1,script=vif-bridge,model=e1000,type=ioemu" ] parallel = "none" serial = "none" diff --git a/tests/xmconfigdata/test-escape-paths.xml b/tests/xmconfigdata/test-escape-paths.xml index dabf492..13e6e29 100644 --- a/tests/xmconfigdata/test-escape-paths.xml +++ b/tests/xmconfigdata/test-escape-paths.xml @@ -31,6 +31,11 @@ <target dev='hdc' bus='ide'/> <readonly/> </disk> + <disk type='block' device='disk'> + <driver name='phy'/> + <source dev='/dev/HostVG/XenGuest'"'/> + <target dev='hdb' bus='ide'/> + </disk> <interface type='bridge'> <mac address='00:16:3e:66:92:9c'/> <source bridge='xenbr1'/> -- 1.7.3.2

Since we send the sexpr to xend via HTTP, we need to properly escape '&' Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/xen/xend_internal.c | 1 + 1 files changed, 1 insertions(+), 0 deletions(-) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d6d66bd..3ccadde 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -710,6 +710,7 @@ urlencode(const char *string) switch (string[i]) { case ' ': case '\n': + case '&': snprintf(ptr, 4, "%%%02x", string[i]); ptr += 3; break; -- 1.7.3.2

We are about to copy this function, so clean it up before we do. Signed-off-by: Cole Robinson <crobinso@redhat.com> --- src/util/buf.c | 38 +++++++------------------------------- 1 files changed, 7 insertions(+), 31 deletions(-) diff --git a/src/util/buf.c b/src/util/buf.c index 553e2a0..702bb10 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -282,7 +282,7 @@ err: void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str) { - int size, count, len, grow_size; + int len; char *escaped, *out; const char *cur; @@ -293,6 +293,11 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st return; len = strlen(str); + if (strcspn(str, "<>&'\"") == len) { + virBufferVSprintf(buf, format, str); + return; + } + if (VIR_ALLOC_N(escaped, 6 * len + 1) < 0) { virBufferNoMemory(buf); return; @@ -345,36 +350,7 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st } *out = 0; - if ((buf->use >= buf->size) && - virBufferGrow(buf, 100) < 0) { - goto err; - } - - size = buf->size - buf->use; - if ((count = snprintf(&buf->content[buf->use], size, - format, (char *)escaped)) < 0) { - buf->error = 1; - goto err; - } - - /* Grow buffer if necessary and retry */ - if (count >= size) { - buf->content[buf->use] = 0; - grow_size = (count + 1 > 1000) ? count + 1 : 1000; - if (virBufferGrow(buf, grow_size) < 0) { - goto err; - } - size = buf->size - buf->use; - - if ((count = snprintf(&buf->content[buf->use], size, - format, (char *)escaped)) < 0) { - buf->error = 1; - goto err; - } - } - buf->use += count; - -err: + virBufferVSprintf(buf, format, escaped); VIR_FREE(escaped); } -- 1.7.3.2

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@redhat.com> --- src/util/buf.c | 55 ++++++++++++++ src/util/buf.h | 1 + src/xen/xend_internal.c | 112 +++++++++++++++------------ tests/xml2sexprdata/xml2sexpr-escape.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-escape.xml | 24 ++++++ tests/xml2sexprtest.c | 1 + 6 files changed, 144 insertions(+), 50 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 702bb10..7557ad1 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -355,6 +355,61 @@ virBufferEscapeString(const virBufferPtr buf, const char *format, const char *st } /** + * 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 len; + char *escaped, *out; + const char *cur; + + if ((format == NULL) || (buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + len = strlen(str); + if (strcspn(str, "\\'") == len) { + virBufferVSprintf(buf, format, str); + return; + } + + if (VIR_ALLOC_N(escaped, 2 * len + 1) < 0) { + virBufferNoMemory(buf); + return; + } + + cur = str; + out = escaped; + while (*cur != 0) { + switch (*cur) { + case '\\': + case '\'': + *out++ = '\\'; + /* fallthrough */ + default: + *out++ = *cur; + } + cur++; + } + *out = 0; + + virBufferVSprintf(buf, format, escaped); + VIR_FREE(escaped); +} + +/** * virBufferURIEncodeString: * @buf: the buffer to append to * @str: the string argument which will be URI-encoded diff --git a/src/util/buf.h b/src/util/buf.h index 6616898..54f4de5 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -45,6 +45,7 @@ void virBufferVSprintf(const virBufferPtr buf, const char *format, ...) void virBufferStrcat(const virBufferPtr buf, ...) ATTRIBUTE_SENTINEL; void virBufferEscapeString(const virBufferPtr buf, const char *format, const char *str); +void virBufferEscapeSexpr(const virBufferPtr buf, const char *format, const char *str); void virBufferURIEncodeString (const virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 3ccadde..8b7c4d3 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -522,6 +522,7 @@ xend_op_ext(virConnectPtr xend, const char *path, const char *key, va_list ap) } content = virBufferContentAndReset(&buf); + DEBUG("xend op: %s\n", content); ret = http2unix(xend_post(xend, path, content)); VIR_FREE(content); @@ -4605,8 +4606,6 @@ virDomainPtr xenDaemonDomainDefineXML(virConnectPtr conn, const char *xmlDesc) { goto error; } - DEBUG("Defining w/ sexpr: \n%s", sexpr); - ret = xend_op(conn, "", "op", "new", "config", sexpr, NULL); VIR_FREE(sexpr); if (ret != 0) { @@ -5297,11 +5296,12 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, case VIR_DOMAIN_CHR_TYPE_FILE: case VIR_DOMAIN_CHR_TYPE_PIPE: - virBufferVSprintf(buf, "%s:%s", type, def->data.file.path); + virBufferVSprintf(buf, "%s:", type); + virBufferEscapeSexpr(buf, "%s", def->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_DEV: - virBufferVSprintf(buf, "%s", def->data.file.path); + virBufferEscapeSexpr(buf, "%s", def->data.file.path); break; case VIR_DOMAIN_CHR_TYPE_TCP: @@ -5322,8 +5322,9 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, break; case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferVSprintf(buf, "%s:%s%s", type, - def->data.nix.path, + virBufferVSprintf(buf, "%s:", type); + virBufferEscapeSexpr(buf, "%s", def->data.nix.path); + virBufferVSprintf(buf, "%s", def->data.nix.listen ? ",server,nowait" : ""); break; } @@ -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, + if (xendConfigVersion == 1) { + virBufferEscapeSexpr(buf, "(dev 'ioemu:%s')", def->dst); + } else { + /* But newer does not */ + virBufferEscapeSexpr(buf, "(dev '%s:", def->dst); + virBufferVSprintf(buf, "%s')", def->device == VIR_DOMAIN_DISK_DEVICE_CDROM ? "cdrom" : "disk"); + } } else if (def->device == VIR_DOMAIN_DISK_DEVICE_CDROM) { - virBufferVSprintf(buf, "(dev '%s:cdrom')", def->dst); + virBufferEscapeSexpr(buf, "(dev '%s:cdrom')", def->dst); } else { - virBufferVSprintf(buf, "(dev '%s')", def->dst); + virBufferEscapeSexpr(buf, "(dev '%s')", def->dst); } if (def->src) { if (def->driverName) { if (STREQ(def->driverName, "tap") || STREQ(def->driverName, "tap2")) { - virBufferVSprintf(buf, "(uname '%s:%s:%s')", - def->driverName, - def->driverType ? def->driverType : "aio", - def->src); + virBufferEscapeSexpr(buf, "(uname '%s:", def->driverName); + virBufferEscapeSexpr(buf, "%s:", + def->driverType ? def->driverType : "aio"); + virBufferEscapeSexpr(buf, "%s')", def->src); } else { - virBufferVSprintf(buf, "(uname '%s:%s')", - def->driverName, - def->src); + virBufferEscapeSexpr(buf, "(uname '%s:", def->driverName); + virBufferEscapeSexpr(buf, "%s')", def->src); } } else { if (def->type == VIR_DOMAIN_DISK_TYPE_FILE) { - virBufferVSprintf(buf, "(uname 'file:%s')", def->src); + virBufferEscapeSexpr(buf, "(uname 'file:%s')", def->src); } else if (def->type == VIR_DOMAIN_DISK_TYPE_BLOCK) { if (def->src[0] == '/') - virBufferVSprintf(buf, "(uname 'phy:%s')", def->src); + virBufferEscapeSexpr(buf, "(uname 'phy:%s')", def->src); else - virBufferVSprintf(buf, "(uname 'phy:/dev/%s')", def->src); + virBufferEscapeSexpr(buf, "(uname 'phy:/dev/%s')", + def->src); } else { virXendError(VIR_ERR_CONFIG_UNSUPPORTED, _("unsupported disk type %s"), @@ -5501,13 +5505,13 @@ xenDaemonFormatSxprNet(virConnectPtr conn, switch (def->type) { case VIR_DOMAIN_NET_TYPE_BRIDGE: - virBufferVSprintf(buf, "(bridge '%s')", def->data.bridge.brname); + virBufferEscapeSexpr(buf, "(bridge '%s')", def->data.bridge.brname); if (def->data.bridge.script) script = def->data.bridge.script; - virBufferVSprintf(buf, "(script '%s')", script); + virBufferEscapeSexpr(buf, "(script '%s')", script); if (def->data.bridge.ipaddr != NULL) - virBufferVSprintf(buf, "(ip '%s')", def->data.bridge.ipaddr); + virBufferEscapeSexpr(buf, "(ip '%s')", def->data.bridge.ipaddr); break; case VIR_DOMAIN_NET_TYPE_NETWORK: @@ -5530,17 +5534,18 @@ xenDaemonFormatSxprNet(virConnectPtr conn, def->data.network.name); return -1; } - virBufferVSprintf(buf, "(bridge '%s')", bridge); - virBufferVSprintf(buf, "(script '%s')", script); + virBufferEscapeSexpr(buf, "(bridge '%s')", bridge); + virBufferEscapeSexpr(buf, "(script '%s')", script); VIR_FREE(bridge); } break; case VIR_DOMAIN_NET_TYPE_ETHERNET: if (def->data.ethernet.script) - virBufferVSprintf(buf, "(script '%s')", def->data.ethernet.script); + virBufferEscapeSexpr(buf, "(script '%s')", + def->data.ethernet.script); if (def->data.ethernet.ipaddr != NULL) - virBufferVSprintf(buf, "(ip '%s')", def->data.ethernet.ipaddr); + virBufferEscapeSexpr(buf, "(ip '%s')", def->data.ethernet.ipaddr); break; case VIR_DOMAIN_NET_TYPE_USER: @@ -5555,11 +5560,11 @@ xenDaemonFormatSxprNet(virConnectPtr conn, if (def->ifname != NULL && !STRPREFIX(def->ifname, "vif")) - virBufferVSprintf(buf, "(vifname '%s')", def->ifname); + virBufferEscapeSexpr(buf, "(vifname '%s')", def->ifname); if (!hvm) { if (def->model != NULL) - virBufferVSprintf(buf, "(model '%s')", def->model); + virBufferEscapeSexpr(buf, "(model '%s')", def->model); } else if (def->model == NULL) { /* @@ -5573,7 +5578,7 @@ xenDaemonFormatSxprNet(virConnectPtr conn, virBufferAddLit(buf, "(type netfront)"); } else { - virBufferVSprintf(buf, "(model '%s')", def->model); + virBufferEscapeSexpr(buf, "(model '%s')", def->model); virBufferAddLit(buf, "(type ioemu)"); } @@ -5680,7 +5685,9 @@ xenDaemonFormatSxprSound(virDomainDefPtr def, def->sounds[i]->model); return -1; } - virBufferVSprintf(buf, "%s%s", i ? "," : "", str); + if (i) + virBufferAddChar(buf, ','); + virBufferEscapeSexpr(buf, "%s", str); } if (virBufferError(buf)) { @@ -5737,10 +5744,13 @@ xenDaemonFormatSxpr(virConnectPtr conn, virBuffer buf = VIR_BUFFER_INITIALIZER; char uuidstr[VIR_UUID_STRING_BUFLEN]; const char *tmp; + char *bufout; int hvm = 0, i; + DEBUG0("Formatting domain sexpr"); + virBufferAddLit(&buf, "(vm "); - virBufferVSprintf(&buf, "(name '%s')", def->name); + virBufferEscapeSexpr(&buf, "(name '%s')", def->name); virBufferVSprintf(&buf, "(memory %lu)(maxmem %lu)", def->mem.cur_balloon/1024, def->mem.max_balloon/1024); virBufferVSprintf(&buf, "(vcpus %u)", def->maxvcpus); @@ -5753,7 +5763,7 @@ xenDaemonFormatSxpr(virConnectPtr conn, char *ranges = virDomainCpuSetFormat(def->cpumask, def->cpumasklen); if (ranges == NULL) goto error; - virBufferVSprintf(&buf, "(cpus '%s')", ranges); + virBufferEscapeSexpr(&buf, "(cpus '%s')", ranges); VIR_FREE(ranges); } @@ -5761,16 +5771,16 @@ xenDaemonFormatSxpr(virConnectPtr conn, virBufferVSprintf(&buf, "(uuid '%s')", uuidstr); if (def->description) - virBufferVSprintf(&buf, "(description '%s')", def->description); + virBufferEscapeSexpr(&buf, "(description '%s')", def->description); if (def->os.bootloader) { if (def->os.bootloader[0]) - virBufferVSprintf(&buf, "(bootloader '%s')", def->os.bootloader); + virBufferEscapeSexpr(&buf, "(bootloader '%s')", def->os.bootloader); else virBufferAddLit(&buf, "(bootloader)"); if (def->os.bootloaderArgs) - virBufferVSprintf(&buf, "(bootloader_args '%s')", def->os.bootloaderArgs); + virBufferEscapeSexpr(&buf, "(bootloader_args '%s')", def->os.bootloaderArgs); } if (!(tmp = virDomainLifecycleTypeToString(def->onPoweroff))) { @@ -5827,20 +5837,20 @@ xenDaemonFormatSxpr(virConnectPtr conn, } if (def->os.kernel) - virBufferVSprintf(&buf, "(kernel '%s')", def->os.kernel); + virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.kernel); if (def->os.initrd) - virBufferVSprintf(&buf, "(ramdisk '%s')", def->os.initrd); + virBufferEscapeSexpr(&buf, "(ramdisk '%s')", def->os.initrd); if (def->os.root) - virBufferVSprintf(&buf, "(root '%s')", def->os.root); + virBufferEscapeSexpr(&buf, "(root '%s')", def->os.root); if (def->os.cmdline) - virBufferVSprintf(&buf, "(args '%s')", def->os.cmdline); + virBufferEscapeSexpr(&buf, "(args '%s')", def->os.cmdline); if (hvm) { char bootorder[VIR_DOMAIN_BOOT_LAST+1]; if (def->os.kernel) - virBufferVSprintf(&buf, "(loader '%s')", def->os.loader); + virBufferEscapeSexpr(&buf, "(loader '%s')", def->os.loader); else - virBufferVSprintf(&buf, "(kernel '%s')", def->os.loader); + virBufferEscapeSexpr(&buf, "(kernel '%s')", def->os.loader); virBufferVSprintf(&buf, "(vcpus %u)", def->maxvcpus); if (def->vcpus < def->maxvcpus) @@ -5883,14 +5893,14 @@ xenDaemonFormatSxpr(virConnectPtr conn, def->disks[i]->src == NULL) break; - virBufferVSprintf(&buf, "(cdrom '%s')", - def->disks[i]->src); + virBufferEscapeSexpr(&buf, "(cdrom '%s')", + def->disks[i]->src); break; case VIR_DOMAIN_DISK_DEVICE_FLOPPY: /* all xend versions define floppies here */ - virBufferVSprintf(&buf, "(%s '%s')", def->disks[i]->dst, - def->disks[i]->src); + virBufferEscapeSexpr(&buf, "(%s ", def->disks[i]->dst); + virBufferEscapeSexpr(&buf, "'%s')", def->disks[i]->src); break; default: @@ -5942,7 +5952,7 @@ xenDaemonFormatSxpr(virConnectPtr conn, /* get the device emulation model */ if (def->emulator && (hvm || xendConfigVersion >= 3)) - virBufferVSprintf(&buf, "(device_model '%s')", def->emulator); + virBufferEscapeSexpr(&buf, "(device_model '%s')", def->emulator); /* PV graphics for xen <= 3.0.4, or HVM graphics for xen <= 3.1.0 */ @@ -5986,7 +5996,9 @@ xenDaemonFormatSxpr(virConnectPtr conn, goto error; } - return virBufferContentAndReset(&buf); + bufout = virBufferContentAndReset(&buf); + DEBUG("Formatted sexpr: \n%s", bufout); + return bufout; error: virBufferFreeAndReset(&buf); diff --git a/tests/xml2sexprdata/xml2sexpr-escape.sexpr b/tests/xml2sexprdata/xml2sexpr-escape.sexpr new file mode 100644 index 0000000..c78d6a6 --- /dev/null +++ b/tests/xml2sexprdata/xml2sexpr-escape.sexpr @@ -0,0 +1 @@ +(vm (name 'fvtest')(memory 420)(maxmem 420)(vcpus 2)(uuid '596a5d21-71f4-8fb2-e068-e2386a5c413e')(on_poweroff 'destroy')(on_reboot 'destroy')(on_crash 'destroy')(image (hvm (kernel '/var/lib/xen/vmlinuz.2Dn2YT')(ramdisk '/var/lib/xen/initrd.img.0u-Vhq')(args ' method=http://download.fedora.devel.redhat.com/pub/fedora/linux/core/test/5.91/x86_64/os&version="devel" ')(loader '/usr/lib/xen/boot/hvmloader')(vcpus 2)(boot c)(usb 1)(parallel none)(serial pty)(device_model '/usr/lib/xen/bin/qemu-dm')))(device (vbd (dev 'ioemu:xvda')(uname 'file:/root/\'\\some.img')(mode 'w')))) \ No newline at end of file diff --git a/tests/xml2sexprdata/xml2sexpr-escape.xml b/tests/xml2sexprdata/xml2sexpr-escape.xml new file mode 100644 index 0000000..6eed578 --- /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&version="devel" </cmdline> + <loader>/usr/lib/xen/boot/hvmloader</loader> + </os> + <memory>430080</memory> + <vcpu>2</vcpu> + <on_poweroff>destroy</on_poweroff> + <on_reboot>destroy</on_reboot> + <on_crash>destroy</on_crash> + <devices> + <emulator>/usr/lib/xen/bin/qemu-dm</emulator> + <disk type='file' device='disk'> + <source file='/root/'\some.img'/> + <target dev='xvda'/> + </disk> + <console tty='/dev/pts/4'/> + </devices> +</domain> diff --git a/tests/xml2sexprtest.c b/tests/xml2sexprtest.c index 9cf8d39..8a5d115 100644 --- a/tests/xml2sexprtest.c +++ b/tests/xml2sexprtest.c @@ -163,6 +163,7 @@ mymain(int argc, char **argv) DO_TEST("fv-net-netfront", "fv-net-netfront", "fvtest", 1); DO_TEST("boot-grub", "boot-grub", "fvtest", 1); + DO_TEST("escape", "escape", "fvtest", 1); virCapabilitiesFree(caps); -- 1.7.3.2

On 11/22/2010 10:28 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@redhat.com> --- src/util/buf.c | 55 ++++++++++++++ src/util/buf.h | 1 + src/xen/xend_internal.c | 112 +++++++++++++++------------ tests/xml2sexprdata/xml2sexpr-escape.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-escape.xml | 24 ++++++ tests/xml2sexprtest.c | 1 + 6 files changed, 144 insertions(+), 50 deletions(-) create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.xml
ACK series, with two nits: You also need to update src/libvirt_private.syms: diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms index 9ed05df..edbc4bc 100644 --- i/src/libvirt_private.syms +++ w/src/libvirt_private.syms @@ -23,6 +23,7 @@ virBufferAdd; virBufferAddChar; virBufferContentAndReset; virBufferError; +virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; virBufferStrcat;
@@ -5322,8 +5322,9 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, break;
case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferVSprintf(buf, "%s:%s%s", type, - def->data.nix.path, + virBufferVSprintf(buf, "%s:", type); + virBufferEscapeSexpr(buf, "%s", def->data.nix.path); + virBufferVSprintf(buf, "%s", def->data.nix.listen ? ",server,nowait" : "");
Change that last line to: if (def->data.nix.listen) virBufferAddLit(buf, ",server,nowait");
+++ 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&version="devel" </cmdline>
Yep, definitely makes more sense for a use of '&' in a URL. -- Eric Blake eblake@redhat.com +1-801-349-2682 Libvirt virtualization library http://libvirt.org

On 11/22/2010 01:08 PM, Eric Blake wrote:
On 11/22/2010 10:28 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@redhat.com> --- src/util/buf.c | 55 ++++++++++++++ src/util/buf.h | 1 + src/xen/xend_internal.c | 112 +++++++++++++++------------ tests/xml2sexprdata/xml2sexpr-escape.sexpr | 1 + tests/xml2sexprdata/xml2sexpr-escape.xml | 24 ++++++ tests/xml2sexprtest.c | 1 + 6 files changed, 144 insertions(+), 50 deletions(-) create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.sexpr create mode 100644 tests/xml2sexprdata/xml2sexpr-escape.xml
ACK series, with two nits:
You also need to update src/libvirt_private.syms:
diff --git i/src/libvirt_private.syms w/src/libvirt_private.syms index 9ed05df..edbc4bc 100644 --- i/src/libvirt_private.syms +++ w/src/libvirt_private.syms @@ -23,6 +23,7 @@ virBufferAdd; virBufferAddChar; virBufferContentAndReset; virBufferError; +virBufferEscapeSexpr; virBufferEscapeString; virBufferFreeAndReset; virBufferStrcat;
@@ -5322,8 +5322,9 @@ xenDaemonFormatSxprChr(virDomainChrDefPtr def, break;
case VIR_DOMAIN_CHR_TYPE_UNIX: - virBufferVSprintf(buf, "%s:%s%s", type, - def->data.nix.path, + virBufferVSprintf(buf, "%s:", type); + virBufferEscapeSexpr(buf, "%s", def->data.nix.path); + virBufferVSprintf(buf, "%s", def->data.nix.listen ? ",server,nowait" : "");
Change that last line to:
if (def->data.nix.listen) virBufferAddLit(buf, ",server,nowait");
Thanks, pushed with those changes. - Cole
participants (2)
-
Cole Robinson
-
Eric Blake