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

--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list

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
--
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.
--
libvir-list mailing list
libvir-list@redhat.com
https://www.redhat.com/mailman/listinfo/libvir-list