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: 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(a)redhat.com&gt; --- Notes: 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);

On Fri, Feb 24, 2023 at 05:39:35AM +0100, Laszlo Ersek wrote: Emacs (and probably other editors with auto-indent features) has a tough time with this style choice. I agree that because the macro ends with a function definition for name##_search(){}, neither including a trailing semicolon in the macro nor supplying one after expanding the macro is syntactically necessary. But without the semicolon after the invocation, emacs thinks that DEFINE_VECTOR_TYPE() starts an incomplete declaration, and hitting <TAB> on the next text ('struct export {') causes indentation by two more spaces as a continuation. I agree that our code base is inconsistent on whether we supply or omit a trailing semicolon, but I would also like to appease the editor syntax engine. What if we instead go for a consistent style in the opposite direction? My counter-proposal: hack the definition of the macro to REQUIRE that callers include a trailing semicolon. Here's the easiest way I could think of: diff --git c/common/utils/vector.h i/common/utils/vector.h index fb2482c8..00b6a0da 100644 --- c/common/utils/vector.h +++ i/common/utils/vector.h @@ -1,5 +1,5 @@ /* nbdkit - * Copyright (C) 2020-2022 Red Hat Inc. + * Copyright (C) 2020-2023 Red Hat Inc. * * Redistribution and use in source and binary forms, with or without * modification, are permitted provided that the following conditions are @@ -176,7 +176,10 @@ { \ return bsearch (key, v->ptr, v->len, sizeof (type), \ (void *) compare); \ - } + } \ + \ + /* End with duplicate declaration, so callers must supply ';' */ \ + typedef struct name name #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 } diff --git c/lib/uri.c i/lib/uri.c index 367621d4..31ee90f3 100644 --- c/lib/uri.c +++ i/lib/uri.c @@ -58,7 +58,7 @@ struct uri_query { char *value; }; -DEFINE_VECTOR_TYPE (uri_query_list, struct uri_query) +DEFINE_VECTOR_TYPE (uri_query_list, struct uri_query); /* Parse the query_raw substring of a URI into a list of decoded queries. * Return 0 on success or -1 on error. diff --git c/copy/file-ops.c i/copy/file-ops.c index 18cae74a..1efece26 100644 --- c/copy/file-ops.c +++ i/copy/file-ops.c @@ -64,7 +64,7 @@ #endif #ifdef PAGE_CACHE_MAPPING -DEFINE_VECTOR_TYPE (byte_vector, uint8_t) +DEFINE_VECTOR_TYPE (byte_vector, uint8_t); #endif static struct rw_ops file_ops; diff --git c/copy/nbd-ops.c i/copy/nbd-ops.c index 34ab4857..0980a5ed 100644 --- c/copy/nbd-ops.c +++ i/copy/nbd-ops.c @@ -33,7 +33,7 @@ static struct rw_ops nbd_ops; -DEFINE_VECTOR_TYPE (handles, struct nbd_handle *) +DEFINE_VECTOR_TYPE (handles, struct nbd_handle *); struct rw_nbd { struct rw rw; diff --git c/dump/dump.c i/dump/dump.c index d0da2879..922bd435 100644 --- c/dump/dump.c +++ i/dump/dump.c @@ -38,7 +38,7 @@ #include "version.h" #include "vector.h" -DEFINE_VECTOR_TYPE (uint32_vector, uint32_t) +DEFINE_VECTOR_TYPE (uint32_vector, uint32_t); static const char *progname; static struct nbd_handle *nbd; diff --git c/fuse/nbdfuse.h i/fuse/nbdfuse.h index 371ca8bb..93b66aac 100644 --- c/fuse/nbdfuse.h +++ i/fuse/nbdfuse.h @@ -30,7 +30,7 @@ #include "vector.h" -DEFINE_VECTOR_TYPE (handles, struct nbd_handle *) +DEFINE_VECTOR_TYPE (handles, struct nbd_handle *); extern handles nbd; extern unsigned connections; diff --git c/info/list.c i/info/list.c index c2741ba0..4b4e8f86 100644 --- c/info/list.c +++ i/info/list.c @@ -35,7 +35,7 @@ struct export { char *name; char *desc; }; -DEFINE_VECTOR_TYPE (exports, struct export) +DEFINE_VECTOR_TYPE (exports, struct export); static exports export_list = empty_vector; static int diff --git c/info/map.c i/info/map.c index a5aad955..4924866a 100644 --- c/info/map.c +++ i/info/map.c @@ -36,7 +36,7 @@ #include "nbdinfo.h" -DEFINE_VECTOR_TYPE (uint32_vector, uint32_t) +DEFINE_VECTOR_TYPE (uint32_vector, uint32_t); static void print_extents (uint32_vector *entries); static void print_totals (uint32_vector *entries, int64_t size); -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On 2/24/23 16:04, Eric Blake wrote: This does not conform to C99. In C99 6.7 "Declarations" p3 (under "Constraints"), we have If an identifier has no linkage, there shall be no more than one declaration of the identifier (in a declarator or type specifier) with the same scope and in the same name space, except for tags as specified in 6.7.2.3. And in p5 (under "Semantics"), we have A declaration specifies the interpretation and attributes of a set of identifiers. A /definition/ of an identifier is a declaration for that identifier that: — for an object, causes storage to be reserved for that object; — for a function, includes the function body; 101) — for an enumeration constant or typedef name, is the (only) declaration of the identifier. Consider the source code "typedef.c": ----- struct s { int x; }; typedef struct s s; typedef struct s s; ----- When built with $ gcc -std=c99 -pedantic -fsyntax-only typedef.c we get ----- typedef.c:3:18: warning: redefinition of typedef ‘s’ [-Wpedantic] 3 | typedef struct s s; | ^ typedef.c:2:18: note: previous declaration of ‘s’ with type ‘s’ 2 | typedef struct s s; | ^ ----- What does work is plain "struct s" though: ----- struct s { int x; }; typedef struct s s; struct s; ----- and that's because of the above-cited C99 6.7.2.3 reference (specifically p9): If a type specifier of the form struct-or-union identifier or enum identifier occurs other than as part of one of the above forms, and a declaration of the identifier as a tag is visible, then it specifies the same type as that other declaration, and does not redeclare the tag. Laszlo

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(a)redhat.com&gt; --- Notes: 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 27c8da1c06ab..8143669e95b9 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);