9:26 a.m.
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
9:26 a.m.
New subject: [libvirt] [PATCH 1/5] util: virstring: introduce virStrcat and VIR_STRCAT
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:06 a.m.
New subject: [libvirt] [PATCH 1/5] util: virstring: introduce virStrcat and VIR_STRCAT
On Thu, Feb 23, 2017 at 04:26:56PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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.
10:15 a.m.
New subject: [libvirt] [PATCH 1/5] util: virstring: introduce virStrcat and VIR_STRCAT
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(a)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
10:25 a.m.
New subject: [libvirt] [PATCH 1/5] util: virstring: introduce virStrcat and VIR_STRCAT
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:40 a.m.
New subject: [libvirt] [PATCH 1/5] util: virstring: introduce virStrcat and VIR_STRCAT
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(a)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.
9:26 a.m.
New subject: [libvirt] [PATCH 2/5] util: use VIR_STRCAT instead of strcat
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:15 a.m.
New subject: [libvirt] [PATCH 2/5] util: use VIR_STRCAT instead of strcat
On Thu, Feb 23, 2017 at 04:26:57PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
10:22 a.m.
New subject: [libvirt] [PATCH 2/5] util: use VIR_STRCAT instead of strcat
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(a)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(a)redhat.com
>https://www.redhat.com/mailman/listinfo/libvir-list
10:25 a.m.
New subject: [libvirt] [PATCH 2/5] util: use VIR_STRCAT instead of strcat
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(a)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/ :|
9:26 a.m.
New subject: [libvirt] [PATCH 3/5] util: virbuffer: introduce virBufferEscapeN
Signed-off-by: Pavel Hrdina <phrdina(a)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
10:32 a.m.
New subject: [libvirt] [PATCH 3/5] util: virbuffer: introduce virBufferEscapeN
On Thu, Feb 23, 2017 at 04:26:58PM +0100, Pavel Hrdina wrote:
Signed-off-by: Pavel Hrdina <phrdina(a)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/ :|
10:51 a.m.
New subject: [libvirt] [PATCH 3/5] util: virbuffer: introduce virBufferEscapeN
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(a)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(a)redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list
11:55 a.m.
New subject: [libvirt] [PATCH 3/5] util: virbuffer: introduce virBufferEscapeN
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(a)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/ :|
9:26 a.m.
New subject: [libvirt] [PATCH 4/5] util: virqemu: introduce virQEMUBuildBufferEscape
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(a)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
9:27 a.m.
New subject: [libvirt] [PATCH 5/5] qemu: properly escape socket path for graphics
Resolves: https://bugzilla.redhat.com/show_bug.cgi?id=1352529
Signed-off-by: Pavel Hrdina <phrdina(a)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
2829
days inactive
2829
days old
15 comments
3 participants
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Pavel Hrdina