[libvirt] [PATCH 0/5] virstring and virbuffer improvements and bug fix
Pavel Hrdina (5): util: virstring: introduce virStrcat and VIR_STRCAT util: use VIR_STRCAT instead of strcat util: virbuffer: introduce virBufferEscapeN util: virqemu: introduce virQEMUBuildBufferEscape qemu: properly escape socket path for graphics cfg.mk | 16 ++-- src/libvirt_private.syms | 4 + src/qemu/qemu_command.c | 6 +- src/storage/storage_backend_logical.c | 6 +- src/test/test_driver.c | 2 +- src/util/virbuffer.c | 104 +++++++++++++++++++++ src/util/virbuffer.h | 2 + src/util/vircgroup.c | 4 +- src/util/virqemu.c | 17 ++++ src/util/virqemu.h | 1 + src/util/virstring.c | 70 ++++++++++++++ src/util/virstring.h | 27 ++++++ src/xen/xend_internal.c | 2 +- .../qemuxml2argvdata/qemuxml2argv-name-escape.args | 5 +- .../qemuxml2argvdata/qemuxml2argv-name-escape.xml | 7 +- tests/qemuxml2argvtest.c | 3 +- tests/virbuftest.c | 41 ++++++++ tests/virstringtest.c | 49 ++++++++++ 18 files changed, 345 insertions(+), 21 deletions(-) -- 2.11.1
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 2 +- src/libvirt_private.syms | 2 ++ src/util/virstring.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 27 +++++++++++++++++++ tests/virstringtest.c | 49 +++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 1 deletion(-) diff --git a/cfg.mk b/cfg.mk index aaba61f1dc..22c655eac6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1147,7 +1147,7 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$ exclude_file_name_regexp--sc_prohibit_internal_functions = \ - ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$ + ^src/(util/(viralloc|virutil|virfile|virstring)\.[hc]|esx/esx_vi\.c)$$ exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07a35333b1..e9c4d73779 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2502,6 +2502,8 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virStrcat; +virStrcatInplace; virStrcpy; virStrdup; virStringBufferIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 69abc267bf..bc15ce7e9e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -837,6 +837,76 @@ virStrndup(char **dest, } +/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * @inPlace: false if we should expand the allocated memory before moving, + * true if we should assume someone else has already done that. + * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strcat, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrcat is called from. Consider + * using VIR_STRCAT which sets these automatically. + * + * Returns: 0 for NULL src, 1 on successful concatenate, -1 otherwise. + */ +int +virStrcat(char **dest, + const char *src, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + size_t dest_len = 0; + size_t src_len = 0; + + if (!src) + return 0; + + if (*dest) + dest_len = strlen(*dest); + src_len = strlen(src); + + if (virReallocN(dest, sizeof(*dest), dest_len + src_len + 1, + report, domcode, filename, funcname, linenr) < 0) + return -1; + + strcat(*dest, src); + + return 1; +} + + +/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * + * Wrapper over strcat, which properly handles if @src is NULL. + * + * Returns: 0 for NULL src, 1 on successful concatenate. + */ +int +virStrcatInplace(char *dest, const char *src) +{ + if (!src) + return 0; + + strcat(dest, src); + + return 1; +} + + size_t virStringListLength(const char * const *strings) { size_t i = 0; diff --git a/src/util/virstring.h b/src/util/virstring.h index a5550e30d2..79d2f23c80 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -131,6 +131,13 @@ int virStrdup(char **dest, const char *src, bool report, int domcode, int virStrndup(char **dest, const char *src, ssize_t n, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); + +int virStrcat(char **dest, const char *src, bool report, int domcode, + const char *filename, const char *funcname, size_t linenr) + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); +int virStrcatInplace(char *dest, const char *src) + ATTRIBUTE_NONNULL(1); + int virAsprintfInternal(bool report, int domcode, const char *filename, const char *funcname, size_t linenr, char **strp, const char *fmt, ...) @@ -209,6 +216,26 @@ int virVasprintfInternal(bool report, int domcode, const char *filename, # define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \ 0, NULL, NULL, 0) +/** + * VIR_STRCAT: + * @dst: variable to hold result (char*, not char**) + * @src: string to concatenate to @dst + * + * Concatenate @src string into @dst string, @dst may be NULL. + * + * This macro is safe to use on arguments with side effects. + * + * VIR_STRCAT_INPLACE expect the @dst to be already allocated to hold + * the result. + * + * Returns -1 on failure, 0 if @src was NULL, 1 if @src was concatenated. + */ +# define VIR_STRCAT(dst, src) virStrcat(&(dst), src, true, VIR_FROM_THIS, \ + __FILE__, __FUNCTION__, __LINE__) +# define VIR_STRCAT_QUIET(dst, src) virStrcat(&(dst), src, false, 0, \ + NULL, NULL, 0) +# define VIR_STRCAT_INPLACE(dst, src) virStrcatInplace(dst, src) + size_t virStringListLength(const char * const *strings); /** diff --git a/tests/virstringtest.c b/tests/virstringtest.c index db1731f96a..d2d6138326 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -693,6 +693,37 @@ static int testStripControlChars(const void *args) return ret; } + +struct testStrcatData { + const char *dest; + const char *src; + const char *res; +}; + +static int +testStrcat(const void *args) +{ + const struct testStrcatData *data = args; + char *str = NULL; + int ret = -1; + + if (VIR_STRDUP(str, data->dest) < 0) + goto cleanup; + + if (VIR_STRCAT(str, data->src) < 0) + goto cleanup; + + if (!STREQ_NULLABLE(str, data->res)) + goto cleanup; + + ret = 0; + + cleanup: + VIR_FREE(str); + return ret; +} + + static int mymain(void) { @@ -958,6 +989,24 @@ mymain(void) TEST_STRIP_CONTROL_CHARS("\x01H\x02" "E\x03L\x04L\x05O", "HELLO"); TEST_STRIP_CONTROL_CHARS("\x01\x02\x03\x04HELL\x05O", "HELLO"); TEST_STRIP_CONTROL_CHARS("\nhello \x01\x07hello\t", "\nhello hello\t"); + +#define TEST_STRCAT(dests, srcs, ress) \ + do { \ + struct testStrcatData strcatData = { \ + .dest = dests, \ + .src = srcs, \ + .res = ress, \ + }; \ + if (virTestRun("Concatenate '" #dests "' with '" #srcs "'", \ + testStrcat, &strcatData) < 0) \ + ret = -1; \ + } while (0) + + TEST_STRCAT(NULL, NULL, NULL); + TEST_STRCAT(NULL, "world", "world"); + TEST_STRCAT("hello", NULL, "hello"); + TEST_STRCAT("hello", "world", "helloworld"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.11.1
On Thu, Feb 23, 2017 at 04:26:56PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 2 +- src/libvirt_private.syms | 2 ++ src/util/virstring.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 27 +++++++++++++++++++ tests/virstringtest.c | 49 +++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index aaba61f1dc..22c655eac6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1147,7 +1147,7 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
exclude_file_name_regexp--sc_prohibit_internal_functions = \ - ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$ + ^src/(util/(viralloc|virutil|virfile|virstring)\.[hc]|esx/esx_vi\.c)$$
exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07a35333b1..e9c4d73779 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2502,6 +2502,8 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virStrcat; +virStrcatInplace; virStrcpy; virStrdup; virStringBufferIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 69abc267bf..bc15ce7e9e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -837,6 +837,76 @@ virStrndup(char **dest, }
+/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * @inPlace: false if we should expand the allocated memory before moving, + * true if we should assume someone else has already done that.
This is here probably from some work in progress version.
+ * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strcat, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrcat is called from. Consider + * using VIR_STRCAT which sets these automatically. + * + * Returns: 0 for NULL src, 1 on successful concatenate, -1 otherwise. + */ +int +virStrcat(char **dest, + const char *src, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + size_t dest_len = 0; + size_t src_len = 0; + + if (!src) + return 0; + + if (*dest) + dest_len = strlen(*dest); + src_len = strlen(src); + + if (virReallocN(dest, sizeof(*dest), dest_len + src_len + 1, + report, domcode, filename, funcname, linenr) < 0) + return -1; + + strcat(*dest, src); + + return 1; +} + + +/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * + * Wrapper over strcat, which properly handles if @src is NULL. + * + * Returns: 0 for NULL src, 1 on successful concatenate. + */
Really? This whole wrapper just for checking NULL? So instead of: if (x) strcat (y, x) I should do: VIR_STRCAT_INPLACE(y, x) now? It's not even saving any characters. Plus, is there *really* any occurrence of strcat that might be called with NULL? I would say that such code has more logic problems in that case. The reallocating strcat makes sense, but the name is *really* confusing for people who are used to what strcat/strncat does. So while I see how that might be useful, we also have a buffer for more interesting dynamic string modifications and I'm not sure this is something that will help a lot.
On Thu, Feb 23, 2017 at 05:06:53PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 04:26:56PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 2 +- src/libvirt_private.syms | 2 ++ src/util/virstring.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 27 +++++++++++++++++++ tests/virstringtest.c | 49 +++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index aaba61f1dc..22c655eac6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1147,7 +1147,7 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
exclude_file_name_regexp--sc_prohibit_internal_functions = \ - ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$ + ^src/(util/(viralloc|virutil|virfile|virstring)\.[hc]|esx/esx_vi\.c)$$
exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07a35333b1..e9c4d73779 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2502,6 +2502,8 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virStrcat; +virStrcatInplace; virStrcpy; virStrdup; virStringBufferIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 69abc267bf..bc15ce7e9e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -837,6 +837,76 @@ virStrndup(char **dest, }
+/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * @inPlace: false if we should expand the allocated memory before moving, + * true if we should assume someone else has already done that.
This is here probably from some work in progress version.
+ * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strcat, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrcat is called from. Consider + * using VIR_STRCAT which sets these automatically. + * + * Returns: 0 for NULL src, 1 on successful concatenate, -1 otherwise. + */ +int +virStrcat(char **dest, + const char *src, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + size_t dest_len = 0; + size_t src_len = 0; + + if (!src) + return 0; + + if (*dest) + dest_len = strlen(*dest); + src_len = strlen(src); + + if (virReallocN(dest, sizeof(*dest), dest_len + src_len + 1, + report, domcode, filename, funcname, linenr) < 0) + return -1; + + strcat(*dest, src); + + return 1; +} + + +/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * + * Wrapper over strcat, which properly handles if @src is NULL. + * + * Returns: 0 for NULL src, 1 on successful concatenate. + */
Really? This whole wrapper just for checking NULL? So instead of:
if (x) strcat (y, x)
I should do:
VIR_STRCAT_INPLACE(y, x) now? It's not even saving any characters.
Plus, is there *really* any occurrence of strcat that might be called with NULL? I would say that such code has more logic problems in that case.
The reason is to forbid using strcat directly and force to use the wrappers. I had two options in my mind, one that will use the virStrcatInplace function with return values 0 and 1 to determine whether something actually happened or only the "if (x) strcat (y, x)" as a macro without any return value.
The reallocating strcat makes sense, but the name is *really* confusing for people who are used to what strcat/strncat does. So while I see how
In that case it would be nice to provide some alternative :).
that might be useful, we also have a buffer for more interesting dynamic string modifications and I'm not sure this is something that will help a lot.
Yes we have buffer, but this macro will be used in the buffer code itself and I guess it would be strange to use a buffer inside a buffer function. Pavel
On Thu, Feb 23, 2017 at 05:15:12PM +0100, Pavel Hrdina wrote:
On Thu, Feb 23, 2017 at 05:06:53PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 04:26:56PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 2 +- src/libvirt_private.syms | 2 ++ src/util/virstring.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 27 +++++++++++++++++++ tests/virstringtest.c | 49 +++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index aaba61f1dc..22c655eac6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1147,7 +1147,7 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
exclude_file_name_regexp--sc_prohibit_internal_functions = \ - ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$ + ^src/(util/(viralloc|virutil|virfile|virstring)\.[hc]|esx/esx_vi\.c)$$
exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07a35333b1..e9c4d73779 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2502,6 +2502,8 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virStrcat; +virStrcatInplace; virStrcpy; virStrdup; virStringBufferIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 69abc267bf..bc15ce7e9e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -837,6 +837,76 @@ virStrndup(char **dest, }
+/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * @inPlace: false if we should expand the allocated memory before moving, + * true if we should assume someone else has already done that.
This is here probably from some work in progress version.
+ * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strcat, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrcat is called from. Consider + * using VIR_STRCAT which sets these automatically. + * + * Returns: 0 for NULL src, 1 on successful concatenate, -1 otherwise. + */ +int +virStrcat(char **dest, + const char *src, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + size_t dest_len = 0; + size_t src_len = 0; + + if (!src) + return 0; + + if (*dest) + dest_len = strlen(*dest); + src_len = strlen(src); + + if (virReallocN(dest, sizeof(*dest), dest_len + src_len + 1, + report, domcode, filename, funcname, linenr) < 0) + return -1; + + strcat(*dest, src); + + return 1; +} + + +/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * + * Wrapper over strcat, which properly handles if @src is NULL. + * + * Returns: 0 for NULL src, 1 on successful concatenate. + */
Really? This whole wrapper just for checking NULL? So instead of:
if (x) strcat (y, x)
I should do:
VIR_STRCAT_INPLACE(y, x) now? It's not even saving any characters.
Plus, is there *really* any occurrence of strcat that might be called with NULL? I would say that such code has more logic problems in that case.
The reason is to forbid using strcat directly and force to use the wrappers. I had two options in my mind, one that will use the virStrcatInplace function with return values 0 and 1 to determine whether something actually happened or only the "if (x) strcat (y, x)" as a macro without any return value.
I get the idea, but my point was that I don't get why we should forbid using strcat() by itself.
The reallocating strcat makes sense, but the name is *really* confusing for people who are used to what strcat/strncat does. So while I see how
In that case it would be nice to provide some alternative :).
virStringAppend?
that might be useful, we also have a buffer for more interesting dynamic string modifications and I'm not sure this is something that will help a lot.
Yes we have buffer, but this macro will be used in the buffer code itself and I guess it would be strange to use a buffer inside a buffer function.
Since that will not be changing the input data, it might be safer to just sacrifice few bytes of memory and do virAsprintf() into new buffer. But if you like this approach more, feel free to use it.
Pavel
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 23, 2017 at 05:25:38PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 05:15:12PM +0100, Pavel Hrdina wrote:
On Thu, Feb 23, 2017 at 05:06:53PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 04:26:56PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 2 +- src/libvirt_private.syms | 2 ++ src/util/virstring.c | 70 ++++++++++++++++++++++++++++++++++++++++++++++++ src/util/virstring.h | 27 +++++++++++++++++++ tests/virstringtest.c | 49 +++++++++++++++++++++++++++++++++ 5 files changed, 149 insertions(+), 1 deletion(-)
diff --git a/cfg.mk b/cfg.mk index aaba61f1dc..22c655eac6 100644 --- a/cfg.mk +++ b/cfg.mk @@ -1147,7 +1147,7 @@ exclude_file_name_regexp--sc_prohibit_fork_wrappers = \ exclude_file_name_regexp--sc_prohibit_gethostname = ^src/util/virutil\.c$$
exclude_file_name_regexp--sc_prohibit_internal_functions = \ - ^src/(util/(viralloc|virutil|virfile)\.[hc]|esx/esx_vi\.c)$$ + ^src/(util/(viralloc|virutil|virfile|virstring)\.[hc]|esx/esx_vi\.c)$$
exclude_file_name_regexp--sc_prohibit_newline_at_end_of_diagnostic = \ ^src/rpc/gendispatch\.pl$$ diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 07a35333b1..e9c4d73779 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2502,6 +2502,8 @@ virAsprintfInternal; virSkipSpaces; virSkipSpacesAndBackslash; virSkipSpacesBackwards; +virStrcat; +virStrcatInplace; virStrcpy; virStrdup; virStringBufferIsPrintable; diff --git a/src/util/virstring.c b/src/util/virstring.c index 69abc267bf..bc15ce7e9e 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -837,6 +837,76 @@ virStrndup(char **dest, }
+/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * @inPlace: false if we should expand the allocated memory before moving, + * true if we should assume someone else has already done that.
This is here probably from some work in progress version.
+ * @report: whether to report OOM error, if there is one + * @domcode: error domain code + * @filename: caller's filename + * @funcname: caller's funcname + * @linenr: caller's line number + * + * Wrapper over strcat, which reports OOM error if told so, + * in which case callers wants to pass @domcode, @filename, + * @funcname and @linenr which should represent location in + * caller's body where virStrcat is called from. Consider + * using VIR_STRCAT which sets these automatically. + * + * Returns: 0 for NULL src, 1 on successful concatenate, -1 otherwise. + */ +int +virStrcat(char **dest, + const char *src, + bool report, + int domcode, + const char *filename, + const char *funcname, + size_t linenr) +{ + size_t dest_len = 0; + size_t src_len = 0; + + if (!src) + return 0; + + if (*dest) + dest_len = strlen(*dest); + src_len = strlen(src); + + if (virReallocN(dest, sizeof(*dest), dest_len + src_len + 1, + report, domcode, filename, funcname, linenr) < 0) + return -1; + + strcat(*dest, src); + + return 1; +} + + +/** + * virStrcat + * @dest: where to store concatenated string + * @src: the source string to append to @dest + * + * Wrapper over strcat, which properly handles if @src is NULL. + * + * Returns: 0 for NULL src, 1 on successful concatenate. + */
Really? This whole wrapper just for checking NULL? So instead of:
if (x) strcat (y, x)
I should do:
VIR_STRCAT_INPLACE(y, x) now? It's not even saving any characters.
Plus, is there *really* any occurrence of strcat that might be called with NULL? I would say that such code has more logic problems in that case.
The reason is to forbid using strcat directly and force to use the wrappers. I had two options in my mind, one that will use the virStrcatInplace function with return values 0 and 1 to determine whether something actually happened or only the "if (x) strcat (y, x)" as a macro without any return value.
I get the idea, but my point was that I don't get why we should forbid using strcat() by itself.
Developers may not know that we already have a helper for it and they would want to reallocate the destination string and then simply call strcat. If we forbid strcat we can advertise that helper instead.
The reallocating strcat makes sense, but the name is *really* confusing for people who are used to what strcat/strncat does. So while I see how
In that case it would be nice to provide some alternative :).
virStringAppend?
Sure, I'm usually bad in naming functions.
that might be useful, we also have a buffer for more interesting dynamic string modifications and I'm not sure this is something that will help a lot.
Yes we have buffer, but this macro will be used in the buffer code itself and I guess it would be strange to use a buffer inside a buffer function.
Since that will not be changing the input data, it might be safer to just sacrifice few bytes of memory and do virAsprintf() into new buffer. But if you like this approach more, feel free to use it.
This would not be used to modify the resulting buffer, it just concatenates all the chars that needs to be escaped from all the groups passed to virBufferEscapeN in order to simply check with one "if" whether we need to escape something or not.
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 14 ++++++++------ src/storage/storage_backend_logical.c | 6 +++--- src/test/test_driver.c | 2 +- src/util/vircgroup.c | 4 +--- src/xen/xend_internal.c | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-) diff --git a/cfg.mk b/cfg.mk index 22c655eac6..6646509206 100644 --- a/cfg.mk +++ b/cfg.mk @@ -390,6 +390,11 @@ sc_prohibit_strdup: halt='use VIR_STRDUP, not strdup' \ $(_sc_search_regexp) +sc_prohibit_strcat: + @prohibit='\<strn?cat\> *\(' \ + halt='use VIR_STRCAT, not strcat' \ + $(_sc_search_regexp) + # Prefer virSetUIDGID. sc_prohibit_setuid: @prohibit='\<set(re)?[ug]id\> *\(' \ @@ -994,12 +999,6 @@ sc_prohibit_not_streq: halt='Use STRNEQ instead of !STREQ and STREQ instead of !STRNEQ' \ $(_sc_search_regexp) -sc_prohibit_verbose_strcat: - @prohibit='strncat\([^,]*,\s+([^,]*),\s+strlen\(\1\)\)' \ - in_vc_files='\.[ch]$$' \ - halt='Use strcat(a, b) instead of strncat(a, b, strlen(b))' \ - $(_sc_search_regexp) - # Ensure that each .c file containing a "main" function also # calls virGettextInitialize sc_gettext_init: @@ -1134,6 +1133,9 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$) +exclude_file_name_regexp--sc_prohibit_strcat = \ + ^(docs/|src/util/virstring\.c|tests/virstringtest\.c)$$ + exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$) diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 756c62e908..5e006a980a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -201,11 +201,11 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ if (VIR_ALLOC_N(regex, nextents * (strlen(regex_unit) + 1) + 1) < 0) goto cleanup; - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, regex_unit); for (i = 1; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, ","); + VIR_STRCAT_INPLACE(regex, regex_unit); } if (VIR_ALLOC(reg) < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f10b9..be887ec1bb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -699,7 +699,7 @@ static char *testBuildFilename(const char *relativeTo, VIR_FREE(absFile); return NULL; } - strcat(absFile, filename); + ignore_value(VIR_STRCAT_INPLACE(absFile, filename)); return absFile; } else { ignore_value(VIR_STRDUP(ret, filename)); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 5aa1db5b14..e8210ca6eb 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1299,10 +1299,8 @@ virCgroupSetPartitionSuffix(const char *path, char **res) */ if (STRNEQ(tokens[i], "") && !strchr(tokens[i], '.')) { - if (VIR_REALLOC_N(tokens[i], - strlen(tokens[i]) + strlen(".partition") + 1) < 0) + if (VIR_STRCAT(tokens[i], ".partition") < 0) goto cleanup; - strcat(tokens[i], ".partition"); } if (virCgroupPartitionEscape(&(tokens[i])) < 0) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 605c3cdccf..1f9d4c6959 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1824,7 +1824,7 @@ xenDaemonDomainPinVcpu(virConnectPtr conn, for (i = 0; i < maplen; i++) for (j = 0; j < 8; j++) if (cpumap[i] & (1 << j)) { snprintf(buf, sizeof(buf), "%zu,", (8 * i) + j); - strcat(mapstr, buf); + VIR_STRCAT_INPLACE(mapstr, buf); } mapstr[strlen(mapstr) - 1] = 0; -- 2.11.1
On Thu, Feb 23, 2017 at 04:26:57PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 14 ++++++++------ src/storage/storage_backend_logical.c | 6 +++--- src/test/test_driver.c | 2 +- src/util/vircgroup.c | 4 +--- src/xen/xend_internal.c | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 22c655eac6..6646509206 100644 --- a/cfg.mk +++ b/cfg.mk @@ -390,6 +390,11 @@ sc_prohibit_strdup: halt='use VIR_STRDUP, not strdup' \ $(_sc_search_regexp)
+sc_prohibit_strcat: + @prohibit='\<strn?cat\> *\(' \ + halt='use VIR_STRCAT, not strcat' \ + $(_sc_search_regexp) + # Prefer virSetUIDGID. sc_prohibit_setuid: @prohibit='\<set(re)?[ug]id\> *\(' \ @@ -994,12 +999,6 @@ sc_prohibit_not_streq: halt='Use STRNEQ instead of !STREQ and STREQ instead of !STRNEQ' \ $(_sc_search_regexp)
-sc_prohibit_verbose_strcat: - @prohibit='strncat\([^,]*,\s+([^,]*),\s+strlen\(\1\)\)' \ - in_vc_files='\.[ch]$$' \ - halt='Use strcat(a, b) instead of strncat(a, b, strlen(b))' \ - $(_sc_search_regexp) - # Ensure that each .c file containing a "main" function also # calls virGettextInitialize sc_gettext_init: @@ -1134,6 +1133,9 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
+exclude_file_name_regexp--sc_prohibit_strcat = \ + ^(docs/|src/util/virstring\.c|tests/virstringtest\.c)$$
why virstringtest.c?
+ exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 756c62e908..5e006a980a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -201,11 +201,11 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ if (VIR_ALLOC_N(regex, nextents * (strlen(regex_unit) + 1) + 1) < 0) goto cleanup; - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, regex_unit); for (i = 1; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, ","); + VIR_STRCAT_INPLACE(regex, regex_unit); }
if (VIR_ALLOC(reg) < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f10b9..be887ec1bb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -699,7 +699,7 @@ static char *testBuildFilename(const char *relativeTo, VIR_FREE(absFile); return NULL; } - strcat(absFile, filename); + ignore_value(VIR_STRCAT_INPLACE(absFile, filename)); return absFile; } else { ignore_value(VIR_STRDUP(ret, filename)); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 5aa1db5b14..e8210ca6eb 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1299,10 +1299,8 @@ virCgroupSetPartitionSuffix(const char *path, char **res) */ if (STRNEQ(tokens[i], "") && !strchr(tokens[i], '.')) { - if (VIR_REALLOC_N(tokens[i], - strlen(tokens[i]) + strlen(".partition") + 1) < 0) + if (VIR_STRCAT(tokens[i], ".partition") < 0) goto cleanup; - strcat(tokens[i], ".partition");
Not counting the rest of your patches, just for now, is this the only place the VIR_STRCAT adds value? This makes me even more cautious about the patches.
}
if (virCgroupPartitionEscape(&(tokens[i])) < 0) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 605c3cdccf..1f9d4c6959 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1824,7 +1824,7 @@ xenDaemonDomainPinVcpu(virConnectPtr conn, for (i = 0; i < maplen; i++) for (j = 0; j < 8; j++) if (cpumap[i] & (1 << j)) { snprintf(buf, sizeof(buf), "%zu,", (8 * i) + j); - strcat(mapstr, buf); + VIR_STRCAT_INPLACE(mapstr, buf); } mapstr[strlen(mapstr) - 1] = 0;
-- 2.11.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 23, 2017 at 05:15:37PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 04:26:57PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 14 ++++++++------ src/storage/storage_backend_logical.c | 6 +++--- src/test/test_driver.c | 2 +- src/util/vircgroup.c | 4 +--- src/xen/xend_internal.c | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 22c655eac6..6646509206 100644 --- a/cfg.mk +++ b/cfg.mk @@ -390,6 +390,11 @@ sc_prohibit_strdup: halt='use VIR_STRDUP, not strdup' \ $(_sc_search_regexp)
+sc_prohibit_strcat: + @prohibit='\<strn?cat\> *\(' \ + halt='use VIR_STRCAT, not strcat' \ + $(_sc_search_regexp) + # Prefer virSetUIDGID. sc_prohibit_setuid: @prohibit='\<set(re)?[ug]id\> *\(' \ @@ -994,12 +999,6 @@ sc_prohibit_not_streq: halt='Use STRNEQ instead of !STREQ and STREQ instead of !STRNEQ' \ $(_sc_search_regexp)
-sc_prohibit_verbose_strcat: - @prohibit='strncat\([^,]*,\s+([^,]*),\s+strlen\(\1\)\)' \ - in_vc_files='\.[ch]$$' \ - halt='Use strcat(a, b) instead of strncat(a, b, strlen(b))' \ - $(_sc_search_regexp) - # Ensure that each .c file containing a "main" function also # calls virGettextInitialize sc_gettext_init: @@ -1134,6 +1133,9 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
+exclude_file_name_regexp--sc_prohibit_strcat = \ + ^(docs/|src/util/virstring\.c|tests/virstringtest\.c)$$
why virstringtest.c?
I'll remove it, nice catch.
+ exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 756c62e908..5e006a980a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -201,11 +201,11 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ if (VIR_ALLOC_N(regex, nextents * (strlen(regex_unit) + 1) + 1) < 0) goto cleanup; - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, regex_unit); for (i = 1; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, ","); + VIR_STRCAT_INPLACE(regex, regex_unit); }
if (VIR_ALLOC(reg) < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f10b9..be887ec1bb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -699,7 +699,7 @@ static char *testBuildFilename(const char *relativeTo, VIR_FREE(absFile); return NULL; } - strcat(absFile, filename); + ignore_value(VIR_STRCAT_INPLACE(absFile, filename)); return absFile; } else { ignore_value(VIR_STRDUP(ret, filename)); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 5aa1db5b14..e8210ca6eb 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1299,10 +1299,8 @@ virCgroupSetPartitionSuffix(const char *path, char **res) */ if (STRNEQ(tokens[i], "") && !strchr(tokens[i], '.')) { - if (VIR_REALLOC_N(tokens[i], - strlen(tokens[i]) + strlen(".partition") + 1) < 0) + if (VIR_STRCAT(tokens[i], ".partition") < 0) goto cleanup; - strcat(tokens[i], ".partition");
Not counting the rest of your patches, just for now, is this the only place the VIR_STRCAT adds value? This makes me even more cautious about the patches.
Yes, the VIR_STRCAT was introduced solely for the following patch and since there was code using strcat I though that it would be nice to switch to VIR_STRCAT. It definitely doesn't hurt if we have this helper and who knows, someone may use it some day :). Pavel
}
if (virCgroupPartitionEscape(&(tokens[i])) < 0) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 605c3cdccf..1f9d4c6959 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1824,7 +1824,7 @@ xenDaemonDomainPinVcpu(virConnectPtr conn, for (i = 0; i < maplen; i++) for (j = 0; j < 8; j++) if (cpumap[i] & (1 << j)) { snprintf(buf, sizeof(buf), "%zu,", (8 * i) + j); - strcat(mapstr, buf); + VIR_STRCAT_INPLACE(mapstr, buf); } mapstr[strlen(mapstr) - 1] = 0;
-- 2.11.1
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 23, 2017 at 05:15:37PM +0100, Martin Kletzander wrote:
On Thu, Feb 23, 2017 at 04:26:57PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- cfg.mk | 14 ++++++++------ src/storage/storage_backend_logical.c | 6 +++--- src/test/test_driver.c | 2 +- src/util/vircgroup.c | 4 +--- src/xen/xend_internal.c | 2 +- 5 files changed, 14 insertions(+), 14 deletions(-)
diff --git a/cfg.mk b/cfg.mk index 22c655eac6..6646509206 100644 --- a/cfg.mk +++ b/cfg.mk @@ -390,6 +390,11 @@ sc_prohibit_strdup: halt='use VIR_STRDUP, not strdup' \ $(_sc_search_regexp)
+sc_prohibit_strcat: + @prohibit='\<strn?cat\> *\(' \ + halt='use VIR_STRCAT, not strcat' \ + $(_sc_search_regexp) + # Prefer virSetUIDGID. sc_prohibit_setuid: @prohibit='\<set(re)?[ug]id\> *\(' \ @@ -994,12 +999,6 @@ sc_prohibit_not_streq: halt='Use STRNEQ instead of !STREQ and STREQ instead of !STRNEQ' \ $(_sc_search_regexp)
-sc_prohibit_verbose_strcat: - @prohibit='strncat\([^,]*,\s+([^,]*),\s+strlen\(\1\)\)' \ - in_vc_files='\.[ch]$$' \ - halt='Use strcat(a, b) instead of strncat(a, b, strlen(b))' \ - $(_sc_search_regexp) - # Ensure that each .c file containing a "main" function also # calls virGettextInitialize sc_gettext_init: @@ -1134,6 +1133,9 @@ exclude_file_name_regexp--sc_prohibit_asprintf = \ exclude_file_name_regexp--sc_prohibit_strdup = \ ^(docs/|examples/|src/util/virstring\.c|tests/vir(netserverclient|cgroup)mock.c$$)
+exclude_file_name_regexp--sc_prohibit_strcat = \ + ^(docs/|src/util/virstring\.c|tests/virstringtest\.c)$$
why virstringtest.c?
+ exclude_file_name_regexp--sc_prohibit_close = \ (\.p[yl]$$|\.spec\.in$$|^docs/|^(src/util/virfile\.c|src/libvirt-stream\.c|tests/vir.+mock\.c)$$)
diff --git a/src/storage/storage_backend_logical.c b/src/storage/storage_backend_logical.c index 756c62e908..5e006a980a 100644 --- a/src/storage/storage_backend_logical.c +++ b/src/storage/storage_backend_logical.c @@ -201,11 +201,11 @@ virStorageBackendLogicalParseVolExtents(virStorageVolDefPtr vol, /* Allocate space for 'nextents' regex_unit strings plus a comma for each */ if (VIR_ALLOC_N(regex, nextents * (strlen(regex_unit) + 1) + 1) < 0) goto cleanup; - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, regex_unit); for (i = 1; i < nextents; i++) { /* "," is the separator of "devices" field */ - strcat(regex, ","); - strcat(regex, regex_unit); + VIR_STRCAT_INPLACE(regex, ","); + VIR_STRCAT_INPLACE(regex, regex_unit); }
if (VIR_ALLOC(reg) < 0) diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 5fef3f10b9..be887ec1bb 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -699,7 +699,7 @@ static char *testBuildFilename(const char *relativeTo, VIR_FREE(absFile); return NULL; } - strcat(absFile, filename); + ignore_value(VIR_STRCAT_INPLACE(absFile, filename)); return absFile; } else { ignore_value(VIR_STRDUP(ret, filename)); diff --git a/src/util/vircgroup.c b/src/util/vircgroup.c index 5aa1db5b14..e8210ca6eb 100644 --- a/src/util/vircgroup.c +++ b/src/util/vircgroup.c @@ -1299,10 +1299,8 @@ virCgroupSetPartitionSuffix(const char *path, char **res) */ if (STRNEQ(tokens[i], "") && !strchr(tokens[i], '.')) { - if (VIR_REALLOC_N(tokens[i], - strlen(tokens[i]) + strlen(".partition") + 1) < 0) + if (VIR_STRCAT(tokens[i], ".partition") < 0) goto cleanup; - strcat(tokens[i], ".partition");
Not counting the rest of your patches, just for now, is this the only place the VIR_STRCAT adds value? This makes me even more cautious about the patches.
That code could be rewritten with virAsprintf for clarity eg char *part; if (virAsprintf(&part, "%s.partition", tokens[i]) < 0) goto cleanup; VIR_FREE(tokens[i]); tokens[i] = part; No, it doesn't make it shorter, but I think it makes it clearer as to what we're actually doing here. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 2 + tests/virbuftest.c | 41 +++++++++++++++++++ 4 files changed, 148 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9c4d73779..6ce32f1101 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1286,6 +1286,7 @@ virBufferContentAndReset; virBufferCurrentContent; virBufferError; virBufferEscape; +virBufferEscapeN; virBufferEscapeSexpr; virBufferEscapeShell; virBufferEscapeString; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index d582e7dbec..ad6b29951e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "viralloc.h" #include "virerror.h" +#include "virstring.h" /* If adding more fields, ensure to edit buf.h to match @@ -588,6 +589,109 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, VIR_FREE(escaped); } + +struct _virBufferEscapePair { + char escape; + char *toescape; +}; + + +/** + * virBufferEscapeN: + * @buf: the buffer to append to + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which needs to be escaped + * @...: the variable list of arguments composed + * + * The variable list of arguments @... must be composed of + * 'char escape, char *toescape' pairs followed by NULL. + * + * This has the same functionality as virBufferEscape with the extension + * that allows to specify multiple pairs of chars that needs to be escaped. + */ +void +virBufferEscapeN(virBufferPtr buf, + const char *format, + const char *str, + ...) +{ + int len; + size_t i; + char escape; + char *toescape; + char *toescapeall = NULL; + char *escaped = NULL; + char *out; + const char *cur; + struct _virBufferEscapePair escapeItem; + struct _virBufferEscapePair *escapeList = NULL; + size_t nescapeList = 0; + va_list ap; + + if ((format == NULL) || (buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + va_start(ap, str); + + while ((escape = va_arg(ap, int))) { + if (!(toescape = va_arg(ap, char *))) { + virBufferSetError(buf, errno); + goto cleanup; + } + + escapeItem.escape = escape; + escapeItem.toescape = toescape; + + if (VIR_STRCAT_QUIET(toescapeall, toescape) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + + if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + } + + len = strlen(str); + if (strcspn(str, toescapeall) == len) { + virBufferAsprintf(buf, format, str); + goto cleanup; + } + + if (xalloc_oversized(2, len) || + VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + + cur = str; + out = escaped; + while (*cur != 0) { + for (i = 0; i < nescapeList; i++) { + if (strchr(escapeList[i].toescape, *cur)) { + *out++ = escapeList[i].escape; + break; + } + } + *out++ = *cur; + cur++; + } + *out = 0; + + virBufferAsprintf(buf, format, escaped); + + cleanup: + va_end(ap); + VIR_FREE(toescapeall); + VIR_FREE(escapeList); + VIR_FREE(escaped); +} + + /** * virBufferURIEncodeString: * @buf: the buffer to append to diff --git a/src/util/virbuffer.h b/src/util/virbuffer.h index 144a1ba06e..94f14b5b16 100644 --- a/src/util/virbuffer.h +++ b/src/util/virbuffer.h @@ -82,6 +82,8 @@ void virBufferStrcat(virBufferPtr buf, ...) ATTRIBUTE_SENTINEL; void virBufferEscape(virBufferPtr buf, char escape, const char *toescape, const char *format, const char *str); +void virBufferEscapeN(virBufferPtr buf, const char *format, + const char *str, ...); void virBufferEscapeString(virBufferPtr buf, const char *format, const char *str); void virBufferEscapeSexpr(virBufferPtr buf, const char *format, diff --git a/tests/virbuftest.c b/tests/virbuftest.c index 22407ab6a8..34160e6b28 100644 --- a/tests/virbuftest.c +++ b/tests/virbuftest.c @@ -376,6 +376,35 @@ testBufEscapeStr(const void *opaque ATTRIBUTE_UNUSED) static int +testBufEscapeN(const void *opaque) +{ + const struct testBufAddStrData *data = opaque; + virBuffer buf = VIR_BUFFER_INITIALIZER; + char *actual; + int ret = -1; + + virBufferEscapeN(&buf, "%s", data->data, '\\', "=", ',', ",", NULL); + + if (!(actual = virBufferContentAndReset(&buf))) { + VIR_TEST_DEBUG("testBufEscapeN: buf is empty"); + goto cleanup; + } + + if (STRNEQ_NULLABLE(actual, data->expect)) { + VIR_TEST_DEBUG("testBufEscapeN: Strings don't match:\n"); + virTestDifference(stderr, data->expect, actual); + goto cleanup; + } + + ret = 0; + + cleanup: + VIR_FREE(actual); + return ret; +} + + +static int mymain(void) { int ret = 0; @@ -422,6 +451,18 @@ mymain(void) DO_TEST_ESCAPE("\x01\x01\x02\x03\x05\x08", "<c>\n <el></el>\n</c>"); +#define DO_TEST_ESCAPEN(data, expect) \ + do { \ + struct testBufAddStrData info = { data, expect }; \ + if (virTestRun("Buf: EscapeN", testBufEscapeN, &info) < 0) \ + ret = -1; \ + } while (0) + + DO_TEST_ESCAPEN("noescape", "noescape"); + DO_TEST_ESCAPEN("comma,escape", "comma,,escape"); + DO_TEST_ESCAPEN("equal=escape", "equal\\=escape"); + DO_TEST_ESCAPEN("comma,equal=escape", "comma,,equal\\=escape"); + return ret == 0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 2.11.1
On Thu, Feb 23, 2017 at 04:26:58PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 2 + tests/virbuftest.c | 41 +++++++++++++++++++ 4 files changed, 148 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9c4d73779..6ce32f1101 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1286,6 +1286,7 @@ virBufferContentAndReset; virBufferCurrentContent; virBufferError; virBufferEscape; +virBufferEscapeN; virBufferEscapeSexpr; virBufferEscapeShell; virBufferEscapeString; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index d582e7dbec..ad6b29951e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "viralloc.h" #include "virerror.h" +#include "virstring.h"
/* If adding more fields, ensure to edit buf.h to match @@ -588,6 +589,109 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, VIR_FREE(escaped); }
+ +struct _virBufferEscapePair { + char escape; + char *toescape; +}; + + +/** + * virBufferEscapeN: + * @buf: the buffer to append to + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which needs to be escaped + * @...: the variable list of arguments composed + * + * The variable list of arguments @... must be composed of + * 'char escape, char *toescape' pairs followed by NULL.
So most of the complexity you have here is to deal with the ability to turn a single character into a string of escaped characters. Unless I'm missing something, you don't actually use that ability in practice. If we make it more restrictive so you just have a 1-1 mapping we can simplify the API & implementation. eg virBufferEscapeN(virBufferPtr buf, const char *format, const char *str, const char *forbidden, const char *replacements) with strlen(forbidden) == strlen(replacements);
+ * + * This has the same functionality as virBufferEscape with the extension + * that allows to specify multiple pairs of chars that needs to be escaped. + */ +void +virBufferEscapeN(virBufferPtr buf, + const char *format, + const char *str, + ...) +{ + int len; + size_t i; + char escape; + char *toescape; + char *toescapeall = NULL; + char *escaped = NULL; + char *out; + const char *cur; + struct _virBufferEscapePair escapeItem; + struct _virBufferEscapePair *escapeList = NULL; + size_t nescapeList = 0; + va_list ap; + + if ((format == NULL) || (buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + va_start(ap, str); + + while ((escape = va_arg(ap, int))) { + if (!(toescape = va_arg(ap, char *))) { + virBufferSetError(buf, errno); + goto cleanup; + } + + escapeItem.escape = escape; + escapeItem.toescape = toescape; + + if (VIR_STRCAT_QUIET(toescapeall, toescape) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + + if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + }
This whole loop would go away with the simpler API contract, which nicely avoids doing allocations in the loop body.
+ len = strlen(str); + if (strcspn(str, toescapeall) == len) { + virBufferAsprintf(buf, format, str); + goto cleanup; + } + + if (xalloc_oversized(2, len) || + VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + + cur = str; + out = escaped; + while (*cur != 0) { + for (i = 0; i < nescapeList; i++) { + if (strchr(escapeList[i].toescape, *cur)) { + *out++ = escapeList[i].escape; + break; + } + } + *out++ = *cur; + cur++; + } + *out = 0; + + virBufferAsprintf(buf, format, escaped); + + cleanup: + va_end(ap); + VIR_FREE(toescapeall); + VIR_FREE(escapeList); + VIR_FREE(escaped); +}
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
On Thu, Feb 23, 2017 at 04:32:32PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 23, 2017 at 04:26:58PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 2 + tests/virbuftest.c | 41 +++++++++++++++++++ 4 files changed, 148 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9c4d73779..6ce32f1101 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1286,6 +1286,7 @@ virBufferContentAndReset; virBufferCurrentContent; virBufferError; virBufferEscape; +virBufferEscapeN; virBufferEscapeSexpr; virBufferEscapeShell; virBufferEscapeString; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index d582e7dbec..ad6b29951e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "viralloc.h" #include "virerror.h" +#include "virstring.h"
/* If adding more fields, ensure to edit buf.h to match @@ -588,6 +589,109 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, VIR_FREE(escaped); }
+ +struct _virBufferEscapePair { + char escape; + char *toescape; +}; + + +/** + * virBufferEscapeN: + * @buf: the buffer to append to + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which needs to be escaped + * @...: the variable list of arguments composed + * + * The variable list of arguments @... must be composed of + * 'char escape, char *toescape' pairs followed by NULL.
So most of the complexity you have here is to deal with the ability to turn a single character into a string of escaped characters. Unless I'm missing something, you don't actually use that ability in practice. If we make it more restrictive so you just have a 1-1 mapping we can simplify the API & implementation. eg
virBufferEscapeN(virBufferPtr buf, const char *format, const char *str, const char *forbidden, const char *replacements)
with strlen(forbidden) == strlen(replacements);
The idea was that a group of characters can be escaped with one character, so far the usage seems to be really simple. Currently this would work and it would be really simple: virBufferEscapeN(buf, "%s", str, ",=", ",\\"); but imagine this scenario: virBufferEscapeN(buf, "%s", str, ",=%&*()", ",\\\\\\\\\\\\"); virBufferEscapeN(buf, "%s", str, ',', ",", '\\', "=%&*()", NULL); I think that the second approach is better and easier for user of virBufferEscapeN.
+ * + * This has the same functionality as virBufferEscape with the extension + * that allows to specify multiple pairs of chars that needs to be escaped. + */ +void +virBufferEscapeN(virBufferPtr buf, + const char *format, + const char *str, + ...) +{ + int len; + size_t i; + char escape; + char *toescape; + char *toescapeall = NULL; + char *escaped = NULL; + char *out; + const char *cur; + struct _virBufferEscapePair escapeItem; + struct _virBufferEscapePair *escapeList = NULL; + size_t nescapeList = 0; + va_list ap; + + if ((format == NULL) || (buf == NULL) || (str == NULL)) + return; + + if (buf->error) + return; + + va_start(ap, str); + + while ((escape = va_arg(ap, int))) { + if (!(toescape = va_arg(ap, char *))) { + virBufferSetError(buf, errno); + goto cleanup; + } + + escapeItem.escape = escape; + escapeItem.toescape = toescape; + + if (VIR_STRCAT_QUIET(toescapeall, toescape) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + + if (VIR_APPEND_ELEMENT_QUIET(escapeList, nescapeList, escapeItem) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + }
This whole loop would go away with the simpler API contract, which nicely avoids doing allocations in the loop body.
+ len = strlen(str); + if (strcspn(str, toescapeall) == len) { + virBufferAsprintf(buf, format, str); + goto cleanup; + } + + if (xalloc_oversized(2, len) || + VIR_ALLOC_N_QUIET(escaped, 2 * len + 1) < 0) { + virBufferSetError(buf, errno); + goto cleanup; + } + + cur = str; + out = escaped; + while (*cur != 0) { + for (i = 0; i < nescapeList; i++) { + if (strchr(escapeList[i].toescape, *cur)) { + *out++ = escapeList[i].escape; + break; + } + } + *out++ = *cur; + cur++; + } + *out = 0; + + virBufferAsprintf(buf, format, escaped); + + cleanup: + va_end(ap); + VIR_FREE(toescapeall); + VIR_FREE(escapeList); + VIR_FREE(escaped); +}
Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Thu, Feb 23, 2017 at 05:51:21PM +0100, Pavel Hrdina wrote:
On Thu, Feb 23, 2017 at 04:32:32PM +0000, Daniel P. Berrange wrote:
On Thu, Feb 23, 2017 at 04:26:58PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virbuffer.c | 104 +++++++++++++++++++++++++++++++++++++++++++++++ src/util/virbuffer.h | 2 + tests/virbuftest.c | 41 +++++++++++++++++++ 4 files changed, 148 insertions(+)
diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index e9c4d73779..6ce32f1101 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -1286,6 +1286,7 @@ virBufferContentAndReset; virBufferCurrentContent; virBufferError; virBufferEscape; +virBufferEscapeN; virBufferEscapeSexpr; virBufferEscapeShell; virBufferEscapeString; diff --git a/src/util/virbuffer.c b/src/util/virbuffer.c index d582e7dbec..ad6b29951e 100644 --- a/src/util/virbuffer.c +++ b/src/util/virbuffer.c @@ -33,6 +33,7 @@ #include "virbuffer.h" #include "viralloc.h" #include "virerror.h" +#include "virstring.h"
/* If adding more fields, ensure to edit buf.h to match @@ -588,6 +589,109 @@ virBufferEscape(virBufferPtr buf, char escape, const char *toescape, VIR_FREE(escaped); }
+ +struct _virBufferEscapePair { + char escape; + char *toescape; +}; + + +/** + * virBufferEscapeN: + * @buf: the buffer to append to + * @format: a printf like format string but with only one %s parameter + * @str: the string argument which needs to be escaped + * @...: the variable list of arguments composed + * + * The variable list of arguments @... must be composed of + * 'char escape, char *toescape' pairs followed by NULL.
So most of the complexity you have here is to deal with the ability to turn a single character into a string of escaped characters. Unless I'm missing something, you don't actually use that ability in practice. If we make it more restrictive so you just have a 1-1 mapping we can simplify the API & implementation. eg
virBufferEscapeN(virBufferPtr buf, const char *format, const char *str, const char *forbidden, const char *replacements)
with strlen(forbidden) == strlen(replacements);
The idea was that a group of characters can be escaped with one character, so far the usage seems to be really simple. Currently this would work and it would be really simple:
virBufferEscapeN(buf, "%s", str, ",=", ",\\");
but imagine this scenario:
virBufferEscapeN(buf, "%s", str, ",=%&*()", ",\\\\\\\\\\\\");
virBufferEscapeN(buf, "%s", str, ',', ",", '\\', "=%&*()", NULL);
I think that the second approach is better and easier for user of virBufferEscapeN.
Oh, that's the opposite way around to what I thought you were doing ! I thought a single character could be escaped to a string of multiple characters, but you have a string containing a set of characters that must be escaped to a single character. Regards, Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
This will eventually replace virQEMUBuildBufferEscapeComma, however it's not possible right now. Some parts of the code that uses the old function needs to be refactored. Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/libvirt_private.syms | 1 + src/util/virqemu.c | 17 +++++++++++++++++ src/util/virqemu.h | 1 + 3 files changed, 19 insertions(+) diff --git a/src/libvirt_private.syms b/src/libvirt_private.syms index 6ce32f1101..3fb123751a 100644 --- a/src/libvirt_private.syms +++ b/src/libvirt_private.syms @@ -2298,6 +2298,7 @@ virProcessWait; # util/virqemu.h +virQEMUBuildBufferEscape; virQEMUBuildBufferEscapeComma; virQEMUBuildCommandLineJSON; virQEMUBuildCommandLineJSONArrayBitmap; diff --git a/src/util/virqemu.c b/src/util/virqemu.c index 2e9e65f9ef..f10b356781 100644 --- a/src/util/virqemu.c +++ b/src/util/virqemu.c @@ -300,6 +300,23 @@ virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str) /** + * virQEMUBuildBufferEscape: + * @buf: buffer to append the escaped string + * @str: the string to escape + * + * Some characters passed as values on the QEMU command line must be escaped. + * + * - ',' must by escaped by ',' + * - '=' must by escaped by '\' + */ +void +virQEMUBuildBufferEscape(virBufferPtr buf, const char *str) +{ + virBufferEscapeN(buf, "%s", str, ',', ",", '\\', "=", NULL); +} + + +/** * virQEMUBuildLuksOpts: * @buf: buffer to build the string into * @enc: pointer to encryption info diff --git a/src/util/virqemu.h b/src/util/virqemu.h index 539d62ab14..10aeb67f4e 100644 --- a/src/util/virqemu.h +++ b/src/util/virqemu.h @@ -50,6 +50,7 @@ char *virQEMUBuildObjectCommandlineFromJSON(const char *type, char *virQEMUBuildDriveCommandlineFromJSON(virJSONValuePtr src); void virQEMUBuildBufferEscapeComma(virBufferPtr buf, const char *str); +void virQEMUBuildBufferEscape(virBufferPtr buf, const char *str); void virQEMUBuildLuksOpts(virBufferPtr buf, virStorageEncryptionInfoDefPtr enc, const char *alias) -- 2.11.1
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1352529 Signed-off-by: Pavel Hrdina <phrdina@redhat.com> --- src/qemu/qemu_command.c | 6 ++++-- tests/qemuxml2argvdata/qemuxml2argv-name-escape.args | 5 +++-- tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml | 7 ++++++- tests/qemuxml2argvtest.c | 3 ++- 4 files changed, 15 insertions(+), 6 deletions(-) diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c index d5da533e50..41eecfd187 100644 --- a/src/qemu/qemu_command.c +++ b/src/qemu/qemu_command.c @@ -7495,7 +7495,7 @@ qemuBuildGraphicsVNCCommandLine(virQEMUDriverConfigPtr cfg, switch (glisten->type) { case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_SOCKET: virBufferAddLit(&opt, "unix:"); - virQEMUBuildBufferEscapeComma(&opt, glisten->socket); + virQEMUBuildBufferEscape(&opt, glisten->socket); break; case VIR_DOMAIN_GRAPHICS_LISTEN_TYPE_ADDRESS: @@ -7627,7 +7627,9 @@ qemuBuildGraphicsSPICECommandLine(virQEMUDriverConfigPtr cfg, goto error; } - virBufferAsprintf(&opt, "unix,addr=%s,", glisten->socket); + virBufferAddLit(&opt, "unix,addr="); + virQEMUBuildBufferEscape(&opt, glisten->socket); + virBufferAddLit(&opt, ","); hasInsecure = true; break; diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args index 9ae50bd455..deb37e09e6 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.args @@ -3,7 +3,7 @@ PATH=/bin \ HOME=/home/test \ USER=test \ LOGNAME=test \ -QEMU_AUDIO_DRV=none \ +QEMU_AUDIO_DRV=spice \ /usr/bin/qemu \ -name guest=foo=1,,bar=2,debug-threads=on \ -S \ @@ -20,6 +20,7 @@ bar=2/monitor.sock,server,nowait \ -no-acpi \ -boot c \ -usb \ --vnc unix:/tmp/bar,,foo.sock \ +-vnc 'unix:/tmp/lib/domain--1-foo\=1,,bar\=2/vnc.sock' \ +-spice 'unix,addr=/tmp/lib/domain--1-foo\=1,,bar\=2/spice.sock' \ -vga cirrus \ -device virtio-balloon-pci,id=balloon0,bus=pci.0,addr=0x3 diff --git a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml index 5e8c7476fe..604e1453f2 100644 --- a/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml +++ b/tests/qemuxml2argvdata/qemuxml2argv-name-escape.xml @@ -14,6 +14,11 @@ <on_crash>destroy</on_crash> <devices> <emulator>/usr/bin/qemu</emulator> - <graphics type='vnc' socket='/tmp/bar,foo.sock'/> + <graphics type='vnc'> + <listen type='socket'/> + </graphics> + <graphics type='spice'> + <listen type='socket'/> + </graphics> </devices> </domain> diff --git a/tests/qemuxml2argvtest.c b/tests/qemuxml2argvtest.c index f55b04b057..f3b5648b5c 100644 --- a/tests/qemuxml2argvtest.c +++ b/tests/qemuxml2argvtest.c @@ -2455,7 +2455,8 @@ mymain(void) DO_TEST("name-escape", QEMU_CAPS_NAME_DEBUG_THREADS, QEMU_CAPS_OBJECT_SECRET, QEMU_CAPS_CHARDEV, QEMU_CAPS_VNC, - QEMU_CAPS_NAME_GUEST, QEMU_CAPS_DEVICE_CIRRUS_VGA); + QEMU_CAPS_NAME_GUEST, QEMU_CAPS_DEVICE_CIRRUS_VGA, + QEMU_CAPS_SPICE, QEMU_CAPS_SPICE_UNIX); DO_TEST("debug-threads", QEMU_CAPS_NAME_DEBUG_THREADS); DO_TEST("master-key", QEMU_CAPS_OBJECT_SECRET); -- 2.11.1
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Pavel Hrdina