[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [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 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.

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]