From 6b74a9f5d98e066f8dfdf5d5ccda68230b516246 Mon Sep 17 00:00:00 2001 From: Eric Blake Date: Mon, 6 May 2013 20:42:44 -0600 Subject: [PATCH] string: make VIR_STRDUP easier to use While reviewing proposed VIR_STRDUP conversions, I've already noticed several places that do: if (str && VIR_STRDUP(dest, str) < 0) which can be simplified by allowing str to be NULL (something that strdup() doesn't allow). Meanwhile, code that wants to ensure a non-NULL dest regardless of the source can check for <= 0. Also, make it part of the VIR_STRDUP contract that macro arguments are evaluated exactly once. * src/util/virstring.h (VIR_STRDUP, VIR_STRDUP_QUIET, VIR_STRNDUP) (VIR_STRNDUP_QUIET): Improve contract. * src/util/virstring.c (virStrdup, virStrndup): Change return conventions. * docs/hacking.html.in: Document this. * HACKING: Regenerate. Signed-off-by: Eric Blake --- HACKING | 14 +++++++------- docs/hacking.html.in | 9 +++++---- src/util/virstring.c | 12 ++++++++---- src/util/virstring.h | 22 ++++++++++++++++------ 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/HACKING b/HACKING index b4dcd3da62..2bd6d692f9 100644 --- a/HACKING +++ b/HACKING @@ -421,7 +421,7 @@ Preprocessor Macros defined with an ALL_CAPS name should generally be assumed to be unsafe with regards to arguments with side-effects (that is, MAX(a++, b--) might increment a or decrement b too many or too few times). Exceptions to this rule -are explicitly documented for macros in viralloc.h. +are explicitly documented for macros in viralloc.h and virstring.h. For variadic macros, stick with C99 syntax: @@ -728,12 +728,12 @@ virStrncpy(dest, src, strlen(src), sizeof(dest)). VIR_STRNDUP(char *dst, const char *src, size_t n); You should avoid using strdup or strndup directly as they do not report -out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, that -these two behave similar to VIR_ALLOC: on success zero is returned, otherwise -the result is -1 and dst is guaranteed to be NULL. In very specific cases, -when you don't want to report the out-of-memory error, you can use -VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually -considered a flaw. +out-of-memory error, and do not allow a NULL source. Use VIR_STRDUP or +VIR_STRNDUP macros instead, which return 0 for NULL source, 1 for successful +copy, and -1 for allocation failure with the error already reported. In very +specific cases, when you don't want to report the out-of-memory error, you can +use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and +usually considered a flaw. Variable length string buffer diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 8708cb3a72..78959f3ad3 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -523,7 +523,7 @@ assumed to be unsafe with regards to arguments with side-effects (that is, MAX(a++, b--) might increment a or decrement b too many or too few times). Exceptions to this rule are explicitly - documented for macros in viralloc.h. + documented for macros in viralloc.h and virstring.h.

@@ -868,9 +868,10 @@

You should avoid using strdup or strndup directly as they do not report - out-of-memory error. Use VIR_STRDUP or VIR_STRNDUP macros instead. Note, - that these two behave similar to VIR_ALLOC: on success zero is returned, - otherwise the result is -1 and dst is guaranteed to be NULL. In very + out-of-memory error, and do not allow a NULL source. Use + VIR_STRDUP or VIR_STRNDUP macros instead, which return 0 for + NULL source, 1 for successful copy, and -1 for allocation + failure with the error already reported. In very specific cases, when you don't want to report the out-of-memory error, you can use VIR_STRDUP_QUIET or VIR_STRNDUP_QUIET, but such usage is very rare and usually considered a flaw. diff --git a/src/util/virstring.c b/src/util/virstring.c index 394a558587..ec124623b0 100644 --- a/src/util/virstring.c +++ b/src/util/virstring.c @@ -532,7 +532,7 @@ virArgvToString(const char *const *argv) * caller's body where virStrdup is called from. Consider * using VIR_STRDUP which sets these automatically. * - * Returns: 0 on success, -1 otherwise. + * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. */ int virStrdup(char **dest, @@ -543,13 +543,15 @@ virStrdup(char **dest, const char *funcname, size_t linenr) { + if (!src) + return 0; if (!(*dest = strdup(src))) { if (report) virReportOOMErrorFull(domcode, filename, funcname, linenr); return -1; } - return 0; + return 1; } /** @@ -569,7 +571,7 @@ virStrdup(char **dest, * caller's body where virStrndup is called from. Consider * using VIR_STRNDUP which sets these automatically. * - * Returns: 0 on success, -1 otherwise. + * Returns: 0 for NULL src, 1 on successful copy, -1 otherwise. */ int virStrndup(char **dest, @@ -581,11 +583,13 @@ virStrndup(char **dest, const char *funcname, size_t linenr) { + if (!src) + return 0; if (!(*dest = strndup(src, n))) { if (report) virReportOOMErrorFull(domcode, filename, funcname, linenr); return -1; } - return 0; + return 1; } diff --git a/src/util/virstring.h b/src/util/virstring.h index 4fba53b1bf..5bd6c185b0 100644 --- a/src/util/virstring.h +++ b/src/util/virstring.h @@ -92,11 +92,11 @@ char *virStrcpy(char *dest, const char *src, size_t destbytes) /* Don't call these directly - use the macros below */ int virStrdup(char **dest, const char *src, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, const char *filename, const char *funcname, size_t linenr) - ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1) ATTRIBUTE_NONNULL(2); + ATTRIBUTE_RETURN_CHECK ATTRIBUTE_NONNULL(1); /** * VIR_STRDUP: @@ -105,7 +105,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * * Duplicate @src string and store it into @dst. * - * Returns -1 on failure (with OOM error reported), 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure (with OOM error reported), 0 if @src was NULL, + * 1 if @src was copied */ # define VIR_STRDUP(dst, src) virStrdup(&(dst), src, true, VIR_FROM_THIS, \ __FILE__, __FUNCTION__, __LINE__) @@ -117,7 +120,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * * Duplicate @src string and store it into @dst. * - * Returns -1 on failure, 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied */ # define VIR_STRDUP_QUIET(dst, src) virStrdup(&(dst), src, false, 0, NULL, NULL, 0) @@ -130,7 +135,10 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * Duplicate @src string and store it into @dst. If @src is longer than @n, * only @n bytes are copied and terminating null byte '\0' is added. * - * Returns -1 on failure (with OOM error reported), 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure (with OOM error reported), 0 if @src was NULL, + * 1 if @src was copied */ # define VIR_STRNDUP(dst, src, n) virStrndup(&(dst), src, n, true, \ VIR_FROM_THIS, __FILE__, \ @@ -145,7 +153,9 @@ int virStrndup(char **dest, const char *src, size_t n, bool report, int domcode, * Duplicate @src string and store it into @dst. If @src is longer than @n, * only @n bytes are copied and terminating null byte '\0' is added. * - * Returns -1 on failure, 0 on success + * This macro is safe to use on arguments with side effects. + * + * Returns -1 on failure, 0 if @src was NULL, 1 if @src was copied */ # define VIR_STRNDUP_QUIET(dst, src, n) virStrndup(&(dst), src, n, false, \ 0, NULL, NULL, 0)