[libvirt] [PATCHv2] xml: print uuids or shell-escaped names in the warning

In the XML warning, this prints uuids for domain names with special characters in them and shell-escaped names for other elements (like snapshosts and networks) with special names. --- When saving snapshots, the domain name is appended to the "snapshot-edit" command, so using a domain name that needs escaping would lead to something that can't be just fed to the shell as it would glue them together. diff to xml: shell-escape domain name in comment: - Domain names don't get escaped, UUIDs are preferred. - The command gets escaped too. diff to v1: don't try to use CDATA (it doesn't belong there) --- src/conf/domain_conf.c | 6 +++- src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 3 +- src/util/buf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/buf.h | 1 + src/util/xml.c | 62 ++++++++++++++++++++++--------------------- src/util/xml.h | 1 + 7 files changed, 109 insertions(+), 32 deletions(-) diff --git a/src/conf/domain_conf.c b/src/conf/domain_conf.c index 0933c08..a8cadeb 100644 --- a/src/conf/domain_conf.c +++ b/src/conf/domain_conf.c @@ -14252,6 +14252,7 @@ int virDomainSaveXML(const char *configDir, virDomainDefPtr def, const char *xml) { + char uuidstr[VIR_UUID_STRING_BUFLEN]; char *configFile = NULL; int ret = -1; @@ -14265,7 +14266,10 @@ int virDomainSaveXML(const char *configDir, goto cleanup; } - ret = virXMLSaveFile(configFile, def->name, "edit", xml); + virUUIDFormat(def->uuid, uuidstr); + ret = virXMLSaveFile(configFile, + virXMLPickShellSafeComment(def->name,uuidstr), "edit", + xml); cleanup: VIR_FREE(configFile); diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 699c9a3..b17ef88 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -37,6 +37,7 @@ virBufferError; virBufferEscape; virBufferEscapeSexpr; virBufferEscapeShell; +virBufferEscapeShellXMLComment; virBufferEscapeString; virBufferFreeAndReset; virBufferGetIndent; @@ -1816,6 +1817,7 @@ virURIParse; # xml.h virXMLChildElementCount; virXMLParseHelper; +virXMLPickShellSafeComment; virXMLPropString; virXMLSaveFile; virXPathBoolean; diff --git a/src/qemu/qemu_domain.c b/src/qemu/qemu_domain.c index 4e6a5e9..e4ef7e6 100644 --- a/src/qemu/qemu_domain.c +++ b/src/qemu/qemu_domain.c @@ -1622,7 +1622,8 @@ qemuDomainSnapshotWriteMetadata(virDomainObjPtr vm, goto cleanup; } - if (virAsprintf(&tmp, "snapshot-edit %s", vm->def->name) < 0) { + if (virAsprintf(&tmp, "snapshot-edit %s", + virXMLPickShellSafeComment(vm->def->name, uuidstr)) < 0) { virReportOOMError(); goto cleanup; } diff --git a/src/util/buf.c b/src/util/buf.c index 030dc97..a326465 100644 --- a/src/util/buf.c +++ b/src/util/buf.c @@ -623,6 +623,72 @@ virBufferEscapeShell(virBufferPtr buf, const char *str) } /** + * virBufferEscapeShellXMLComment: + * @buf: the buffer to append to + * @str: an unquoted string + * + * Quotes a string so that the shell (/bin/sh) will interpret the + * quoted string to mean str. Auto indentation may be applied. + * It also escapes any "--" so that the string can be used in XML comments. + */ +void virBufferEscapeShellXMLComment(virBufferPtr buf, const char *str) +{ + int len; + char *escaped, *out; + const char *cur; + char prev = '\0'; + + if ((buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + /* Only quote if str includes shell metacharacters. */ + if (*str && !strpbrk(str, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") && + !strstr(str, "--")) { + virBufferAdd(buf, str, -1); + return; + } + + if (*str) { + len = strlen(str); + if (xalloc_oversized(4, len) || + VIR_ALLOC_N(escaped, 4 * len + 3) < 0) { + virBufferSetError(buf, errno); + return; + } + } else { + virBufferAddChar(buf, '\''); + return; + } + + cur = str; + out = escaped; + + *out++ = '\''; + while (*cur != 0) { + if (*cur == '\'') { + *out++ = '\''; + /* Replace literal ' with a close ', a \', and a open ' */ + *out++ = '\\'; + *out++ = '\''; + } else if (prev == '-' && *cur == '-') { + /* Replace -- with -''- */ + *out++ = '\''; + *out++ = '\''; + } + prev = *cur; + *out++ = *cur++; + } + *out++ = '\''; + *out = 0; + + virBufferAdd(buf, escaped, -1); + VIR_FREE(escaped); +} + +/** * virBufferStrcat: * @buf: the buffer to append to * @...: the variable list of strings, the last argument must be NULL diff --git a/src/util/buf.h b/src/util/buf.h index c3a498d..25e31fd 100644 --- a/src/util/buf.h +++ b/src/util/buf.h @@ -69,6 +69,7 @@ void virBufferEscapeString(virBufferPtr buf, const char *format, void virBufferEscapeSexpr(virBufferPtr buf, const char *format, const char *str); void virBufferEscapeShell(virBufferPtr buf, const char *str); +void virBufferEscapeShellXMLComment(virBufferPtr buf, const char *str); void virBufferURIEncodeString(virBufferPtr buf, const char *str); # define virBufferAddLit(buf_, literal_string_) \ diff --git a/src/util/xml.c b/src/util/xml.c index f3dc256..01507a9 100644 --- a/src/util/xml.c +++ b/src/util/xml.c @@ -780,49 +780,51 @@ error: goto cleanup; } +const char *virXMLPickShellSafeComment(const char *str1, const char *str2) +{ + if(str1 && !strpbrk(str1, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") && + !strstr(str1, "--")) + return str1; + if(str2 && !strpbrk(str2, "\r\t\n !\"#$&'()*;<>?[\\]^`{|}~") && + !strstr(str2, "--")) + return str2; + return NULL; +} static int virXMLEmitWarning(int fd, const char *name, const char *cmd) { + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *str = NULL; size_t len; - const char *prologue = "<!--\n\ -WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE \n\ -OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:\n\ - virsh "; - const char *epilogue = "\n\ -or other application using the libvirt API.\n\ --->\n\n"; if (fd < 0 || !name || !cmd) { errno = EINVAL; return -1; } - len = strlen(prologue); - if (safewrite(fd, prologue, len) != len) - return -1; - - len = strlen(cmd); - if (safewrite(fd, cmd, len) != len) - return -1; - - /* Omit the domain name if it contains a double hyphen - * because they aren't allowed in XML comments */ - if (!strstr(name, "--")) { - if (safewrite(fd, " ", 1) != 1) - return -1; - - len = strlen(name); - if (safewrite(fd, name, len) != len) - return -1; - } - - len = strlen(epilogue); - if (safewrite(fd, epilogue, len) != len) - return -1; + virBufferAddLit(&buf, "<!--\n" +"WARNING: THIS IS AN AUTO-GENERATED FILE. CHANGES TO IT ARE LIKELY TO BE\n" +"OVERWRITTEN AND LOST. Changes to this xml configuration should be made using:\n" +" virsh "); + virBufferEscapeShellXMLComment(&buf, cmd); + virBufferAddChar(&buf,' '); + virBufferEscapeShellXMLComment(&buf, name); + virBufferAddLit(&buf, "\n" +"or other application using the libvirt API.\n" +"-->\n\n"); + + str = virBufferContentAndReset(&buf); + len = strlen(str); + if (str) + if (safewrite(fd, str, len) == len) { + VIR_FREE(str); + return 0; + } - return 0; + VIR_FREE(str); + return -1; } diff --git a/src/util/xml.h b/src/util/xml.h index a3750fa..c3b05df 100644 --- a/src/util/xml.h +++ b/src/util/xml.h @@ -61,6 +61,7 @@ xmlDocPtr virXMLParseHelper(int domcode, const char *url, xmlXPathContextPtr *pctxt); +const char *virXMLPickShellSafeComment(const char *str1, const char *str2); /** * virXMLParse: * @filename: file to parse, or NULL for string parsing -- 1.7.8.6

On Thu, Oct 25, 2012 at 16:37:37 +0200, Ján Tomko wrote:
In the XML warning, this prints uuids for domain names with special characters in them and shell-escaped names for other elements (like snapshosts and networks) with special names. --- When saving snapshots, the domain name is appended to the "snapshot-edit" command, so using a domain name that needs escaping would lead to something that can't be just fed to the shell as it would glue them together.
diff to xml: shell-escape domain name in comment: - Domain names don't get escaped, UUIDs are preferred. - The command gets escaped too.
diff to v1: don't try to use CDATA (it doesn't belong there) --- src/conf/domain_conf.c | 6 +++- src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 3 +- src/util/buf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/buf.h | 1 + src/util/xml.c | 62 ++++++++++++++++++++++--------------------- src/util/xml.h | 1 + 7 files changed, 109 insertions(+), 32 deletions(-)
I think we're making this too complicated for no real benefit. The goal is to provide a hint to anyone who looks at the autogenerated XML files and IMHO providing an escaped string (that would only work in environments for which it was designed). I would just keep it simple: - emit "virsh command name" in case name is nice - emit "virsh command uuid" in case name is ugly and uuid is known - emit "virsh command" in all other cases This should keep the hints in domain and network XML files in /etc/libvirt usable for copy&paste (the UUID fallback works there). Snapshot files (located in /var/lib/libvirt) use "virsh snapshot-edit domain snapshot", where domains are passed as part of the command and snapshots have no UUIDs. Thus to keep the code simple, I'd emit just "virsh snapshot-edit", which is still a useful hint and I don't believe we need to do anything beyond that. Jirka

On 10/29/2012 10:38 AM, Jiri Denemark wrote:
On Thu, Oct 25, 2012 at 16:37:37 +0200, Ján Tomko wrote:
In the XML warning, this prints uuids for domain names with special characters in them and shell-escaped names for other elements (like snapshosts and networks) with special names. --- When saving snapshots, the domain name is appended to the "snapshot-edit" command, so using a domain name that needs escaping would lead to something that can't be just fed to the shell as it would glue them together.
diff to xml: shell-escape domain name in comment: - Domain names don't get escaped, UUIDs are preferred. - The command gets escaped too.
diff to v1: don't try to use CDATA (it doesn't belong there) --- src/conf/domain_conf.c | 6 +++- src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 3 +- src/util/buf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/buf.h | 1 + src/util/xml.c | 62 ++++++++++++++++++++++--------------------- src/util/xml.h | 1 + 7 files changed, 109 insertions(+), 32 deletions(-)
I think we're making this too complicated for no real benefit. The goal is to provide a hint to anyone who looks at the autogenerated XML files and IMHO providing an escaped string (that would only work in environments for which it was designed). I would just keep it simple:
- emit "virsh command name" in case name is nice - emit "virsh command uuid" in case name is ugly and uuid is known - emit "virsh command" in all other cases
This should keep the hints in domain and network XML files in /etc/libvirt usable for copy&paste (the UUID fallback works there). Snapshot files (located in /var/lib/libvirt) use "virsh snapshot-edit domain snapshot", where domains are passed as part of the command and snapshots have no UUIDs. Thus to keep the code simple, I'd emit just "virsh snapshot-edit", which is still a useful hint and I don't believe we need to do anything beyond that.
Since the comment is merely a warning for people that aren't used to the commands or don't know how libvirt works, I second that opinion. I myself believe there is no need for the whole command anyway, especially when getting to know how to specify the right command-line encourages the user to get to know virsh better. Martin

On 10/29/12 10:38, Jiri Denemark wrote:
On Thu, Oct 25, 2012 at 16:37:37 +0200, Ján Tomko wrote:
In the XML warning, this prints uuids for domain names with special characters in them and shell-escaped names for other elements (like snapshosts and networks) with special names. --- When saving snapshots, the domain name is appended to the "snapshot-edit" command, so using a domain name that needs escaping would lead to something that can't be just fed to the shell as it would glue them together.
diff to xml: shell-escape domain name in comment: - Domain names don't get escaped, UUIDs are preferred. - The command gets escaped too.
diff to v1: don't try to use CDATA (it doesn't belong there) --- src/conf/domain_conf.c | 6 +++- src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 3 +- src/util/buf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/buf.h | 1 + src/util/xml.c | 62 ++++++++++++++++++++++--------------------- src/util/xml.h | 1 + 7 files changed, 109 insertions(+), 32 deletions(-)
I think we're making this too complicated for no real benefit. The goal is to provide a hint to anyone who looks at the autogenerated XML files and IMHO providing an escaped string (that would only work in environments for which it was designed). I would just keep it simple:
- emit "virsh command name" in case name is nice - emit "virsh command uuid" in case name is ugly and uuid is known - emit "virsh command" in all other cases
This looks reasonable.
This should keep the hints in domain and network XML files in /etc/libvirt usable for copy&paste (the UUID fallback works there). Snapshot files (located in /var/lib/libvirt) use "virsh snapshot-edit domain snapshot", where domains are passed as part of the command and snapshots have no UUIDs. Thus to keep the code simple, I'd emit just "virsh snapshot-edit", which is still a useful hint and I don't believe we need to do anything beyond that.
Well, we might check if the names contain dashes, and then just print the line without the names. But that's probably not worth even trying. Putting the static hint should be good enough. I'd just put there tokens that the user should replace for real values: "virsh snapshot-edit 'domain-name' 'snapshot-name'" or something like that. A 109+/32- patch doesn't look worth of the usability improvement that is marginal. Peter

On Mon, Oct 29, 2012 at 11:07:20AM +0100, Peter Krempa wrote:
On 10/29/12 10:38, Jiri Denemark wrote:
On Thu, Oct 25, 2012 at 16:37:37 +0200, Ján Tomko wrote:
In the XML warning, this prints uuids for domain names with special characters in them and shell-escaped names for other elements (like snapshosts and networks) with special names. --- When saving snapshots, the domain name is appended to the "snapshot-edit" command, so using a domain name that needs escaping would lead to something that can't be just fed to the shell as it would glue them together.
diff to xml: shell-escape domain name in comment: - Domain names don't get escaped, UUIDs are preferred. - The command gets escaped too.
diff to v1: don't try to use CDATA (it doesn't belong there) --- src/conf/domain_conf.c | 6 +++- src/libvirt_private.syms | 2 + src/qemu/qemu_domain.c | 3 +- src/util/buf.c | 66 ++++++++++++++++++++++++++++++++++++++++++++++ src/util/buf.h | 1 + src/util/xml.c | 62 ++++++++++++++++++++++--------------------- src/util/xml.h | 1 + 7 files changed, 109 insertions(+), 32 deletions(-)
I think we're making this too complicated for no real benefit. The goal is to provide a hint to anyone who looks at the autogenerated XML files and IMHO providing an escaped string (that would only work in environments for which it was designed). I would just keep it simple:
- emit "virsh command name" in case name is nice - emit "virsh command uuid" in case name is ugly and uuid is known - emit "virsh command" in all other cases
This looks reasonable.
This should keep the hints in domain and network XML files in /etc/libvirt usable for copy&paste (the UUID fallback works there). Snapshot files (located in /var/lib/libvirt) use "virsh snapshot-edit domain snapshot", where domains are passed as part of the command and snapshots have no UUIDs. Thus to keep the code simple, I'd emit just "virsh snapshot-edit", which is still a useful hint and I don't believe we need to do anything beyond that.
Well, we might check if the names contain dashes, and then just print the line without the names. But that's probably not worth even trying. Putting the static hint should be good enough. I'd just put there tokens that the user should replace for real values:
"virsh snapshot-edit 'domain-name' 'snapshot-name'" or something like that.
I agree; I'm pretty confident that people will understand what's meant by that and the additional complexity of adding the actual object name seems excessive. Dave
A 109+/32- patch doesn't look worth of the usability improvement that is marginal.
Peter
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
participants (5)
-
Dave Allan
-
Jiri Denemark
-
Ján Tomko
-
Martin Kletzander
-
Peter Krempa