[libvirt] [PATCH 1/5] util: virstring: introduce virStrcat and VIR_STRCAT

[Pavel Hrdina]

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 at 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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20170223/cd8f80e2/attachment-0001.sig>