8:27 a.m.
From: Eric Blake <eblake(a)redhat.com>
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(a)redhat.com>
Conflicts:
HACKING
docs/hacking.html.in
(cherry picked from commit 6b74a9f5d98e066f8dfdf5d5ccda68230b516246)
---
HACKING | 17 +++++++++++------
docs/hacking.html.in | 16 ++++++++++++----
src/util/virstring.c | 12 ++++++++----
src/util/virstring.h | 22 ++++++++++++++++------
4 files changed, 47 insertions(+), 20 deletions(-)
diff --git a/HACKING b/HACKING
index 5b5c34b..758d327 100644
--- a/HACKING
+++ b/HACKING
@@ -234,6 +234,11 @@ But if negating a complex condition is too ugly, then at least add
braces:
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 and virstring.h.
+
For variadic macros, stick with C99 syntax:
#define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__)
@@ -539,12 +544,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 198afe7..d553f42 100644
--- a/docs/hacking.html.in
+++ b/docs/hacking.html.in
@@ -303,7 +303,14 @@
}
</pre>
- <h2><a href="types">Preprocessor</a></h2>
+ <h2><a name="preprocessor">Preprocessor</a></h2>
+
+ <p>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 and virstring.h.
+ </p>
<p>
For variadic macros, stick with C99 syntax:
@@ -647,9 +654,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 21142b4..915c771 100644
--- a/src/util/virstring.c
+++ b/src/util/virstring.c
@@ -194,7 +194,7 @@ size_t virStringListLength(char **strings)
* 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,
@@ -205,13 +205,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;
}
/**
@@ -231,7 +233,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,
@@ -243,11 +245,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 cd5ccda..74164bf 100644
--- a/src/util/virstring.h
+++ b/src/util/virstring.h
@@ -40,11 +40,11 @@ size_t virStringListLength(char **strings);
/* 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:
@@ -53,7 +53,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__)
@@ -65,7 +68,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)
@@ -78,7 +83,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__, \
@@ -93,7 +101,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)
--
1.8.5.2