[libvirt] [PATCH 0/2] VIR_STRDUP ease of use
As pointed out during my (ongoing) review of Michal's series, I think that letting VIR_STRDUP take a NULL source can make it easier to use; I also think that documenting the guarantee that it evaluates arguments exactly once is worth enforcing. Hmm, I guess that means I should write a 3/2 patch that enhances the testsuite to intentionally do a side effect and show that it happens exactly once. Eric Blake (2): alloc: make VIR_APPEND_ELEMENT safer string: make VIR_STRDUP easier to use HACKING | 19 ++++++++++++------- docs/hacking.html.in | 16 ++++++++++++---- src/util/viralloc.c | 9 ++++++--- src/util/viralloc.h | 36 +++++++++++++++++++++++++++--------- src/util/virstring.c | 12 ++++++++---- src/util/virstring.h | 22 ++++++++++++++++------ 6 files changed, 81 insertions(+), 33 deletions(-) -- 1.8.1.4
VIR_APPEND_ELEMENT(array, size, elem) was not safe if the expression for 'size' had side effects. While no one in the current code base was trying to pass side effects, we might as well be robust and explicitly document our intentions. * src/util/viralloc.c (virInsertElementsN): Add special case. * src/util/viralloc.h (VIR_APPEND_ELEMENT): Use it. (VIR_ALLOC, VIR_ALLOC_N, VIR_REALLOC_N, VIR_EXPAND_N) (VIR_RESIZE_N, VIR_SHRINK_N, VIR_INSERT_ELEMENT) (VIR_DELETE_ELEMENT, VIR_ALLOC_VAR, VIR_FREE): Document which macros are safe in the presence of side effects. * docs/hacking.html.in: Document this. * HACKING: Regenerate. Signed-off-by: Eric Blake <eblake@redhat.com> --- HACKING | 7 ++++++- docs/hacking.html.in | 9 ++++++++- src/util/viralloc.c | 9 ++++++--- src/util/viralloc.h | 36 +++++++++++++++++++++++++++--------- 4 files changed, 47 insertions(+), 14 deletions(-) diff --git a/HACKING b/HACKING index b70b89d..b4dcd3d 100644 --- a/HACKING +++ b/HACKING @@ -418,6 +418,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. + For variadic macros, stick with C99 syntax: #define vshPrint(_ctl, ...) fprintf(stdout, __VA_ARGS__) @@ -501,7 +506,7 @@ Low level memory management Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt codebase, because they encourage a number of serious coding bugs and do not enable compile time verification of checks for NULL. Instead of these -routines, use the macros from memory.h. +routines, use the macros from viralloc.h. - To allocate a single object: diff --git a/docs/hacking.html.in b/docs/hacking.html.in index b15d187..8708cb3 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -519,6 +519,13 @@ <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. + </p> + <p> For variadic macros, stick with C99 syntax: </p> @@ -616,7 +623,7 @@ Use of the malloc/free/realloc/calloc APIs is deprecated in the libvirt codebase, because they encourage a number of serious coding bugs and do not enable compile time verification of checks for NULL. Instead of these - routines, use the macros from memory.h. + routines, use the macros from viralloc.h. </p> <ul> diff --git a/src/util/viralloc.c b/src/util/viralloc.c index 8f219bf..8d6a7e6 100644 --- a/src/util/viralloc.c +++ b/src/util/viralloc.c @@ -281,7 +281,7 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) * virInsertElementsN: * @ptrptr: pointer to hold address of allocated memory * @size: the size of one element in bytes - * @at: index within array where new elements should be added + * @at: index within array where new elements should be added, -1 for end * @countptr: variable tracking number of elements currently allocated * @add: number of elements to add * @newelems: pointer to array of one or more new elements to move into @@ -300,7 +300,8 @@ void virShrinkN(void *ptrptr, size_t size, size_t *countptr, size_t toremove) * items from newelems into ptrptr[at], then store the address of * allocated memory in *ptrptr and the new size in *countptr. If * newelems is NULL, the new elements at ptrptr[at] are instead filled - * with zero. + * with zero. at must be between [0,*countptr], except that -1 is + * treated the same as *countptr for convenience. * * Returns -1 on failure, 0 on success */ @@ -310,7 +311,9 @@ virInsertElementsN(void *ptrptr, size_t size, size_t at, size_t add, void *newelems, bool clearOriginal, bool inPlace) { - if (at > *countptr) { + if (at == -1) { + at = *countptr; + } else if (at > *countptr) { VIR_WARN("out of bounds index - count %zu at %zu add %zu", *countptr, at, add); return -1; diff --git a/src/util/viralloc.h b/src/util/viralloc.h index 7be7f82..35d3a37 100644 --- a/src/util/viralloc.h +++ b/src/util/viralloc.h @@ -80,6 +80,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * the address of allocated memory in 'ptr'. Fill the * newly allocated memory with zeros. * + * This macro is safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_ALLOC(ptr) virAlloc(&(ptr), sizeof(*(ptr))) @@ -93,6 +95,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * bytes long and store the address of allocated memory in * 'ptr'. Fill the newly allocated memory with zeros. * + * This macro is safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_ALLOC_N(ptr, count) virAllocN(&(ptr), sizeof(*(ptr)), (count)) @@ -106,6 +110,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * bytes long and store the address of allocated memory in * 'ptr'. If 'ptr' grew, the added memory is uninitialized. * + * This macro is safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_REALLOC_N(ptr, count) virReallocN(&(ptr), sizeof(*(ptr)), (count)) @@ -121,6 +127,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * address of allocated memory in 'ptr' and the new size in 'count'. * The new elements are filled with zero. * + * This macro is safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_EXPAND_N(ptr, count, add) \ @@ -144,6 +152,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * the address of allocated memory in 'ptr' and the new size in 'alloc'. * The new elements are filled with zero. * + * This macro is safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_RESIZE_N(ptr, alloc, count, add) \ @@ -160,6 +170,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * address of allocated memory in 'ptr' and the new size in 'count'. * If 'count' <= 'remove', the entire array is freed. * + * This macro is safe to use on arguments with side effects. + * * No return value. */ # define VIR_SHRINK_N(ptr, count, remove) \ @@ -236,9 +248,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be * assured ptr and &newelem are of compatible types. * - * Returns -1 on failure, 0 on success - * + * These macros are safe to use on arguments with side effects. * + * Returns -1 on failure, 0 on success */ # define VIR_INSERT_ELEMENT(ptr, at, count, newelem) \ virInsertElementsN(&(ptr), sizeof(*(ptr)), at, &(count), \ @@ -284,21 +296,21 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * replacing it with VIR_TYPECHECK(ptr, &newelem) so that we can be * assured ptr and &newelem are of compatible types. * - * Returns -1 on failure, 0 on success - * + * These macros are safe to use on arguments with side effects. * + * Returns -1 on failure, 0 on success */ # define VIR_APPEND_ELEMENT(ptr, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, false) # define VIR_APPEND_ELEMENT_COPY(ptr, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, false) # define VIR_APPEND_ELEMENT_INPLACE(ptr, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), true, true) # define VIR_APPEND_ELEMENT_COPY_INPLACE(ptr, count, newelem) \ - virInsertElementsN(&(ptr), sizeof(*(ptr)), count, &(count), \ + virInsertElementsN(&(ptr), sizeof(*(ptr)), -1, &(count), \ VIR_TYPEMATCH(ptr, &(newelem)), &(newelem), false, true) /** @@ -315,6 +327,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * VIR_DELETE_ELEMENT_INPLACE is identical, but assumes any * necessary memory re-allocation will be done later. * + * These macros are safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_DELETE_ELEMENT(ptr, at, count) \ @@ -348,7 +362,9 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * that will be returned and allocate a suitable buffer to contain the * returned data. C99 refers to these variable length objects as * structs containing flexible array members. - + * + * This macro is safe to use on arguments with side effects. + * * Returns -1 on failure, 0 on success */ # define VIR_ALLOC_VAR(ptr, type, count) \ @@ -360,6 +376,8 @@ void virFree(void *ptrptr) ATTRIBUTE_NONNULL(1); * * Free the memory stored in 'ptr' and update to point * to NULL. + * + * This macro is safe to use on arguments with side effects. */ # if !STATIC_ANALYSIS /* The ternary ensures that ptr is a pointer and not an integer type, -- 1.8.1.4
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> --- 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 b4dcd3d..2bd6d69 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 8708cb3..78959f3 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. </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 394a558..ec12462 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 4fba53b..5bd6c18 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) -- 1.8.1.4
On 07.05.2013 05:24, Eric Blake wrote:
As pointed out during my (ongoing) review of Michal's series, I think that letting VIR_STRDUP take a NULL source can make it easier to use; I also think that documenting the guarantee that it evaluates arguments exactly once is worth enforcing.
Hmm, I guess that means I should write a 3/2 patch that enhances the testsuite to intentionally do a side effect and show that it happens exactly once.
Eric Blake (2): alloc: make VIR_APPEND_ELEMENT safer string: make VIR_STRDUP easier to use
HACKING | 19 ++++++++++++------- docs/hacking.html.in | 16 ++++++++++++---- src/util/viralloc.c | 9 ++++++--- src/util/viralloc.h | 36 +++++++++++++++++++++++++++--------- src/util/virstring.c | 12 ++++++++---- src/util/virstring.h | 22 ++++++++++++++++------ 6 files changed, 81 insertions(+), 33 deletions(-)
ACK series Michal
On 05/07/2013 03:37 AM, Michal Privoznik wrote:
On 07.05.2013 05:24, Eric Blake wrote:
As pointed out during my (ongoing) review of Michal's series, I think that letting VIR_STRDUP take a NULL source can make it easier to use; I also think that documenting the guarantee that it evaluates arguments exactly once is worth enforcing.
Hmm, I guess that means I should write a 3/2 patch that enhances the testsuite to intentionally do a side effect and show that it happens exactly once.
Eric Blake (2): alloc: make VIR_APPEND_ELEMENT safer string: make VIR_STRDUP easier to use
HACKING | 19 ++++++++++++------- docs/hacking.html.in | 16 ++++++++++++---- src/util/viralloc.c | 9 ++++++--- src/util/viralloc.h | 36 +++++++++++++++++++++++++++--------- src/util/virstring.c | 12 ++++++++---- src/util/virstring.h | 22 ++++++++++++++++------ 6 files changed, 81 insertions(+), 33 deletions(-)
ACK series
Pushed, and I'm now working on a 3/2 patch for improving the testsuite. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
The surest way to avoid regressions is to test documented behavior :) * tests/virstringtest.c (testStrdup): New test case. Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstringtest.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/tests/virstringtest.c b/tests/virstringtest.c index db1e8a9..159566b 100644 --- a/tests/virstringtest.c +++ b/tests/virstringtest.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2012 Red Hat, Inc. + * Copyright (C) 2012-2013 Red Hat, Inc. * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Lesser General Public @@ -107,6 +107,51 @@ cleanup: return ret; } +static int +testStrdup(const void *data ATTRIBUTE_UNUSED) +{ + char *array[] = { NULL, NULL }; + size_t i = 0; + int ret = -1; + int value; + + value = VIR_STRDUP(array[i++], "hello"); + if (value != 1) { + fprintf(stderr, "unexpected strdup result %d, expected 1\n", value); + goto cleanup; + } + if (i != 1) { + fprintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); + goto cleanup; + } + if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + value = VIR_STRNDUP(array[i++], NULL, 5); + if (value != 0) { + fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); + goto cleanup; + } + if (i != 2) { + fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); + goto cleanup; + } + if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + ret = 0; +cleanup: + for (i = 0; i < ARRAY_CARDINALITY(array); i++) + VIR_FREE(array[i]); + return ret; +} + static int mymain(void) @@ -153,6 +198,8 @@ mymain(void) const char *tokens7[] = { "The", "quick", "brown", "fox", "", NULL }; TEST_SPLIT("The quick brown fox ", " ", 0, tokens7); + if (virtTestRun("strdup", 1, testStrdup, NULL) < 0) + ret = -1; return ret==0 ? EXIT_SUCCESS : EXIT_FAILURE; } -- 1.8.1.4
On 05/07/2013 11:42 PM, Eric Blake wrote:
The surest way to avoid regressions is to test documented behavior :)
* tests/virstringtest.c (testStrdup): New test case.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstringtest.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)
...
+ value = VIR_STRNDUP(array[i++], NULL, 5); + if (value != 0) { + fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); + goto cleanup; + } + if (i != 2) { + fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); + goto cleanup; + }
Should we test the side effects for the other two arguments as well?
+ if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + ret = 0;
ACK Jan
On 05/14/2013 06:50 AM, Ján Tomko wrote:
On 05/07/2013 11:42 PM, Eric Blake wrote:
The surest way to avoid regressions is to test documented behavior :)
* tests/virstringtest.c (testStrdup): New test case.
Signed-off-by: Eric Blake <eblake@redhat.com> --- tests/virstringtest.c | 49 ++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-)
...
+ value = VIR_STRNDUP(array[i++], NULL, 5); + if (value != 0) { + fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); + goto cleanup; + } + if (i != 2) { + fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); + goto cleanup; + }
Should we test the side effects for the other two arguments as well?
Ooh, good idea.
+ if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { + fprintf(stderr, "incorrect array contents '%s' '%s'\n", + NULLSTR(array[0]), NULLSTR(array[1])); + goto cleanup; + } + + ret = 0;
ACK
I'm squashing this in (it still passes, and now exercises more side effects), then pushing. diff --git i/tests/virstringtest.c w/tests/virstringtest.c index 359cb9a..da06c0f 100644 --- i/tests/virstringtest.c +++ w/tests/virstringtest.c @@ -107,15 +107,41 @@ cleanup: return ret; } +static bool fail; + +static const char * +testStrdupLookup1(size_t i) +{ + switch (i) { + case 0: + return "hello"; + case 1: + return NULL; + default: + fail = true; + return "oops"; + } +} + +static size_t +testStrdupLookup2(size_t i) +{ + if (i) + fail = true; + return 5; +} + static int testStrdup(const void *data ATTRIBUTE_UNUSED) { char *array[] = { NULL, NULL }; size_t i = 0; + size_t j = 0; + size_t k = 0; int ret = -1; int value; - value = VIR_STRDUP(array[i++], "hello"); + value = VIR_STRDUP(array[i++], testStrdupLookup1(j++)); if (value != 1) { fprintf(stderr, "unexpected strdup result %d, expected 1\n", value); goto cleanup; @@ -124,13 +150,18 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) fprintf(stderr, "unexpected side effects i=%zu, expected 1\n", i); goto cleanup; } + if (j != 1) { + fprintf(stderr, "unexpected side effects j=%zu, expected 1\n", j); + goto cleanup; + } if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { fprintf(stderr, "incorrect array contents '%s' '%s'\n", NULLSTR(array[0]), NULLSTR(array[1])); goto cleanup; } - value = VIR_STRNDUP(array[i++], NULL, 5); + value = VIR_STRNDUP(array[i++], testStrdupLookup1(j++), + testStrdupLookup2(k++)); if (value != 0) { fprintf(stderr, "unexpected strdup result %d, expected 0\n", value); goto cleanup; @@ -139,12 +170,25 @@ testStrdup(const void *data ATTRIBUTE_UNUSED) fprintf(stderr, "unexpected side effects i=%zu, expected 2\n", i); goto cleanup; } + if (j != 2) { + fprintf(stderr, "unexpected side effects j=%zu, expected 2\n", j); + goto cleanup; + } + if (k != 1) { + fprintf(stderr, "unexpected side effects k=%zu, expected 1\n", k); + goto cleanup; + } if (STRNEQ_NULLABLE(array[0], "hello") || array[1]) { fprintf(stderr, "incorrect array contents '%s' '%s'\n", NULLSTR(array[0]), NULLSTR(array[1])); goto cleanup; } + if (fail) { + fprintf(stderr, "side effects failed\n"); + goto cleanup; + } + ret = 0; cleanup: for (i = 0; i < ARRAY_CARDINALITY(array); i++) -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Ján Tomko -
Michal Privoznik