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 <eblake@redhat.com>

diff --git a/HACKING b/HACKING
index b4dcd3da6227223fefc3d687b994939a8161019a..2bd6d692f9e2d6c5b4f68464351757f2f09d7cd5 100644 (file)
--- 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 8708cb3a72f3fcbc2fabc15eae0777fa649541e0..78959f3ad3dea833ec94b0db48ae2e5b667c00cd 100644 (file)
--- 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.
     </p>
 
     <p>
@@ -868,9 +868,10 @@
 </pre>
     <p>
       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 394a558587e66e3f472f44c731084e65a8532dea..ec124623b04b184e78e3f5e4c526b5365c809d59 100644 (file)
--- 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 4fba53b1bfe8e824b87cc96594235c9498203968..5bd6c185b0467d40a00afa168796bfb02f4d920c 100644 (file)
--- 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)