Libguestfs - Feb 2023 - [libnbd PATCH v5 5/5] convert string_vector_(iter(free) + reset()) to string_vector_empty()

V4:
http://mid.mail-archive.com/20230224043937.392235-1-lersek at redhat.com

Please see the Notes section in each patch for the updates.

Laszlo

Laszlo Ersek (5):
  common/include: add TYPE_IS_POINTER() macro
  common/include: extract STATIC_ASSERT() macro
  force 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/string-vector.h                 |  2 +-
 common/utils/test-vector.c                   |  3 +-
 common/utils/vector.h                        | 42 ++++++++++++++++-
 copy/file-ops.c                              |  2 +-
 copy/nbd-ops.c                               |  2 +-
 dump/dump.c                                  |  2 +-
 fuse/nbdfuse.h                               |  2 +-
 generator/states-connect-socket-activation.c |  9 ++--
 info/list.c                                  |  2 +-
 info/map.c                                   |  2 +-
 info/show.c                                  |  3 +-
 lib/handle.c                                 | 12 ++---
 lib/uri.c                                    |  2 +-
 lib/utils.c                                  |  6 +--
 ublk/nbdublk.h                               |  2 +-
 ublk/tgt.c                                   |  2 +-
 21 files changed, 157 insertions(+), 71 deletions(-)
 create mode 100644 common/include/static-assert.h


base-commit: 594f7a738e0aba23eda37965c96b3df6b8f76960

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 at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
Reviewed-by: Eric Blake <eblake at redhat.com>
---

Notes:
    v5:
    - pick up Eric's R-b
    - no change
    
    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 at redhat.com>
Reviewed-by: Eric Blake <eblake at 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);

Currently, 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]
Our current usage is inconsistent; a proper subset of all our
DEFINE_VECTOR_TYPE() invocations is succeeded by a semicolon. We could
remove these semicolons, but that doesn't play nicely with Emacs's C
language parser. Thus, force the semicolon instead.

Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---

Notes:
    v5:
    - force semicolon after DEFINE_VECTOR_TYPE() [Eric]
    - reword commit message accordingly
    
    v4:
    - new patch in v4

 common/utils/vector.h | 7 +++++--
 lib/uri.c             | 2 +-
 copy/file-ops.c       | 2 +-
 copy/nbd-ops.c        | 2 +-
 dump/dump.c           | 2 +-
 fuse/nbdfuse.h        | 2 +-
 info/list.c           | 2 +-
 info/map.c            | 2 +-
 ublk/nbdublk.h        | 2 +-
 ublk/tgt.c            | 2 +-
 10 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/common/utils/vector.h b/common/utils/vector.h
index fb2482c853ff..b9b88ba02e7d 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -52,7 +52,7 @@
 /* Use of this macro defines a new type called ?name? containing an
  * extensible vector of ?type? elements.  For example:
  *
- *   DEFINE_VECTOR_TYPE (string_vector, char *)
+ *   DEFINE_VECTOR_TYPE (string_vector, char *);
  *
  * defines a new type called ?string_vector? as a vector of ?char *?.
  * You can create variables of this type:
@@ -176,7 +176,10 @@
   {                                                                     \
     return bsearch (key, v->ptr, v->len, sizeof (type),                 \
                     (void *) compare);                                  \
-  }
+  }                                                                     \
+                                                                        \
+  /* End with duplicate declaration, so callers must supply ';'. */    
\
+  struct name
 
 #define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
 
diff --git a/lib/uri.c b/lib/uri.c
index 367621d40208..31ee90f3b94f 100644
--- a/lib/uri.c
+++ b/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 a/copy/file-ops.c b/copy/file-ops.c
index 18cae74a617d..1efece2614e6 100644
--- a/copy/file-ops.c
+++ b/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 a/copy/nbd-ops.c b/copy/nbd-ops.c
index 34ab4857ee00..0980a5edec46 100644
--- a/copy/nbd-ops.c
+++ b/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 a/dump/dump.c b/dump/dump.c
index d0da28790eb7..922bd4355897 100644
--- a/dump/dump.c
+++ b/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 a/fuse/nbdfuse.h b/fuse/nbdfuse.h
index 371ca8bb53ac..93b66aac52c1 100644
--- a/fuse/nbdfuse.h
+++ b/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 a/info/list.c b/info/list.c
index c2741ba0fa10..4b4e8f860160 100644
--- a/info/list.c
+++ b/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 a/info/map.c b/info/map.c
index a5aad9552208..4924866a3b28 100644
--- a/info/map.c
+++ b/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);
diff --git a/ublk/nbdublk.h b/ublk/nbdublk.h
index 086352e9d17f..1296b8424a5b 100644
--- a/ublk/nbdublk.h
+++ b/ublk/nbdublk.h
@@ -25,7 +25,7 @@
 
 #include "vector.h"
 
-DEFINE_VECTOR_TYPE (handles, struct nbd_handle *)
+DEFINE_VECTOR_TYPE (handles, struct nbd_handle *);
 
 #define UBLKSRV_TGT_TYPE_NBD 0
 
diff --git a/ublk/tgt.c b/ublk/tgt.c
index 5d88e33dcc4b..9b0a64d66b80 100644
--- a/ublk/tgt.c
+++ b/ublk/tgt.c
@@ -62,7 +62,7 @@ struct thread_info {
   struct ublksrv_aio_ctx *aio_ctx;
   struct ublksrv_aio_list compl;
 };
-DEFINE_VECTOR_TYPE (thread_infos, struct thread_info)
+DEFINE_VECTOR_TYPE (thread_infos, struct thread_info);
 static thread_infos thread_info;
 
 static pthread_barrier_t barrier;

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 at 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 at redhat.com>
Reviewed-by: Eric Blake <eblake at 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);

On 2/26/23 12:45, Laszlo Ersek wrote:
> V4:
> http://mid.mail-archive.com/20230224043937.392235-1-lersek at redhat.com
> 
> Please see the Notes section in each patch for the updates.
> 
> Laszlo
Merged as commit range 254ac22dde42..c7ff70101e8c.

Thanks
Laszlo
> 
> Laszlo Ersek (5):
>   common/include: add TYPE_IS_POINTER() macro
>   common/include: extract STATIC_ASSERT() macro
>   force 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/string-vector.h                 |  2 +-
>  common/utils/test-vector.c                   |  3 +-
>  common/utils/vector.h                        | 42 ++++++++++++++++-
>  copy/file-ops.c                              |  2 +-
>  copy/nbd-ops.c                               |  2 +-
>  dump/dump.c                                  |  2 +-
>  fuse/nbdfuse.h                               |  2 +-
>  generator/states-connect-socket-activation.c |  9 ++--
>  info/list.c                                  |  2 +-
>  info/map.c                                   |  2 +-
>  info/show.c                                  |  3 +-
>  lib/handle.c                                 | 12 ++---
>  lib/uri.c                                    |  2 +-
>  lib/utils.c                                  |  6 +--
>  ublk/nbdublk.h                               |  2 +-
>  ublk/tgt.c                                   |  2 +-
>  21 files changed, 157 insertions(+), 71 deletions(-)
>  create mode 100644 common/include/static-assert.h
> 
> 
> base-commit: 594f7a738e0aba23eda37965c96b3df6b8f76960
> _______________________________________________
> Libguestfs mailing list
> Libguestfs at redhat.com
> https://listman.redhat.com/mailman/listinfo/libguestfs
>

Thanks for this series.  I checked it upstream & it looks great.

Rich.

-- 
Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones
Read my programming and virtualization blog: http://rwmj.wordpress.com
libguestfs lets you edit virtual machines.  Supports shell scripting,
bindings from many languages.  http://libguestfs.org