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.
> 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
Attachment:
signature.asc
Description: Digital signature
-- libvir-list mailing list libvir-list@xxxxxxxxxx https://www.redhat.com/mailman/listinfo/libvir-list
