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

[Martin Kletzander <mkletzan@xxxxxxxxxx>]

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

Attachment: signature.asc
Description: Digital signature

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