[libnbd PATCH v4 0/5] vector: introduce DEFINE_POINTER_VECTOR_TYPE()
This reworks the single [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY http://mid.mail-archive.com/20230215141158.2426855-6-lersek@redhat.com according to the review comments I received from Eric and Rich in that subthread. Clearly, the more closely I look at anything, the more refactoring opportunities emerge. :/ At least, although I didn't like the amount of work needed, I quite like the results. :) This series too is bisectable. Laszlo Laszlo Ersek (5): common/include: add TYPE_IS_POINTER() macro common/include: extract STATIC_ASSERT() macro remove semicolon after DEFINE_VECTOR_TYPE() macro invocations vector: introduce DEFINE_POINTER_VECTOR_TYPE() convert string_vector_(iter(free) + reset()) to string_vector_empty() common/include/Makefile.am | 1 + common/include/checked-overflow.h | 8 ++-- common/include/compiler-macros.h | 31 ++++++++++++- common/include/static-assert.h | 48 ++++++++++++++++++++ common/include/test-array-size.c | 45 ++++++------------ common/utils/const-string-vector.h | 2 +- common/utils/nbdkit-string.h | 2 +- common/utils/string-vector.h | 2 +- common/utils/test-vector.c | 7 ++- common/utils/vector.h | 31 +++++++++++++ copy/nbdcopy.h | 2 +- generator/states-connect-socket-activation.c | 9 ++-- info/show.c | 3 +- lib/handle.c | 12 ++--- lib/internal.h | 2 +- lib/utils.c | 6 +-- 16 files changed, 145 insertions(+), 66 deletions(-) create mode 100644 common/include/static-assert.h base-commit: da8887fd4d7ddc1f9cf2f4a3bc20fd51578fa565
Because the current definition of TYPE_IS_ARRAY() already contains a negation, defining TYPE_IS_POINTER() in terms of TYPE_IS_ARRAY() would lead to double negation, which I consider less than ideal style. Therefore, introduce TYPE_IS_POINTER() from scratch, and rebase TYPE_IS_ARRAY() on top of TYPE_IS_POINTER(). Suggested-by: Richard W.M. Jones <rjones@redhat.com> Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v4: - new patch in v4 common/include/compiler-macros.h | 31 ++++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/common/include/compiler-macros.h b/common/include/compiler-macros.h index 7933bb87a5bf..beb3f4fe4e5c 100644 --- a/common/include/compiler-macros.h +++ b/common/include/compiler-macros.h @@ -50,12 +50,39 @@ #define BUILD_BUG_UNLESS_TRUE(cond) \ (BUILD_BUG_STRUCT_SIZE (cond) - BUILD_BUG_STRUCT_SIZE (cond)) -#define TYPE_IS_ARRAY(a) \ - (!__builtin_types_compatible_p (typeof (a), typeof (&(a)[0]))) +/* Each of TYPE_IS_POINTER() and TYPE_IS_ARRAY() produces a build failure if it + * is invoked with an object that has neither pointer-to-object type nor array + * type. + * + * C99 6.5.2.1 constrains one of the operands of the subscript operator to have + * pointer-to-object type, and the other operand to have integer type. In the + * replacement text of TYPE_IS_POINTER(), we use [0] as subscript (providing the + * integer operand), therefore the macro argument (p) is constrained to have + * pointer-to-object type. + * + * If TYPE_IS_POINTER() is invoked with a pointer that has pointer-to-object + * type, the constraint is directly satisfied, and TYPE_IS_POINTER() evaluates, + * at compile time, to 1. + * + * If TYPE_IS_POINTER() is invoked with an array, the constraint of the + * subscript operator is satisfied again -- because the array argument "decays" + * to a pointer to the array's initial element (C99 6.3.2p3) --, and + * TYPE_IS_POINTER() evaluates, at compile time, to 0. + * + * If TYPE_IS_POINTER() is invoked with an argument having any other type, then + * the subscript operator constraint is not satisfied, and C99 5.1.1.3p1 + * requires the emission of a diagnostic message -- the build breaks. Therefore, + * TYPE_IS_ARRAY() can be defined simply as the logical negation of + * TYPE_IS_POINTER(). + */ +#define TYPE_IS_POINTER(p) \ + (__builtin_types_compatible_p (typeof (p), typeof (&(p)[0]))) +#define TYPE_IS_ARRAY(a) (!TYPE_IS_POINTER (a)) #else /* __cplusplus */ #define BUILD_BUG_UNLESS_TRUE(cond) 0 +#define TYPE_IS_POINTER(p) 1 #define TYPE_IS_ARRAY(a) 1 #endif /* __cplusplus */
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> --- 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);
A semicolon after a DEFINE_VECTOR_TYPE(...) macro invocation is not needed, nor does our documentation in "common/utils/vector.h" prescribe one. Furthermore, it breaks ISO C, which gcc reports with "-Wpedantic":
warning: ISO C does not allow extra ‘;’ outside of a function [-Wpedantic]
Remove the semicolons. (Note that we have such DEFINE_VECTOR_TYPE() invocations too that are not followed by a semicolon, so our current usage is inconsistently wrong.) Signed-off-by: Laszlo Ersek <lersek@redhat.com> --- Notes: v4: - new patch in v4 lib/internal.h | 2 +- common/utils/const-string-vector.h | 2 +- common/utils/nbdkit-string.h | 2 +- common/utils/string-vector.h | 2 +- common/utils/test-vector.c | 4 ++-- copy/nbdcopy.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 0b5f793011b8..b88b43ec6e6b 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -71,7 +71,7 @@ struct meta_context { char *name; /* Name of meta context. */ uint32_t context_id; /* Context ID negotiated with the server. */ }; -DEFINE_VECTOR_TYPE (meta_vector, struct meta_context); +DEFINE_VECTOR_TYPE (meta_vector, struct meta_context) struct export { char *name; diff --git a/common/utils/const-string-vector.h b/common/utils/const-string-vector.h index c15f8952b3b3..d537fedd646f 100644 --- a/common/utils/const-string-vector.h +++ b/common/utils/const-string-vector.h @@ -37,6 +37,6 @@ #include "vector.h" -DEFINE_VECTOR_TYPE (const_string_vector, const char *); +DEFINE_VECTOR_TYPE (const_string_vector, const char *) #endif /* CONST_STRING_VECTOR_H */ diff --git a/common/utils/nbdkit-string.h b/common/utils/nbdkit-string.h index b85f05991a82..81bbe9f45784 100644 --- a/common/utils/nbdkit-string.h +++ b/common/utils/nbdkit-string.h @@ -37,6 +37,6 @@ #include "vector.h" -DEFINE_VECTOR_TYPE (string, char); +DEFINE_VECTOR_TYPE (string, char) #endif /* NBDKIT_STRING_H */ diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h index aa33fd48ceb5..373eb319120a 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_VECTOR_TYPE (string_vector, char *) #endif /* STRING_VECTOR_H */ diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c index 55c8ebeb8a1e..27c8da1c06ab 100644 --- a/common/utils/test-vector.c +++ b/common/utils/test-vector.c @@ -50,8 +50,8 @@ #define APPENDS 1000000 -DEFINE_VECTOR_TYPE (int64_vector, int64_t); -DEFINE_VECTOR_TYPE (uint32_vector, uint32_t); +DEFINE_VECTOR_TYPE (int64_vector, int64_t) +DEFINE_VECTOR_TYPE (uint32_vector, uint32_t) static int compare (const int64_t *a, const int64_t *b) diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h index b6257cc13951..5e1e078b34cb 100644 --- a/copy/nbdcopy.h +++ b/copy/nbdcopy.h @@ -116,7 +116,7 @@ struct extent { uint64_t length; bool zero; }; -DEFINE_VECTOR_TYPE (extent_list, struct extent); +DEFINE_VECTOR_TYPE (extent_list, struct extent) /* The operations struct hides some of the differences between local * file, NBD and pipes from the copying code.
On Fri, Feb 24, 2023 at 05:39:35AM +0100, Laszlo Ersek wrote:
A semicolon after a DEFINE_VECTOR_TYPE(...) macro invocation is not needed, nor does our documentation in "common/utils/vector.h" prescribe one. Furthermore, it breaks ISO C, which gcc reports with "-Wpedantic":
warning: ISO C does not allow extra ‘;’ outside of a function [-Wpedantic]
Remove the semicolons. (Note that we have such DEFINE_VECTOR_TYPE() invocations too that are not followed by a semicolon, so our current usage is inconsistently wrong.)
Signed-off-by: Laszlo Ersek <lersek@redhat.com> ---
Notes: v4: - new patch in v4
lib/internal.h | 2 +- common/utils/const-string-vector.h | 2 +- common/utils/nbdkit-string.h | 2 +- common/utils/string-vector.h | 2 +- common/utils/test-vector.c | 4 ++-- copy/nbdcopy.h | 2 +- 6 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h index 0b5f793011b8..b88b43ec6e6b 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -71,7 +71,7 @@ struct meta_context { char *name; /* Name of meta context. */ uint32_t context_id; /* Context ID negotiated with the server. */ }; -DEFINE_VECTOR_TYPE (meta_vector, struct meta_context); +DEFINE_VECTOR_TYPE (meta_vector, struct meta_context)
struct export {
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 Fri, Feb 24, 2023 at 09:04:29AM -0600, Eric Blake wrote:
On Fri, Feb 24, 2023 at 05:39:35AM +0100, Laszlo Ersek wrote:
A semicolon after a DEFINE_VECTOR_TYPE(...) macro invocation is not needed, nor does our documentation in "common/utils/vector.h" prescribe one. ...
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:
Oh, plus I'd also need to document the usage comment in vector.h to document this style, if we want to go with this approach. -- 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:
@@ -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 }
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
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: 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 | 31 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 1 deletion(-) diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h index 373eb319120a..d049a8d5b2b1 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 fb2482c853ff..38272925a65a 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" @@ -180,6 +183,34 @@ #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); \ + } + +/* Convenience macro tying together DEFINE_VECTOR_TYPE() and + * ADD_VECTOR_EMPTY_METHOD(). + */ +#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> --- 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);
On Fri, Feb 24, 2023 at 05:39:32AM +0100, Laszlo Ersek wrote:
This reworks the single
[libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY
http://mid.mail-archive.com/20230215141158.2426855-6-lersek@redhat.com
according to the review comments I received from Eric and Rich in that subthread.
Clearly, the more closely I look at anything, the more refactoring opportunities emerge. :/ At least, although I didn't like the amount of work needed, I quite like the results. :)
This series too is bisectable.
Laszlo
Laszlo Ersek (5): common/include: add TYPE_IS_POINTER() macro common/include: extract STATIC_ASSERT() macro remove semicolon after DEFINE_VECTOR_TYPE() macro invocations vector: introduce DEFINE_POINTER_VECTOR_TYPE() convert string_vector_(iter(free) + reset()) to string_vector_empty()
And turned out very nicely in my opinion. See my comments on 3. For 1,2,4,5: Reviewed-by: Eric Blake <eblake@redhat.com> -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org
participants (2)
-
Eric Blake -
Laszlo Ersek