We already have two use cases for static assertions (and soon we'll have yet another). Namely: - STATIC_ASSERT_UNSIGNED_INT() in "checked-overflow.h". Here, we use our own trick, based on a negative-sized array typedef that's named with NBDKIT_UNIQUE_NAME. - static_assert() in "test-array-size.c". This uses the C11 macro called static_assert() from <assert.h>, which wraps the C11 _Static_assert(). This is not really great: our baseline is C99, not C11 (per commit 762f7c9e5166, "tests: Set minimum compiler to ISO C99.", 2021-04-08) -- which is why the same assertions are repeated in the code as normal runtime assert() calls, in case static_assert() is not defined. Factor out our own STATIC_ASSERT(), from STATIC_ASSERT_UNSIGNED_INT(). Put it to use in "test-array-size.c", replacing both the runtime assert()s and the compile-time static_assert()s. Note that for the latter, in order to remain consistent with STATIC_ASSERT_UNSIGNED_INT(), we no longer provide the *complaint* that we want the compiler to emit upon assertion failure, but an identifier that stands for the predicate that we *expect*. When uncommenting the negative test case in "test-array-size.c", the resultant wall of compiler diagnostics includes the following entry:

test-array-size.c:83:39: error: size of array ‘_array_size_macro_is_applied_to_array13’ is negative

This patch will have to be ported to nbdkit. (IMO we can implement the change in libnbd first -- the "common" subdir is meant to be common, so no particular precedence should be assumed.) Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- Notes: v5: - pick up Eric's R-b - no change v4: - new patch in v4 common/include/checked-overflow.h | 8 ++-- common/include/static-assert.h | 48 ++++++++++++++++++++ common/include/Makefile.am | 1 + common/include/test-array-size.c | 45 ++++++------------ 4 files changed, 67 insertions(+), 35 deletions(-) diff --git a/common/include/checked-overflow.h b/common/include/checked-overflow.h index a1852adcdc7a..4ec72387b332 100644 --- a/common/include/checked-overflow.h +++ b/common/include/checked-overflow.h @@ -53,6 +53,7 @@ #include <stdbool.h> #include <stdint.h> +#include "static-assert.h" #include "unique-name.h" /* Add "a" and "b", both of (possibly different) unsigned integer types, and @@ -173,11 +174,8 @@ * * The expression "x" is not evaluated, unless it has variably modified type. */ -#define STATIC_ASSERT_UNSIGNED_INT(x) \ - do { \ - typedef char NBDKIT_UNIQUE_NAME (_x_has_uint_type)[(typeof (x))-1 > 0 ? 1 : -1] \ - __attribute__ ((__unused__)); \ - } while (0) +#define STATIC_ASSERT_UNSIGNED_INT(x) \ + STATIC_ASSERT ((typeof (x))-1 > 0, _x_has_uint_type) /* Assign the sum "a + b" to "*r", using uintmax_t modular arithmetic. * diff --git a/common/include/static-assert.h b/common/include/static-assert.h new file mode 100644 index 000000000000..5a564f8946e5 --- /dev/null +++ b/common/include/static-assert.h @@ -0,0 +1,48 @@ +/* nbdkit + * Copyright (C) 2013-2023 Red Hat Inc. + * + * Redistribution and use in source and binary forms, with or without + * modification, are permitted provided that the following conditions are + * met: + * + * * Redistributions of source code must retain the above copyright + * notice, this list of conditions and the following disclaimer. + * + * * Redistributions in binary form must reproduce the above copyright + * notice, this list of conditions and the following disclaimer in the + * documentation and/or other materials provided with the distribution. + * + * * Neither the name of Red Hat nor the names of its contributors may be + * used to endorse or promote products derived from this software without + * specific prior written permission. + * + * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS IS'' AND + * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, + * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A + * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR + * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, + * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT + * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF + * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND + * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, + * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT + * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF + * SUCH DAMAGE. + */ + +#ifndef NBDKIT_STATIC_ASSERT_H +#define NBDKIT_STATIC_ASSERT_H + +#include "unique-name.h" + +/* Assert "expression" at compile time. If "expression" evaluates to zero (at + * compile time), produce a compiler error message that includes + * "expectation_id". + */ +#define STATIC_ASSERT(expression, expectation_id) \ + do { \ + typedef char NBDKIT_UNIQUE_NAME (expectation_id)[(expression) ? 1 : -1] \ + __attribute__ ((__unused__)); \ + } while (0) + +#endif /* NBDKIT_STATIC_ASSERT_H */ diff --git a/common/include/Makefile.am b/common/include/Makefile.am index fa2633c25ac3..8d6b2a88be13 100644 --- a/common/include/Makefile.am +++ b/common/include/Makefile.am @@ -28,6 +28,7 @@ EXTRA_DIST = \ iszero.h \ minmax.h \ rounding.h \ + static-assert.h \ unique-name.h \ $(NULL) diff --git a/common/include/test-array-size.c b/common/include/test-array-size.c index 8b0972aaabe1..6244ec1e7d9e 100644 --- a/common/include/test-array-size.c +++ b/common/include/test-array-size.c @@ -38,6 +38,7 @@ #include <assert.h> #include "array-size.h" +#include "static-assert.h" struct st { const char *s; int i; }; @@ -60,42 +61,26 @@ static struct st st4_0[4] __attribute__ ((__unused__)); int main (void) { - assert (ARRAY_SIZE (s0) == 0); - assert (ARRAY_SIZE (s1) == 1); - assert (ARRAY_SIZE (s3) == 3); - assert (ARRAY_SIZE (s4) == 4); - assert (ARRAY_SIZE (i0) == 0); - assert (ARRAY_SIZE (i1) == 1); - assert (ARRAY_SIZE (i3) == 3); - assert (ARRAY_SIZE (i4) == 4); - assert (ARRAY_SIZE (st0) == 0); - assert (ARRAY_SIZE (st1) == 1); - assert (ARRAY_SIZE (st3) == 3); - assert (ARRAY_SIZE (st4) == 4); - assert (ARRAY_SIZE (st4_0) == 4); - -#ifdef static_assert - static_assert (ARRAY_SIZE (s0) == 0, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (s1) == 1, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (s3) == 3, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (s4) == 4, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (i0) == 0, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (i1) == 1, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (i3) == 3, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (i4) == 4, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (st0) == 0, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (st1) == 1, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (st3) == 3, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (st4) == 4, "ARRAY_SIZE macro does not work"); - static_assert (ARRAY_SIZE (st4_0) == 4, "ARRAY_SIZE macro does not work"); -#endif + STATIC_ASSERT (ARRAY_SIZE (s0) == 0, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (s1) == 1, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (s3) == 3, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (s4) == 4, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (i0) == 0, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (i1) == 1, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (i3) == 3, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (i4) == 4, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (st0) == 0, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (st1) == 1, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (st3) == 3, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (st4) == 4, _array_size_macro_works); + STATIC_ASSERT (ARRAY_SIZE (st4_0) == 4, _array_size_macro_works); /* You can uncomment this to test the negative case. Unfortunately * it's difficult to automate this test. */ #if 0 int *p = i4; - assert (ARRAY_SIZE (p) == 4); + STATIC_ASSERT (ARRAY_SIZE (p) == 4, _array_size_macro_is_applied_to_array); #endif exit (EXIT_SUCCESS);

The "name##_iter" function is used 11 times in libnbd; in all those cases, "name" is "string_vector", and the "f" callback is "free": string_vector_iter (..., (void *) free); Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.) Furthermore, in all 11 cases, the freeing of the vector's strings is immediately followed by the release of the vector's array-of-pointers too. (This additional step is not expressed consistently across libnbd: it's sometimes spelled as free(vec.ptr), sometimes as string_vector_reset(&vec).) Introduce "name##_empty", which performs both steps at the same time. Keep the generic "name##_iter" function definition, as we'll want to synch this patch to nbdkit, and in nbdkit, "name##_iter" has other uses as well. Expose the "name##_empty" function definition with a new, separate macro: ADD_VECTOR_EMPTY_METHOD(). The existent DEFINE_VECTOR_TYPE() macro permits such element types that are not pointers, or are pointers to const- and/or volatile-qualified objects. Whereas "name##_empty" requires that the elements be pointers to dynamically allocated, non-const, non-volatile objects. Add DEFINE_POINTER_VECTOR_TYPE() that expands to both DEFINE_VECTOR_TYPE() and the additive ADD_VECTOR_EMPTY_METHOD(). ( For example, after typedef char foobar[5]; the following compiles (as expected): DEFINE_VECTOR_TYPE (foobar_vector, foobar); and the following fails to build (as expected): DEFINE_POINTER_VECTOR_TYPE (foobar_vector_bad, foobar); with the diagnostics including

In function ‘foobar_vector_bad_empty’: error: size of array ‘_vector_contains_pointers1’ is negative

) Switch "string_vector" to DEFINE_POINTER_VECTOR_TYPE(). The 11 string_vector_iter() call sites continue working; they will be converted to string_vector_empty() in a subsequent patch. Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v5: - resolve conflict from update to previous patch - force semicolon after ADD_VECTOR_EMPTY_METHOD() and DEFINE_POINTER_VECTOR_TYPE() too - update example macro invocations in the commit message v4: - rename DEFINE_VECTOR_EMPTY to ADD_VECTOR_EMPTY_METHOD - introduce DEFINE_POINTER_VECTOR_TYPE as a convenience macro for DEFINE_VECTOR_TYPE plus ADD_VECTOR_EMPTY_METHOD - keep "name##_iter" - statically assert in "name##_empty" that the vector contains pointers - redefine string_vector with DEFINE_POINTER_VECTOR_TYPE now, rather than augmenting it as DEFINE_VECTOR_TYPE + (pre-rename, additive) DEFINE_VECTOR_EMPTY - split the call site conversions to a separate patch common/utils/string-vector.h | 2 +- common/utils/vector.h | 35 ++++++++++++++++++++ 2 files changed, 36 insertions(+), 1 deletion(-) diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h index aa33fd48ceb5..115d7d484c71 100644 --- a/common/utils/string-vector.h +++ b/common/utils/string-vector.h @@ -37,6 +37,6 @@ #include "vector.h" -DEFINE_VECTOR_TYPE (string_vector, char *); +DEFINE_POINTER_VECTOR_TYPE (string_vector, char *); #endif /* STRING_VECTOR_H */ diff --git a/common/utils/vector.h b/common/utils/vector.h index b9b88ba02e7d..bdba02f7ebee 100644 --- a/common/utils/vector.h +++ b/common/utils/vector.h @@ -44,6 +44,9 @@ #include <assert.h> #include <string.h> +#include "compiler-macros.h" +#include "static-assert.h" + #ifdef __clang__ #pragma clang diagnostic ignored "-Wunused-function" #pragma clang diagnostic ignored "-Wduplicate-decl-specifier" @@ -183,6 +186,38 @@ #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 } +/* This macro should only be used if: + * - the vector contains pointers, and + * - the pointed-to objects are: + * - neither const- nor volatile-qualified, and + * - allocated with malloc() or equivalent. + */ +#define ADD_VECTOR_EMPTY_METHOD(name) \ + /* Call free() on each element of the vector, then reset the vector. \ + */ \ + static inline void __attribute__ ((__unused__)) \ + name##_empty (name *v) \ + { \ + size_t i; \ + for (i = 0; i < v->len; ++i) { \ + STATIC_ASSERT (TYPE_IS_POINTER (v->ptr[i]), \ + _vector_contains_pointers); \ + free (v->ptr[i]); \ + } \ + name##_reset (v); \ + } \ + \ + /* Force callers to supply ';'. */ \ + struct name + +/* Convenience macro tying together DEFINE_VECTOR_TYPE() and + * ADD_VECTOR_EMPTY_METHOD(). Inherit and forward the requirement for a + * trailing semicolon from ADD_VECTOR_EMPTY_METHOD() to the caller. + */ +#define DEFINE_POINTER_VECTOR_TYPE(name, type) \ + DEFINE_VECTOR_TYPE (name, type); \ + ADD_VECTOR_EMPTY_METHOD (name) + struct generic_vector { void *ptr; size_t len;

Convert the 11 string_vector_(iter(free) + reset()) call sites mentioned previously to string_vector_empty(). Note that the converted code performs more cleanup steps in some cases than strictly necessary, but the extra work is harmless, and arguably beneficial for clarity / consistency. Signed-off-by: Laszlo Ersek <lersek@redhat.com> Reviewed-by: Eric Blake <eblake@redhat.com> --- Notes: v5: - pick up Eric's R-b - no change v4: - split out from the previous [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY generator/states-connect-socket-activation.c | 9 +++------ lib/handle.c | 12 ++++-------- lib/utils.c | 6 ++---- common/utils/test-vector.c | 3 +-- info/show.c | 3 +-- 5 files changed, 11 insertions(+), 22 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 7138e7638e30..3b621b8be44f 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -91,8 +91,7 @@ prepare_socket_activation_environment (string_vector *env) err: set_error (errno, "malloc"); - string_vector_iter (env, (void *) free); - free (env->ptr); + string_vector_empty (env); return -1; } @@ -166,8 +165,7 @@ CONNECT_SA.START: SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); close (s); - string_vector_iter (&env, (void *) free); - free (env.ptr); + string_vector_empty (&env); return 0; } if (pid == 0) { /* child - run command */ @@ -210,8 +208,7 @@ CONNECT_SA.START: /* Parent. */ close (s); - string_vector_iter (&env, (void *) free); - free (env.ptr); + string_vector_empty (&env); h->pid = pid; h->connaddrlen = sizeof addr; diff --git a/lib/handle.c b/lib/handle.c index dfd8c2e5cdb9..fb92f16e6c91 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -128,8 +128,7 @@ nbd_close (struct nbd_handle *h) /* Free user callbacks first. */ nbd_unlocked_clear_debug_callback (h); - string_vector_iter (&h->querylist, (void *) free); - free (h->querylist.ptr); + string_vector_empty (&h->querylist); free (h->bs_entries); nbd_internal_reset_size_and_flags (h); for (i = 0; i < h->meta_contexts.len; ++i) @@ -139,8 +138,7 @@ nbd_close (struct nbd_handle *h) free_cmd_list (h->cmds_to_issue); free_cmd_list (h->cmds_in_flight); free_cmd_list (h->cmds_done); - string_vector_iter (&h->argv, (void *) free); - free (h->argv.ptr); + string_vector_empty (&h->argv); if (h->sact_sockpath) { if (h->pid > 0) kill (h->pid, SIGTERM); @@ -164,8 +162,7 @@ nbd_close (struct nbd_handle *h) free (h->tls_certificates); free (h->tls_username); free (h->tls_psk_file); - string_vector_iter (&h->request_meta_contexts, (void *) free); - free (h->request_meta_contexts.ptr); + string_vector_empty (&h->request_meta_contexts); free (h->hname); pthread_mutex_destroy (&h->lock); free (h); @@ -379,8 +376,7 @@ nbd_unlocked_get_meta_context (struct nbd_handle *h, size_t i) int nbd_unlocked_clear_meta_contexts (struct nbd_handle *h) { - string_vector_iter (&h->request_meta_contexts, (void *) free); - string_vector_reset (&h->request_meta_contexts); + string_vector_empty (&h->request_meta_contexts); return 0; } diff --git a/lib/utils.c b/lib/utils.c index c229ebfc6106..bba4b3846e77 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -93,8 +93,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv) return -1; } - string_vector_iter (&h->argv, (void *) free); - string_vector_reset (&h->argv); + string_vector_empty (&h->argv); if (nbd_internal_copy_string_list (&h->argv, argv) == -1) { set_error (errno, "realloc"); @@ -110,8 +109,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv) int nbd_internal_set_querylist (struct nbd_handle *h, char **queries) { - string_vector_iter (&h->querylist, (void *) free); - string_vector_reset (&h->querylist); + string_vector_empty (&h->querylist); if (queries) { if (nbd_internal_copy_string_list (&h->querylist, queries) == -1) { diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c index 55c8ebeb8a1e..05ac5cec3dba 100644 --- a/common/utils/test-vector.c +++ b/common/utils/test-vector.c @@ -151,8 +151,7 @@ test_string_vector (void) printf ("%s\n", v.ptr[i]); assert (i == 10); - string_vector_iter (&v, (void*)free); - string_vector_reset (&v); + string_vector_empty (&v); } static void diff --git a/info/show.c b/info/show.c index 4bf596715cf9..e6c9b9bf1243 100644 --- a/info/show.c +++ b/info/show.c @@ -275,8 +275,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc, fprintf (fp, "\t},\n"); } - string_vector_iter (&contexts, (void *) free); - free (contexts.ptr); + string_vector_empty (&contexts); free (content); free (export_name); free (export_desc);