We intend to place a space character between the function designator and the opening parenthesis in a function call. We've been executing on that mostly consistently; fix the few exceptions now. The same convention should be applied to the invocations of function-like macros, and to instances of "__attribute__ ((attr))". (The latter is exemplified, although not consistently, by the GCC manual.) Implement this, by inserting the necessary spaces. Furthermore, some compiler attributes take multiple parameters, such as "nonnull". The GCC manual clearly shows that arguments passed to such attributes may be separated with ", " as well, not just ",". Use the former, as it's more readable. Finally, the C standard calls "defined" -- as in "#if defined identifier" and (equivalently) "#if defined (identifier)" -- a unary preprocessing operator. We can spell the parenthesized form as "defined (identifier)" rather than "defined(identifier)", so choose the former. I collected the locations possibly missing spaces with: git grep -EHn '\<[a-zA-Z0-9_]+\(' -- '*.c' '*.h' and then manually updated each as necessary. I didn't change occurrences in comments, except where the comment clearly indicated copying and pasting an expression into new code. "git show -w" outputs nothing for this patch. The test suite passes. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- lib/internal.h | 72 ++++++++++---------- lib/nbd-protocol.h | 12 ++-- common/include/array-size.h | 2 +- common/include/byte-swapping.h | 24 +++---- common/include/checked-overflow.h | 42 ++++++------ common/include/compiler-macros.h | 2 +- common/include/iszero.h | 2 +- common/include/minmax.h | 4 +- common/utils/const-string-vector.h | 2 +- common/utils/nbdkit-string.h | 2 +- common/utils/string-vector.h | 2 +- common/utils/vector.h | 20 +++--- lib/errors.c | 6 +- lib/opt.c | 4 +- lib/uri.c | 2 +- lib/utils.c | 12 ++-- common/include/test-array-size.c | 26 +++---- common/utils/test-human-size.c | 10 +-- common/utils/test-vector.c | 4 +- ocaml/nbd-c.h | 6 +- tests/eflags.c | 6 +- tests/get-size.c | 4 +- tests/newstyle-limited.c | 6 +- tests/oldstyle.c | 4 +- examples/copy-libev.c | 30 ++++---- examples/list-exports.c | 2 +- examples/strict-structured-reads.c | 2 +- examples/threaded-reads-and-writes.c | 2 +- interop/interop.c | 2 +- copy/file-ops.c | 2 +- copy/main.c | 2 +- copy/nbdcopy.h | 2 +- dump/dump.c | 2 +- fuse/nbdfuse.c | 2 +- info/main.c | 2 +- ublk/nbdublk.c | 4 +- ublk/tgt.c | 20 +++--- 37 files changed, 175 insertions(+), 175 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index bbbd26393f5a..c5827f4f0037 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -46,7 +46,7 @@ * debug and error handling code out of hot paths, making the hot path * into common functions use less instruction cache. */ -#if defined(__GNUC__) +#if defined (__GNUC__) #define unlikely(x) __builtin_expect (!!(x), 0) #define if_debug(h) if (unlikely ((h)->debug)) #else @@ -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; @@ -220,20 +220,20 @@ struct nbd_handle { struct { struct nbd_fixed_new_option_reply_server server; char str[NBD_MAX_STRING * 2 + 1]; /* name, description, NUL */ - } __attribute__((packed)) server; + } __attribute__ ((packed)) server; struct nbd_fixed_new_option_reply_info_export export; struct nbd_fixed_new_option_reply_info_block_size block_size; struct { struct nbd_fixed_new_option_reply_info_name_or_desc info; char str[NBD_MAX_STRING]; - } __attribute__((packed)) name_desc; + } __attribute__ ((packed)) name_desc; struct { struct nbd_fixed_new_option_reply_meta_context context; char str[NBD_MAX_STRING]; - } __attribute__((packed)) context; + } __attribute__ ((packed)) context; char err_msg[NBD_MAX_STRING]; } payload; - } __attribute__((packed)) or; + } __attribute__ ((packed)) or; struct nbd_export_name_option_reply export_name_reply; struct nbd_simple_reply simple_reply; struct { @@ -245,9 +245,9 @@ struct nbd_handle { struct nbd_structured_reply_error error; char msg[NBD_MAX_STRING]; /* Common to all error types */ uint64_t offset; /* Only used for NBD_REPLY_TYPE_ERROR_OFFSET */ - } __attribute__((packed)) error; + } __attribute__ ((packed)) error; } payload; - } __attribute__((packed)) sr; + } __attribute__ ((packed)) sr; uint16_t gflags; uint32_t cflags; uint32_t len; @@ -387,26 +387,26 @@ struct command { /* aio.c */ extern void nbd_internal_retire_and_free_command (struct command *) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); /* connect.c */ extern int nbd_internal_wait_until_connected (struct nbd_handle *h) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); /* crypto.c */ extern struct socket *nbd_internal_crypto_create_session (struct nbd_handle *, struct socket *oldsock) - LIBNBD_ATTRIBUTE_NONNULL(1, 2); + LIBNBD_ATTRIBUTE_NONNULL (1, 2); extern bool nbd_internal_crypto_is_reading (struct nbd_handle *) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern int nbd_internal_crypto_handshake (struct nbd_handle *) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern void nbd_internal_crypto_debug_tls_enabled (struct nbd_handle *) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); /* debug.c */ extern void nbd_internal_debug (struct nbd_handle *h, const char *context, const char *fs, ...) - LIBNBD_ATTRIBUTE_NONNULL(1, 3); + LIBNBD_ATTRIBUTE_NONNULL (1, 3); #define debug(h, fs, ...) \ do { \ if_debug ((h)) \ @@ -420,10 +420,10 @@ extern void nbd_internal_debug (struct nbd_handle *h, const char *context, /* errors.c */ extern void nbd_internal_set_error_context (const char *context) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern const char *nbd_internal_get_error_context (void); extern void nbd_internal_set_last_error (int errnum, char *error) - LIBNBD_ATTRIBUTE_NONNULL(2); + LIBNBD_ATTRIBUTE_NONNULL (2); #define set_error(errnum, fs, ...) \ do { \ int _e = (errnum); \ @@ -442,16 +442,16 @@ extern void nbd_internal_set_last_error (int errnum, char *error) /* flags.c */ extern void nbd_internal_reset_size_and_flags (struct nbd_handle *h) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern int nbd_internal_set_size_and_flags (struct nbd_handle *h, uint64_t exportsize, uint16_t eflags) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern int nbd_internal_set_block_size (struct nbd_handle *h, uint32_t min, uint32_t pref, uint32_t max) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern void nbd_internal_set_payload (struct nbd_handle *h) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); /* is-state.c */ extern bool nbd_internal_is_state_created (enum state state); @@ -464,7 +464,7 @@ extern bool nbd_internal_is_state_closed (enum state state); /* opt.c */ extern void nbd_internal_free_option (struct nbd_handle *h) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); /* protocol.c */ extern int nbd_internal_errno_of_nbd_error (uint32_t error); @@ -476,7 +476,7 @@ extern int64_t nbd_internal_command_common (struct nbd_handle *h, uint64_t offset, uint64_t count, int count_err, void *data, struct command_cb *cb) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); /* socket.c */ struct socket *nbd_internal_socket_create (int fd); @@ -484,9 +484,9 @@ struct socket *nbd_internal_socket_create (int fd); /* states.c */ extern void nbd_internal_abort_commands (struct nbd_handle *h, struct command **list) - LIBNBD_ATTRIBUTE_NONNULL(1,2); + LIBNBD_ATTRIBUTE_NONNULL (1, 2); extern int nbd_internal_run (struct nbd_handle *h, enum external_event ev) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern const char *nbd_internal_state_short_string (enum state state); extern enum state_group nbd_internal_state_group (enum state state); extern enum state_group nbd_internal_state_group_parent (enum state_group group); @@ -498,24 +498,24 @@ extern int nbd_internal_aio_get_direction (enum state state); /* utils.c */ extern void nbd_internal_hexdump (const void *data, size_t len, FILE *fp) - LIBNBD_ATTRIBUTE_NONNULL(1,3); + LIBNBD_ATTRIBUTE_NONNULL (1, 3); extern int nbd_internal_copy_string_list (string_vector *v, char **in) - LIBNBD_ATTRIBUTE_NONNULL(1,2); + LIBNBD_ATTRIBUTE_NONNULL (1, 2); extern int nbd_internal_set_argv (struct nbd_handle *h, char **argv) - LIBNBD_ATTRIBUTE_NONNULL(1,2); + LIBNBD_ATTRIBUTE_NONNULL (1, 2); extern int nbd_internal_set_querylist (struct nbd_handle *h, char **queries) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern const char *nbd_internal_fork_safe_itoa (long v, char *buf, size_t len) - LIBNBD_ATTRIBUTE_NONNULL(2); + LIBNBD_ATTRIBUTE_NONNULL (2); extern void nbd_internal_fork_safe_perror (const char *s) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern char *nbd_internal_printable_buffer (const void *buf, size_t count) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_NONNULL (1); extern char *nbd_internal_printable_string (const char *str) - LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free) - LIBNBD_ATTRIBUTE_NONNULL(1); + LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (free) + LIBNBD_ATTRIBUTE_NONNULL (1); extern char *nbd_internal_printable_string_list (char **list) - LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(free); + LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (free); /* These are wrappers around socket(2) and socketpair(2). They * always set SOCK_CLOEXEC. nbd_internal_socket can set SOCK_NONBLOCK @@ -525,6 +525,6 @@ extern int nbd_internal_socket (int domain, int type, int protocol, bool nonblock); extern int nbd_internal_socketpair (int domain, int type, int protocol, int *fds) - LIBNBD_ATTRIBUTE_NONNULL(4); + LIBNBD_ATTRIBUTE_NONNULL (4); #endif /* LIBNBD_INTERNAL_H */ diff --git a/lib/nbd-protocol.h b/lib/nbd-protocol.h index e5d6404b8eaa..9353668605ff 100644 --- a/lib/nbd-protocol.h +++ b/lib/nbd-protocol.h @@ -40,8 +40,8 @@ * these structures. */ -#if defined(__GNUC__) || defined(__clang__) -#define NBD_ATTRIBUTE_PACKED __attribute__((__packed__)) +#if defined (__GNUC__) || defined (__clang__) +#define NBD_ATTRIBUTE_PACKED __attribute__ ((__packed__)) #else #error "Please port to your compiler's notion of a packed struct" #endif @@ -58,8 +58,8 @@ struct nbd_old_handshake { char zeroes[124]; /* must be sent as zero bytes */ } NBD_ATTRIBUTE_PACKED; -#define NBD_MAGIC UINT64_C(0x4e42444d41474943) /* ASCII "NBDMAGIC" */ -#define NBD_OLD_VERSION UINT64_C(0x0000420281861253) +#define NBD_MAGIC UINT64_C (0x4e42444d41474943) /* ASCII "NBDMAGIC" */ +#define NBD_OLD_VERSION UINT64_C (0x0000420281861253) /* New-style handshake. */ struct nbd_new_handshake { @@ -68,7 +68,7 @@ struct nbd_new_handshake { uint16_t gflags; /* global flags */ } NBD_ATTRIBUTE_PACKED; -#define NBD_NEW_VERSION UINT64_C(0x49484156454F5054) /* ASCII "IHAVEOPT" */ +#define NBD_NEW_VERSION UINT64_C (0x49484156454F5054) /* ASCII "IHAVEOPT" */ /* New-style handshake option (sent by the client to us). */ struct nbd_new_option { @@ -95,7 +95,7 @@ struct nbd_fixed_new_option_reply { uint32_t replylen; } NBD_ATTRIBUTE_PACKED; -#define NBD_REP_MAGIC UINT64_C(0x3e889045565a9) +#define NBD_REP_MAGIC UINT64_C (0x3e889045565a9) /* Global flags. */ #define NBD_FLAG_FIXED_NEWSTYLE (1 << 0) diff --git a/common/include/array-size.h b/common/include/array-size.h index 3212d9dc6cdb..6f0a1ae87233 100644 --- a/common/include/array-size.h +++ b/common/include/array-size.h @@ -36,6 +36,6 @@ #include "compiler-macros.h" #define ARRAY_SIZE(a) \ - ((sizeof (a) / sizeof ((a)[0])) + BUILD_BUG_UNLESS_TRUE (TYPE_IS_ARRAY(a))) + ((sizeof (a) / sizeof ((a)[0])) + BUILD_BUG_UNLESS_TRUE (TYPE_IS_ARRAY (a))) #endif /* NBDKIT_ARRAY_SIZE_H */ diff --git a/common/include/byte-swapping.h b/common/include/byte-swapping.h index c53fba2e8f24..fce56df585ca 100644 --- a/common/include/byte-swapping.h +++ b/common/include/byte-swapping.h @@ -58,20 +58,20 @@ #ifdef __HAIKU__ #include <ByteOrder.h> -#define htobe16(x) B_HOST_TO_BENDIAN_INT16(x) -#define htole16(x) B_HOST_TO_LENDIAN_INT16(x) -#define be16toh(x) B_BENDIAN_TO_HOST_INT16(x) -#define le16toh(x) B_LENDIAN_TO_HOST_INT16(x) +#define htobe16(x) B_HOST_TO_BENDIAN_INT16 (x) +#define htole16(x) B_HOST_TO_LENDIAN_INT16 (x) +#define be16toh(x) B_BENDIAN_TO_HOST_INT16 (x) +#define le16toh(x) B_LENDIAN_TO_HOST_INT16 (x) -#define htobe32(x) B_HOST_TO_BENDIAN_INT32(x) -#define htole32(x) B_HOST_TO_LENDIAN_INT32(x) -#define be32toh(x) B_BENDIAN_TO_HOST_INT32(x) -#define le32toh(x) B_LENDIAN_TO_HOST_INT32(x) +#define htobe32(x) B_HOST_TO_BENDIAN_INT32 (x) +#define htole32(x) B_HOST_TO_LENDIAN_INT32 (x) +#define be32toh(x) B_BENDIAN_TO_HOST_INT32 (x) +#define le32toh(x) B_LENDIAN_TO_HOST_INT32 (x) -#define htobe64(x) B_HOST_TO_BENDIAN_INT64(x) -#define htole64(x) B_HOST_TO_LENDIAN_INT64(x) -#define be64toh(x) B_BENDIAN_TO_HOST_INT64(x) -#define le64toh(x) B_LENDIAN_TO_HOST_INT64(x) +#define htobe64(x) B_HOST_TO_BENDIAN_INT64 (x) +#define htole64(x) B_HOST_TO_LENDIAN_INT64 (x) +#define be64toh(x) B_BENDIAN_TO_HOST_INT64 (x) +#define le64toh(x) B_LENDIAN_TO_HOST_INT64 (x) #endif /* If we didn't define bswap_16, bswap_32 and bswap_64 already above, diff --git a/common/include/checked-overflow.h b/common/include/checked-overflow.h index 546c930b33d2..a1852adcdc7a 100644 --- a/common/include/checked-overflow.h +++ b/common/include/checked-overflow.h @@ -46,7 +46,7 @@ #ifndef NBDKIT_CHECKED_OVERFLOW_H #define NBDKIT_CHECKED_OVERFLOW_H -#if !defined(__GNUC__) && !defined(__clang__) +#if !defined (__GNUC__) && !defined (__clang__) #error "this file may need to be ported to your compiler" #endif @@ -66,9 +66,9 @@ * mathematical sum are stored to "*r". */ #if HAVE_DECL___BUILTIN_ADD_OVERFLOW -#define ADD_OVERFLOW(a, b, r) ADD_OVERFLOW_BUILTIN((a), (b), (r)) +#define ADD_OVERFLOW(a, b, r) ADD_OVERFLOW_BUILTIN ((a), (b), (r)) #else -#define ADD_OVERFLOW(a, b, r) ADD_OVERFLOW_FALLBACK((a), (b), (r)) +#define ADD_OVERFLOW(a, b, r) ADD_OVERFLOW_FALLBACK ((a), (b), (r)) #endif /* Multiply "a" and "b", both of (possibly different) unsigned integer types, @@ -83,9 +83,9 @@ * the mathematical product are stored to "*r". */ #if HAVE_DECL___BUILTIN_MUL_OVERFLOW -#define MUL_OVERFLOW(a, b, r) MUL_OVERFLOW_BUILTIN((a), (b), (r)) +#define MUL_OVERFLOW(a, b, r) MUL_OVERFLOW_BUILTIN ((a), (b), (r)) #else -#define MUL_OVERFLOW(a, b, r) MUL_OVERFLOW_FALLBACK((a), (b), (r)) +#define MUL_OVERFLOW(a, b, r) MUL_OVERFLOW_FALLBACK ((a), (b), (r)) #endif /* The ADD_OVERFLOW_BUILTIN and MUL_OVERFLOW_BUILTIN function-like macros @@ -99,22 +99,22 @@ * variably modified type. */ #if HAVE_DECL___BUILTIN_ADD_OVERFLOW -#define ADD_OVERFLOW_BUILTIN(a, b, r) \ - ({ \ - STATIC_ASSERT_UNSIGNED_INT (a); \ - STATIC_ASSERT_UNSIGNED_INT (b); \ - STATIC_ASSERT_UNSIGNED_INT (*(r)); \ - __builtin_add_overflow((a), (b), (r)); \ +#define ADD_OVERFLOW_BUILTIN(a, b, r) \ + ({ \ + STATIC_ASSERT_UNSIGNED_INT (a); \ + STATIC_ASSERT_UNSIGNED_INT (b); \ + STATIC_ASSERT_UNSIGNED_INT (*(r)); \ + __builtin_add_overflow ((a), (b), (r)); \ }) #endif #if HAVE_DECL___BUILTIN_MUL_OVERFLOW -#define MUL_OVERFLOW_BUILTIN(a, b, r) \ - ({ \ - STATIC_ASSERT_UNSIGNED_INT (a); \ - STATIC_ASSERT_UNSIGNED_INT (b); \ - STATIC_ASSERT_UNSIGNED_INT (*(r)); \ - __builtin_mul_overflow((a), (b), (r)); \ +#define MUL_OVERFLOW_BUILTIN(a, b, r) \ + ({ \ + STATIC_ASSERT_UNSIGNED_INT (a); \ + STATIC_ASSERT_UNSIGNED_INT (b); \ + STATIC_ASSERT_UNSIGNED_INT (*(r)); \ + __builtin_mul_overflow ((a), (b), (r)); \ }) #endif @@ -173,10 +173,10 @@ * * 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__)); \ +#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) /* Assign the sum "a + b" to "*r", using uintmax_t modular arithmetic. diff --git a/common/include/compiler-macros.h b/common/include/compiler-macros.h index d41166614f95..7933bb87a5bf 100644 --- a/common/include/compiler-macros.h +++ b/common/include/compiler-macros.h @@ -48,7 +48,7 @@ #define BUILD_BUG_STRUCT_SIZE(cond) \ (sizeof (struct { int: (cond) ? 1 : -1; })) #define BUILD_BUG_UNLESS_TRUE(cond) \ - (BUILD_BUG_STRUCT_SIZE(cond) - BUILD_BUG_STRUCT_SIZE(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]))) diff --git a/common/include/iszero.h b/common/include/iszero.h index 7fdbb52c2a03..657ed5bac4ef 100644 --- a/common/include/iszero.h +++ b/common/include/iszero.h @@ -45,7 +45,7 @@ * See also: * https://gcc.gnu.org/bugzilla/show_bug.cgi?id=69908 */ -static inline bool __attribute__((__nonnull__ (1))) +static inline bool __attribute__ ((__nonnull__ (1))) is_zero (const char *buffer, size_t size) { size_t i; diff --git a/common/include/minmax.h b/common/include/minmax.h index 8d1fbf19bc23..8b0e6b7a4dbb 100644 --- a/common/include/minmax.h +++ b/common/include/minmax.h @@ -48,10 +48,10 @@ #undef MIN #define MIN(x, y) \ - MIN_1((x), (y), NBDKIT_UNIQUE_NAME(_x), NBDKIT_UNIQUE_NAME(_y)) + MIN_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y)) #undef MAX #define MAX(x, y) \ - MAX_1((x), (y), NBDKIT_UNIQUE_NAME(_x), NBDKIT_UNIQUE_NAME(_y)) + MAX_1 ((x), (y), NBDKIT_UNIQUE_NAME (_x), NBDKIT_UNIQUE_NAME (_y)) #ifdef HAVE_AUTO_TYPE diff --git a/common/utils/const-string-vector.h b/common/utils/const-string-vector.h index 91307ca1cf91..c15f8952b3b3 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 14998703b80a..b85f05991a82 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 80d7311debfb..aa33fd48ceb5 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/vector.h b/common/utils/vector.h index 60577eaefd74..fb2482c853ff 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: @@ -94,7 +94,7 @@ * allocated and capacity is increased, but the vector length is \ * not increased and the new elements are not initialized. \ */ \ - static inline int __attribute__((__unused__)) \ + static inline int __attribute__ ((__unused__)) \ name##_reserve (name *v, size_t n) \ { \ return generic_vector_reserve ((struct generic_vector *)v, n, \ @@ -104,7 +104,7 @@ /* Same as _reserve, but the allocation will be page aligned. Note \ * that the machine page size must be divisible by sizeof (type). \ */ \ - static inline int __attribute__((__unused__)) \ + static inline int __attribute__ ((__unused__)) \ name##_reserve_page_aligned (name *v, size_t n) \ { \ return generic_vector_reserve_page_aligned ((struct generic_vector *)v, \ @@ -112,7 +112,7 @@ } \ \ /* Insert at i'th element. i=0 => beginning i=len => append */ \ - static inline int __attribute__((__unused__)) \ + static inline int __attribute__ ((__unused__)) \ name##_insert (name *v, type elem, size_t i) \ { \ assert (i <= v->len); \ @@ -126,14 +126,14 @@ } \ \ /* Append a new element to the end of the vector. */ \ - static inline int __attribute__((__unused__)) \ + static inline int __attribute__ ((__unused__)) \ name##_append (name *v, type elem) \ { \ return name##_insert (v, elem, v->len); \ } \ \ /* Remove i'th element. i=0 => beginning i=len-1 => end */ \ - static inline void __attribute__((__unused__)) \ + static inline void __attribute__ ((__unused__)) \ name##_remove (name *v, size_t i) \ { \ assert (i < v->len); \ @@ -142,7 +142,7 @@ } \ \ /* Remove all elements and deallocate the vector. */ \ - static inline void __attribute__((__unused__)) \ + static inline void __attribute__ ((__unused__)) \ name##_reset (name *v) \ { \ free (v->ptr); \ @@ -151,7 +151,7 @@ } \ \ /* Iterate over the vector, calling f() on each element. */ \ - static inline void __attribute__((__unused__)) \ + static inline void __attribute__ ((__unused__)) \ name##_iter (name *v, void (*f) (type elem)) \ { \ size_t i; \ @@ -160,7 +160,7 @@ } \ \ /* Sort the elements of the vector. */ \ - static inline void __attribute__((__unused__)) \ + static inline void __attribute__ ((__unused__)) \ name##_sort (name *v, \ int (*compare) (const type *p1, const type *p2)) \ { \ @@ -170,7 +170,7 @@ /* Search for an exactly matching element in the vector using a \ * binary search. Returns a pointer to the element or NULL. \ */ \ - static inline type * __attribute__((__unused__)) \ + static inline type * __attribute__ ((__unused__)) \ name##_search (const name *v, const void *key, \ int (*compare) (const void *key, const type *v)) \ { \ diff --git a/lib/errors.c b/lib/errors.c index 8859ae739528..c601a204d2b9 100644 --- a/lib/errors.c +++ b/lib/errors.c @@ -35,13 +35,13 @@ struct last_error { /* Thread-local storage of the last error. */ static pthread_key_t errors_key; -static void free_errors_key (void *vp) LIBNBD_ATTRIBUTE_NONNULL(1); +static void free_errors_key (void *vp) LIBNBD_ATTRIBUTE_NONNULL (1); /* Constructor/destructor to create the thread-local key when the * library is loaded and unloaded. */ -static void errors_init (void) __attribute__((constructor)); -static void errors_free (void) __attribute__((destructor)); +static void errors_init (void) __attribute__ ((constructor)); +static void errors_free (void) __attribute__ ((destructor)); static void errors_init (void) diff --git a/lib/opt.c b/lib/opt.c index 776a3612056a..9346d6b5f792 100644 --- a/lib/opt.c +++ b/lib/opt.c @@ -30,13 +30,13 @@ static int opt_meta_context_queries (struct nbd_handle *h, uint32_t opt, char **queries, nbd_context_callback *context) - LIBNBD_ATTRIBUTE_NONNULL(1,4); + LIBNBD_ATTRIBUTE_NONNULL (1, 4); static int aio_opt_meta_context_queries (struct nbd_handle *h, uint32_t opt, char **queries, nbd_context_callback *context, nbd_completion_callback *complete) - LIBNBD_ATTRIBUTE_NONNULL(1,4,5); + LIBNBD_ATTRIBUTE_NONNULL (1, 4, 5); /* Internal function which frees an option with callback. */ void diff --git a/lib/uri.c b/lib/uri.c index ae5b9adbc3cc..367621d40208 100644 --- a/lib/uri.c +++ b/lib/uri.c @@ -403,7 +403,7 @@ nbd_unlocked_aio_connect_uri (struct nbd_handle *h, const char *raw_uri) static int append_query_params (char **query_params, const char *key, const char *value) - LIBNBD_ATTRIBUTE_NONNULL(1,2,3); + LIBNBD_ATTRIBUTE_NONNULL (1, 2, 3); char * nbd_unlocked_get_uri (struct nbd_handle *h) diff --git a/lib/utils.c b/lib/utils.c index 9c5c38060a29..c229ebfc6106 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -178,7 +178,7 @@ nbd_internal_fork_safe_itoa (long v, char *buf, size_t bufsize) return &buf[i]; } -#if defined(__GNUC__) +#if defined (__GNUC__) #pragma GCC diagnostic push #pragma GCC diagnostic ignored "-Wunused-result" #endif @@ -213,7 +213,7 @@ nbd_internal_fork_safe_perror (const char *s) errno = err; } -#if defined(__GNUC__) +#if defined (__GNUC__) #pragma GCC diagnostic pop #endif @@ -334,10 +334,10 @@ nbd_internal_printable_string_list (char **list) } -int nbd_internal_socket(int domain, - int type, - int protocol, - bool nonblock) +int nbd_internal_socket (int domain, + int type, + int protocol, + bool nonblock) { int fd; diff --git a/common/include/test-array-size.c b/common/include/test-array-size.c index 1ce2142cd39c..8b0972aaabe1 100644 --- a/common/include/test-array-size.c +++ b/common/include/test-array-size.c @@ -41,21 +41,21 @@ struct st { const char *s; int i; }; -static const char *s0[] __attribute__((__unused__)) = { }; -static const char *s1[] __attribute__((__unused__)) = { "a" }; -static const char *s3[] __attribute__((__unused__)) = { "a", "b", "c" }; -static const char *s4[4] __attribute__((__unused__)) = { "a", "b", "c", "d" }; -static int i0[] __attribute__((__unused__)) = { }; -static int i1[] __attribute__((__unused__)) = { 1 }; -static int i3[] __attribute__((__unused__)) = { 1, 2, 3 }; -static int i4[4] __attribute__((__unused__)) = { 1, 2, 3, 4 }; -static struct st st0[] __attribute__((__unused__)) = { }; -static struct st st1[] __attribute__((__unused__)) = { { "a", 1 } }; -static struct st st3[] __attribute__((__unused__)) = +static const char *s0[] __attribute__ ((__unused__)) = { }; +static const char *s1[] __attribute__ ((__unused__)) = { "a" }; +static const char *s3[] __attribute__ ((__unused__)) = { "a", "b", "c" }; +static const char *s4[4] __attribute__ ((__unused__)) = { "a", "b", "c", "d" }; +static int i0[] __attribute__ ((__unused__)) = { }; +static int i1[] __attribute__ ((__unused__)) = { 1 }; +static int i3[] __attribute__ ((__unused__)) = { 1, 2, 3 }; +static int i4[4] __attribute__ ((__unused__)) = { 1, 2, 3, 4 }; +static struct st st0[] __attribute__ ((__unused__)) = { }; +static struct st st1[] __attribute__ ((__unused__)) = { { "a", 1 } }; +static struct st st3[] __attribute__ ((__unused__)) = { { "a", 1 }, { "b", 2 }, { "c", 3 } }; -static struct st st4[4] __attribute__((__unused__)) = +static struct st st4[4] __attribute__ ((__unused__)) = { { "a", 1 }, { "b", 2 }, { "c", 3 }, { "d", 4 } }; -static struct st st4_0[4] __attribute__((__unused__)); +static struct st st4_0[4] __attribute__ ((__unused__)); int main (void) diff --git a/common/utils/test-human-size.c b/common/utils/test-human-size.c index 193f535f5ac6..2747ee67ffe6 100644 --- a/common/utils/test-human-size.c +++ b/common/utils/test-human-size.c @@ -73,13 +73,13 @@ main (int argc, char *argv[]) test (1048576, "1M", true); test (1048577, "1048577", false); - test (UINT64_C(1073741824), "1G", true); + test (UINT64_C (1073741824), "1G", true); - test (UINT64_C(1099511627776), "1T", true); - test (UINT64_C(1099511627777), "1099511627777", false); - test (UINT64_C(1099511627776) + 1024, "1073741825K", true); + test (UINT64_C (1099511627776), "1T", true); + test (UINT64_C (1099511627777), "1099511627777", false); + test (UINT64_C (1099511627776) + 1024, "1073741825K", true); - test (UINT64_C(1125899906842624), "1P", true); + test (UINT64_C (1125899906842624), "1P", true); test ((uint64_t)INT64_MAX+1, "8E", true); test (UINT64_MAX-1023, "18014398509481983K", true); diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c index ecd5c1610e3c..55c8ebeb8a1e 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/ocaml/nbd-c.h b/ocaml/nbd-c.h index 8b0c088da7ec..24505a0e0478 100644 --- a/ocaml/nbd-c.h +++ b/ocaml/nbd-c.h @@ -32,7 +32,7 @@ /* Workaround for OCaml < 4.06.0 */ #ifndef Bytes_val -#define Bytes_val(x) String_val(x) +#define Bytes_val(x) String_val (x) #endif /* Wrapper around caml_alloc_custom_mem for pre-2019 versions of OCaml. */ @@ -68,7 +68,7 @@ extern void nbd_internal_unix_sockaddr_to_sa (value, struct sockaddr_storage *, extern void nbd_internal_ocaml_exception_in_wrapper (const char *, value); /* Extract an NBD handle from an OCaml heap value. */ -#define NBD_val(v) (*((struct nbd_handle **)Data_custom_val(v))) +#define NBD_val(v) (*((struct nbd_handle **)Data_custom_val (v))) static struct custom_operations libnbd_custom_operations = { (char *) "libnbd_custom_operations", @@ -108,7 +108,7 @@ struct nbd_buffer { * whole struct is stored in the custom, not a pointer. This macro * returns a pointer to the struct. */ -#define NBD_buffer_val(v) ((struct nbd_buffer *)Data_custom_val(v)) +#define NBD_buffer_val(v) ((struct nbd_buffer *)Data_custom_val (v)) static struct custom_operations nbd_buffer_custom_operations = { (char *) "nbd_buffer_custom_operations", diff --git a/tests/eflags.c b/tests/eflags.c index 78fb6d3ec15a..023b7845c483 100644 --- a/tests/eflags.c +++ b/tests/eflags.c @@ -37,10 +37,10 @@ /* https://stackoverflow.com/a/1489985 */ #define XNBD_FLAG_FUNCTION(f) nbd_ ## f -#define NBD_FLAG_FUNCTION(f) XNBD_FLAG_FUNCTION(f) +#define NBD_FLAG_FUNCTION(f) XNBD_FLAG_FUNCTION (f) #define XSTR(x) #x -#define STR(x) XSTR(x) +#define STR(x) XSTR (x) int main (int argc, char *argv[]) @@ -108,7 +108,7 @@ main (int argc, char *argv[]) #error "unknown value" #endif - if ((r = NBD_FLAG_FUNCTION(flag) (nbd)) == -1) { + if ((r = NBD_FLAG_FUNCTION (flag) (nbd)) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } diff --git a/tests/get-size.c b/tests/get-size.c index e6f44f776f13..d09e0d046d4c 100644 --- a/tests/get-size.c +++ b/tests/get-size.c @@ -32,7 +32,7 @@ #define SIZE 123456789 #define XSTR(s) #s -#define STR(s) XSTR(s) +#define STR(s) XSTR (s) int main (int argc, char *argv[]) @@ -41,7 +41,7 @@ main (int argc, char *argv[]) int64_t r; /* -n forces newstyle even if someone is still using nbdkit < 1.3 */ char *args[] = { "nbdkit", "-s", "--exit-with-parent", "-n", "-v", - "null", "size=" STR(SIZE), NULL }; + "null", "size=" STR (SIZE), NULL }; const char *s; nbd = nbd_create (); diff --git a/tests/newstyle-limited.c b/tests/newstyle-limited.c index a8f16bc88707..54cba442b28a 100644 --- a/tests/newstyle-limited.c +++ b/tests/newstyle-limited.c @@ -33,7 +33,7 @@ #define SIZE 65536 #define XSTR(s) #s -#define STR(s) XSTR(s) +#define STR(s) XSTR (s) static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512]; static const char *progname; @@ -105,9 +105,9 @@ main (int argc, char *argv[]) char *args[] = { "nbdkit", "-s", "--mask-handshake", "0", "--exit-with-parent", "-v", "eval", "list_exports=printf a\\nb\\n", - "get_size=echo " STR(SIZE), + "get_size=echo " STR (SIZE), "open=if [ \"$3\" != a ]; then exit 1; fi\n" - " truncate --size=" STR(SIZE) " $tmpdir/a", + " truncate --size=" STR (SIZE) " $tmpdir/a", "pread=dd bs=1 skip=$4 count=$3 if=$tmpdir/a || exit 1", "pwrite=dd bs=1 conv=notrunc seek=$4 of=$tmpdir/a || exit 1", "can_write=exit 0", diff --git a/tests/oldstyle.c b/tests/oldstyle.c index fd8bdc5fdd7c..5667e388d3d2 100644 --- a/tests/oldstyle.c +++ b/tests/oldstyle.c @@ -31,7 +31,7 @@ #define SIZE 65536 #define XSTR(s) #s -#define STR(s) XSTR(s) +#define STR(s) XSTR (s) static char wbuf[512] = { 1, 2, 3, 4 }, rbuf[512]; static const char *progname; @@ -82,7 +82,7 @@ main (int argc, char *argv[]) struct nbd_handle *nbd; int64_t r; char *args[] = { "nbdkit", "-s", "-o", "--exit-with-parent", "-v", - "memory", "size=" STR(SIZE), NULL }; + "memory", "size=" STR (SIZE), NULL }; int calls = 0; const char *s; diff --git a/examples/copy-libev.c b/examples/copy-libev.c index 418d99f1419d..02f9cad492b9 100644 --- a/examples/copy-libev.c +++ b/examples/copy-libev.c @@ -121,12 +121,12 @@ static ev_timer progress; static inline void start_request_soon (struct request *r); static void start_request_cb (struct ev_loop *loop, ev_timer *w, int revents); -static void start_request(struct request *r); -static void start_read(struct request *r); -static void start_write(struct request *r); -static void start_zero(struct request *r); -static int read_completed(void *user_data, int *error); -static int request_completed(void *user_data, int *error); +static void start_request (struct request *r); +static void start_read (struct request *r); +static void start_write (struct request *r); +static void start_zero (struct request *r); +static int read_completed (void *user_data, int *error); +static int request_completed (void *user_data, int *error); /* Return true iff data is all zero bytes. * @@ -158,13 +158,13 @@ request_state (struct request *r) } static inline int -get_fd(struct connection *c) +get_fd (struct connection *c) { return nbd_aio_get_fd (c->nbd); } static inline int -get_events(struct connection *c) +get_events (struct connection *c) { unsigned dir = nbd_aio_get_direction (c->nbd); @@ -388,7 +388,7 @@ start_request_cb (struct ev_loop *loop, ev_timer *w, int revents) /* Start async copy or zero request. */ static void -start_request(struct request *r) +start_request (struct request *r) { /* Cancel the request if we are done. */ if (offset == size) @@ -430,7 +430,7 @@ start_request(struct request *r) } static void -start_read(struct request *r) +start_read (struct request *r) { int64_t cookie; @@ -468,7 +468,7 @@ read_completed (void *user_data, int *error) } static void -start_write(struct request *r) +start_write (struct request *r) { int64_t cookie; @@ -487,7 +487,7 @@ start_write(struct request *r) } static void -start_zero(struct request *r) +start_zero (struct request *r) { int64_t cookie; @@ -535,7 +535,7 @@ request_completed (void *user_data, int *error) * iteration, to avoid deadlock if we need to start a zero or write. */ if (offset < size) - start_request_soon(r); + start_request_soon (r); return 1; } @@ -590,7 +590,7 @@ finish_progress () static inline void update_watcher (struct connection *c) { - int events = get_events(c); + int events = get_events (c); if (events != c->watcher.events) { ev_io_stop (loop, &c->watcher); @@ -671,7 +671,7 @@ main (int argc, char *argv[]) if (r->data == NULL) FAIL ("Cannot allocate buffer: %s", strerror (errno)); - start_request(r); + start_request (r); } /* Start watching events on src and dst handles. */ diff --git a/examples/list-exports.c b/examples/list-exports.c index f6200f480a63..c7780ffd5bb7 100644 --- a/examples/list-exports.c +++ b/examples/list-exports.c @@ -50,7 +50,7 @@ list_one (void *opaque, const char *name, printf ("[%d] %s\n", l->i, name); if (*description) - printf(" (%s)\n", description); + printf (" (%s)\n", description); names = realloc (l->names, (l->i + 1) * sizeof *names); if (!names) { diff --git a/examples/strict-structured-reads.c b/examples/strict-structured-reads.c index 5a61caa8681d..1c9664260654 100644 --- a/examples/strict-structured-reads.c +++ b/examples/strict-structured-reads.c @@ -225,7 +225,7 @@ main (int argc, char *argv[]) assert (d && r); offset = rand () % (exportsize - maxsize); - if (rand() & 1) + if (rand () & 1) flags = LIBNBD_CMD_FLAG_DF; *r = (struct range) { .first = offset, .last = offset + maxsize, }; *d = (struct data) { .offset = offset, .count = maxsize, .flags = flags, diff --git a/examples/threaded-reads-and-writes.c b/examples/threaded-reads-and-writes.c index fd3e677d7d9c..e01727c8d072 100644 --- a/examples/threaded-reads-and-writes.c +++ b/examples/threaded-reads-and-writes.c @@ -248,7 +248,7 @@ start_thread (void *arg) * Simulate a mix of large and small requests. */ while (i > 0 && in_flight < MAX_IN_FLIGHT) { - size = (rand() & 1) ? BUFFER_SIZE : 512; + size = (rand () & 1) ? BUFFER_SIZE : 512; offset = rand () % (exportsize - size); cmd = rand () & 1; if (cmd == 0) diff --git a/interop/interop.c b/interop/interop.c index bd5dc2e19656..371bfc47c672 100644 --- a/interop/interop.c +++ b/interop/interop.c @@ -88,7 +88,7 @@ main (int argc, char *argv[]) * GNUTLS_NO_SIGNAL flag, either because it predates GnuTLS 3.4.2 or * because the OS lacks MSG_NOSIGNAL support. */ -#if TLS && !defined(HAVE_GNUTLS_NO_SIGNAL) +#if TLS && !defined (HAVE_GNUTLS_NO_SIGNAL) signal (SIGPIPE, SIG_IGN); #endif diff --git a/copy/file-ops.c b/copy/file-ops.c index cc225de99d9b..18cae74a617d 100644 --- a/copy/file-ops.c +++ b/copy/file-ops.c @@ -90,7 +90,7 @@ struct rw_file { #ifdef PAGE_CACHE_MAPPING static long page_size; -static void page_size_init (void) __attribute__((constructor)); +static void page_size_init (void) __attribute__ ((constructor)); static void page_size_init (void) { diff --git a/copy/main.c b/copy/main.c index eb10c84dcc5d..ae46b76ef053 100644 --- a/copy/main.c +++ b/copy/main.c @@ -66,7 +66,7 @@ static bool is_nbd_uri (const char *s); static struct rw *open_local (const char *filename, direction d); static void print_rw (struct rw *rw, const char *prefix, FILE *fp); -static void __attribute__((noreturn)) +static void __attribute__ ((noreturn)) usage (FILE *fp, int exitcode) { fprintf (fp, diff --git a/copy/nbdcopy.h b/copy/nbdcopy.h index 9438cce4179b..b6257cc13951 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. diff --git a/dump/dump.c b/dump/dump.c index bdbc90401fe2..d0da28790eb7 100644 --- a/dump/dump.c +++ b/dump/dump.c @@ -58,7 +58,7 @@ static void do_connect (void); static void do_dump (void); static void catch_signal (int); -static void __attribute__((noreturn)) +static void __attribute__ ((noreturn)) usage (FILE *fp, int exitcode) { fprintf (fp, diff --git a/fuse/nbdfuse.c b/fuse/nbdfuse.c index b9d54abd988a..b18cce83652b 100644 --- a/fuse/nbdfuse.c +++ b/fuse/nbdfuse.c @@ -66,7 +66,7 @@ enum mode { MODE_VSOCK, /* --vsock */ }; -static void __attribute__((noreturn)) +static void __attribute__ ((noreturn)) usage (FILE *fp, int exitcode) { fprintf (fp, diff --git a/info/main.c b/info/main.c index 5cd91fe1a3cf..0b5a5190ba26 100644 --- a/info/main.c +++ b/info/main.c @@ -51,7 +51,7 @@ bool totals = false; /* --totals option */ static enum { MODE_URI = 1, MODE_SQUARE_BRACKET } mode; static char **args; -static void __attribute__((noreturn)) +static void __attribute__ ((noreturn)) usage (FILE *fp, int exitcode) { fprintf (fp, diff --git a/ublk/nbdublk.c b/ublk/nbdublk.c index 00aa23ec03b5..89976fabcb17 100644 --- a/ublk/nbdublk.c +++ b/ublk/nbdublk.c @@ -66,7 +66,7 @@ enum mode { MODE_VSOCK, /* --vsock */ }; -static void __attribute__((noreturn)) +static void __attribute__ ((noreturn)) usage (FILE *fp, int exitcode) { fprintf (fp, @@ -421,7 +421,7 @@ main (int argc, char *argv[]) exit (EXIT_FAILURE); } - dinfo = ublksrv_ctrl_get_dev_info(dev); + dinfo = ublksrv_ctrl_get_dev_info (dev); /* Register signal handlers to try to stop the device. */ sa.sa_handler = signal_handler; diff --git a/ublk/tgt.c b/ublk/tgt.c index 3fb222333143..5d88e33dcc4b 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; @@ -209,7 +209,7 @@ nbd_work_thread (void *vpinfo) ublksrv_aio_complete_worker (aio_ctx, &compl); - if (nbd_poll2 (h, ublksrv_aio_get_efd(aio_ctx), -1) == -1) { + if (nbd_poll2 (h, ublksrv_aio_get_efd (aio_ctx), -1) == -1) { fprintf (stderr, "%s\n", nbd_get_error ()); exit (EXIT_FAILURE); } @@ -223,13 +223,13 @@ io_uring_thread (void *vpinfo) { struct thread_info *thread_info = vpinfo; const struct ublksrv_dev *dev = thread_info->dev; - const struct ublksrv_ctrl_dev *cdev = ublksrv_get_ctrl_dev(dev); - const struct ublksrv_ctrl_dev_info *dinfo = ublksrv_ctrl_get_dev_info(cdev); + const struct ublksrv_ctrl_dev *cdev = ublksrv_get_ctrl_dev (dev); + const struct ublksrv_ctrl_dev_info *dinfo = ublksrv_ctrl_get_dev_info (cdev); const unsigned dev_id = dinfo->dev_id; const size_t q_id = thread_info->i; const struct ublksrv_queue *q; int r; - int tid = gettid(); + int tid = gettid (); pthread_mutex_lock (&jbuf_lock); ublksrv_json_write_queue_info (cdev, jbuf, sizeof jbuf, q_id, tid); @@ -268,7 +268,7 @@ static int set_parameters (struct ublksrv_ctrl_dev *ctrl_dev, const struct ublksrv_dev *dev) { - const struct ublksrv_ctrl_dev_info *dinfo = ublksrv_ctrl_get_dev_info(ctrl_dev); + const struct ublksrv_ctrl_dev_info *dinfo = ublksrv_ctrl_get_dev_info (ctrl_dev); const unsigned attrs = (readonly ? UBLK_ATTR_READ_ONLY : 0) | (rotational ? UBLK_ATTR_ROTATIONAL : 0) | @@ -308,7 +308,7 @@ set_parameters (struct ublksrv_ctrl_dev *ctrl_dev, int start_daemon (struct ublksrv_ctrl_dev *ctrl_dev) { - const struct ublksrv_ctrl_dev_info *dinfo = ublksrv_ctrl_get_dev_info(ctrl_dev); + const struct ublksrv_ctrl_dev_info *dinfo = ublksrv_ctrl_get_dev_info (ctrl_dev); const struct ublksrv_dev *dev; size_t i; int r; @@ -421,8 +421,8 @@ start_daemon (struct ublksrv_ctrl_dev *ctrl_dev) static int init_tgt (struct ublksrv_dev *dev, int type, int argc, char *argv[]) { - const struct ublksrv_ctrl_dev *cdev = ublksrv_get_ctrl_dev(dev); - const struct ublksrv_ctrl_dev_info *info = ublksrv_ctrl_get_dev_info(cdev); + const struct ublksrv_ctrl_dev *cdev = ublksrv_get_ctrl_dev (dev); + const struct ublksrv_ctrl_dev_info *info = ublksrv_ctrl_get_dev_info (cdev); struct ublksrv_tgt_info *tgt = &dev->tgt; struct ublksrv_tgt_base_json tgt_json = { .type = type, @@ -439,7 +439,7 @@ init_tgt (struct ublksrv_dev *dev, int type, int argc, char *argv[]) tgt->tgt_ring_depth = info->queue_depth; tgt->nr_fds = 0; - ublksrv_json_write_dev_info (ublksrv_get_ctrl_dev(dev), jbuf, sizeof jbuf); + ublksrv_json_write_dev_info (ublksrv_get_ctrl_dev (dev), jbuf, sizeof jbuf); ublksrv_json_write_target_base_info (jbuf, sizeof jbuf, &tgt_json); return 0;

On 2/15/23 20:48, Eric Blake wrote: This patch concerns itself with one thing only: the space character between the function designator (or macro name) and the paren that opens the argument list. Everything else is out of scope for the patch. In the particular case, the original NBDKIT_UNIQUE_NAME line was already 84 characters long; in fact that original line, when introduced, broke the alignment of the backslashes at the right hand side. That state stems from nbdkit (not libnbd) commit cf2b6297646a ("common/include/unique-name.h: Rename UNIQUE_NAME macro", 2022-01-11). It was later ported to libnbd in commit 485f5ddf2d48 ("common/include/unique-name.h: Rename UNIQUE_NAME macro", 2022-02-23). So, this patch highlights another pre-existent task, and creates a new one: - the overlong lines from the above-noted commits should be cleaned up - the whitespace updates from the present patch should be reflected back to nbdkit. I was aware of both of those tasks, it's just that my cleanup stack simply couldn't accommodate further recursions. I was already cleaning up a thing for a cleanup that I needed for another cleanup. I couldn't nest them any deeper, I had to stop the scope creep somewhere. More generally, we should probably invent a way to avoid porting such utility code back and forth between libnbd and nbdkit. For example, libguestfs, guestfs-tools, and virt-v2v share the "common" submodule. Yes, good observation. I actually dislike the space there; the reason being that the cast operator has one of the strongest bindings in C, and the space evokes the opposite impression. We've had actual bugs introduced in edk2 because someone was misled by this. (The cast prefix op is in the same group as "*", "&", "!", "~", and unary "-"; we don't use a space with those either. ... Well, I've seen a space character after "!" in some spots, and I happen to strongly disagree with that, for the exact same reason -- but that's another discussion. :)) It's a monumental task. In edk2, just searching for the right had taken very long, and once they settled on uncrustify, several patches were required for upstream uncrustify, *plus* a humongous config file in edk2. I certainly don't want to dismiss these observations, but I'll definitely forget about them unless we record them somewhere. Do these deserve a bugzilla (or multiple bugzillas)? Thanks! Laszlo

On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek wrote: What's the reason for removing iter? It's used by nbdkit in ways that don't involve free(). I'd prefer if we leave this definition in. I'm going to be that guy ... Can we call it ADD_VECTOR_EMPTY_METHOD or similar/better? Otherwise it seems like DEFINE_VECTOR_EMPTY is an alternative to DEFINE_VECTOR_TYPE (which I thought was the case initially), rather than something you have to call in addition to DEFINE_VECTOR_TYPE. The rest seems fine. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On Wed, Feb 15, 2023 at 03:11:34PM +0100, Laszlo Ersek wrote: Tangentially related: casting function pointers in general may get harder as more compilers move towards C23 and its newer rules (see for example https://lists.gnu.org/archive/html/bug-gnulib/2023-02/msg00055.html or this gcc 13 bug https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108694 which highlights some of the warnings that newer compilers will start warning us about). While this patch focuses on avoiding casts between fn(*)(type*) and void*, I don't know off-hand if C23 will permit fn(*)(void*) as a compatible function pointer with fn(*)(type). As Rich pointed out, just because libnbd is always passing 'free' does not mean that nbdkit is doing likewise, and we want to keep this file shared between the two projects. Being able to conditionally add the cleanup function (where we don't want it on non-pointer vectors) makes sense, but removing the iterator function at the same time is a step too far. Agreed that the extra cleanup is not going to affect our hot path. Thinking higher-level now, your new macro is something where we have to do a two-step declaration of macro types where we want the new function. Would it make more sense to change the signature of the DEFINE_VECTOR_TYPE() macro to take a third argument containing the function name to call on cleanup paths, with the ability to easily write/reuse a no-op function for vectors that don't need to call free(), where we can then unconditionally declare name##_empty() that will work with all vector types? That is, should we consider instead doing something like: DEFINE_VECTOR_TYPE (string_vector, char *, free); DEFINE_VECTOR_TYPE (int_vector, int, noop); At any rate, I like how the callers look improved, even if we aren't quite settled on how the vector.h changes should end up. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Wed, Feb 15, 2023 at 03:11:36PM +0100, Laszlo Ersek wrote: May be affected by outcome of side discussion on 01/29 about whether we want space after cast. Since no code should ever directly call nbd_internal_fork_safe_assert(0,...), but instead go through our macro that has already filtered on expression's value, this conditional is technically dead code; but I'm okay whether it stays in or gets removed. Presumably, if our first best-effort xwrite() fails to produce full output, all later xwrite() will hopefully hit the same error condition so that we aren't producing even more-mangled output where later syscalls succeed despite missing context earlier in the overall output. If it were something we truly wanted to worry about, the solution would be pre-loading the entire message into a single buffer, then calling xwrite() just once - but that's far more effort for something we don't anticipate hitting in normal usage anyways. I'm happy if you ignore this whole paragraph of mine. These days, the quoting style `' seems to be disappearing in favor of ''. But a quick test showed me that at least glibc 2.36 is still producing the message with the quotes as you have spelled it here. Huh. I checked the source to glibc's assert.c, where it includes a comment about having to work cleanly even if a second assertion ends up being raised from within a SIGABRT handler active during the first attempt to abort(). But POSIX says that abort() shall attempt to override any SIGABRT handler in place, so I wonder if glibc's implementation is a bit too defensive. At any rate, your implementation looks reasonable, and more to the point, satisfies your desire of being async-signal-safe and thus appropriate between fork() and exec(). Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Mon, Feb 20, 2023 at 07:21:05PM +0100, Laszlo Ersek wrote: Oh, I was confused. I went back and re-read your macro; I guess I was thinking it was something like: do if (!(expression)) nbd_internal_fork_safe_assert (args); while (0) where the function is only called on failure, but you instead wrote it as: nbd_internal_fork_safe_assert ((expression) != 0), args) So I stand corrected - we do need the early exit here, because we are unconditionally calling the function (when assertions are enabled) at least in the current spelling of the macro. ... My R-b still stands, and I think you cleared up my confusion without having to change any code on your part. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

Don't try to test async-signal-safety, only that NBD_INTERNAL_FORK_SAFE_ASSERT() works similarly to assert(): - it prints diagnostics to stderr, - it calls abort(). Some unfortunate gymnastics are necessary to avoid littering the system with unwanted core dumps. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- configure.ac | 5 ++ lib/test-fork-safe-assert.c | 63 ++++++++++++++++++++ lib/Makefile.am | 38 ++++++++++-- lib/test-fork-safe-assert.sh | 31 ++++++++++ .gitignore | 2 + 5 files changed, 135 insertions(+), 4 deletions(-) diff --git a/configure.ac b/configure.ac index 893e82ac6fea..3549188ec5fb 100644 --- a/configure.ac +++ b/configure.ac @@ -129,11 +129,16 @@ dnl Check for various libc functions, all optional. dnl dnl posix_fadvise helps to optimise linear reads and writes. dnl +dnl When /proc/sys/kernel/core_pattern starts with a pipe (|) symbol, Linux +dnl ignores "ulimit -c" and (equivalent) setrlimit(RLIMIT_CORE) actions, for +dnl disabling core dumping. Only prctl() can be used then, for that purpose. +dnl dnl strerrordesc_np (glibc only) is preferred over sys_errlist: dnl https://lists.fedoraproject.org/archives/list/glibc@lists.fedoraproject.o... AC_CHECK_FUNCS([\ posix_fadvise \ posix_memalign \ + prctl \ strerrordesc_np \ valloc]) diff --git a/lib/test-fork-safe-assert.c b/lib/test-fork-safe-assert.c new file mode 100644 index 000000000000..131cbaa0abca --- /dev/null +++ b/lib/test-fork-safe-assert.c @@ -0,0 +1,63 @@ +/* nbd client library in userspace + * Copyright (C) 2013-2023 Red Hat Inc. + * + * This library is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 2 of the License, or (at your option) any later version. + * + * This library is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public + * License along with this library; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + */ + +#include <config.h> + +#include <stdio.h> +#include <stdlib.h> +#ifdef HAVE_PRCTL +#include <sys/prctl.h> +#endif +#include <sys/resource.h> + +#undef NDEBUG + +#include "internal.h" + +#define TRUE 1 +#define FALSE 0 + +int +main (void) +{ + struct rlimit rlimit; + + /* The standard approach for disabling core dumping. Has no effect on Linux if + * /proc/sys/kernel/core_pattern starts with a pipe (|) symbol. + */ + if (getrlimit (RLIMIT_CORE, &rlimit) == -1) { + perror ("getrlimit"); + return EXIT_FAILURE; + } + rlimit.rlim_cur = 0; + if (setrlimit (RLIMIT_CORE, &rlimit) == -1) { + perror ("setrlimit"); + return EXIT_FAILURE; + } + +#ifdef HAVE_PRCTL + if (prctl (PR_SET_DUMPABLE, 0, 0, 0, 0) == -1) { + perror ("prctl"); + return EXIT_FAILURE; + } +#endif + + NBD_INTERNAL_FORK_SAFE_ASSERT (TRUE); + NBD_INTERNAL_FORK_SAFE_ASSERT (FALSE); + return 0; +} diff --git a/lib/Makefile.am b/lib/Makefile.am index ece50770326e..1f6501ceb49a 100644 --- a/lib/Makefile.am +++ b/lib/Makefile.am @@ -25,15 +25,30 @@ generator_built = \ unlocked.h \ $(NULL) +CLEANFILES += \ + test-fork-safe-assert.err \ + $(NULL) + EXTRA_DIST = \ $(generator_built) \ libnbd.syms \ + test-fork-safe-assert.sh \ $(NULL) lib_LTLIBRARIES = libnbd.la BUILT_SOURCES = $(generator_built) +AM_CPPFLAGS = \ + -I$(top_srcdir)/include \ + -I$(top_srcdir)/common/include \ + -I$(top_srcdir)/common/utils \ + $(NULL) + +AM_CFLAGS = \ + $(WARNINGS_CFLAGS) \ + $(NULL) + libnbd_la_SOURCES = \ aio.c \ api.c \ @@ -60,13 +75,11 @@ libnbd_la_SOURCES = \ utils.c \ $(NULL) libnbd_la_CPPFLAGS = \ - -I$(top_srcdir)/include \ - -I$(top_srcdir)/common/include \ - -I$(top_srcdir)/common/utils \ + $(AM_CPPFLAGS) \ -Dsysconfdir=\"$(sysconfdir)\" \ $(NULL) libnbd_la_CFLAGS = \ - $(WARNINGS_CFLAGS) \ + $(AM_CFLAGS) \ $(PTHREAD_CFLAGS) \ $(GNUTLS_CFLAGS) \ $(LIBXML2_CFLAGS) \ @@ -86,3 +99,20 @@ libnbd_la_LDFLAGS = \ pkgconfigdir = $(libdir)/pkgconfig pkgconfig_DATA = libnbd.pc + +# Unit tests. + +TESTS = \ + test-fork-safe-assert.sh \ + $(NULL) + +check_PROGRAMS = \ + test-fork-safe-assert \ + $(NULL) + +test_fork_safe_assert_SOURCES = \ + $(top_srcdir)/common/utils/vector.c \ + errors.c \ + test-fork-safe-assert.c \ + utils.c \ + $(NULL) diff --git a/lib/test-fork-safe-assert.sh b/lib/test-fork-safe-assert.sh new file mode 100755 index 000000000000..88fc553b484d --- /dev/null +++ b/lib/test-fork-safe-assert.sh @@ -0,0 +1,31 @@ +#!/usr/bin/env bash +# nbd client library in userspace +# Copyright (C) 2013-2023 Red Hat Inc. +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public +# License along with this library; if not, write to the Free Software +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA + +set +e + +./test-fork-safe-assert 2>test-fork-safe-assert.err +exit_status=$? + +set -e + +test $exit_status -gt 128 +signal_name=$(kill -l $exit_status) +test "x$signal_name" = xABRT || test "x$signal_name" = xSIGABRT + +ptrn="^test-fork-safe-assert\\.c:[0-9]+: main: Assertion \`FALSE' failed\\.\$" +grep -E -q -- "$ptrn" test-fork-safe-assert.err diff --git a/.gitignore b/.gitignore index f42737131d8c..cd27a4ed7557 100644 --- a/.gitignore +++ b/.gitignore @@ -124,6 +124,8 @@ Makefile.in /lib/states-run.c /lib/states.c /lib/states.h +/lib/test-fork-safe-assert +/lib/test-fork-safe-assert.err /lib/unlocked.h /libtool /ltmain.sh

On 2/15/23 22:10, Eric Blake wrote: Right, I'll add a comment. My thinking went like this: on Linux, we'll surely have prctl(), so we expect it to work. On OpenBSD and FreeBSD (the other two platforms we support), we might not have prctl(), but on those platforms, I actually expect setrlimit() to suffice for disabling core dumps. So I don't see a case where we should skip the test. I considered that, but it doesn't do the whole job: there's an apostrophe in the expected output, so minimally I'd have to write ptrn='that'\''s still messy' and then I figured double quotes are just more "consequent". For TRUE to be present together with FALSE, the NBD_INTERNAL_FORK_SAFE_ASSERT (TRUE) macro invocation in the C code would have to produce output and not abort() at the same time. I thought that that didn't need checking (seemed too outlandish), but yes, I can add a "grep -v" too. Thanks! Laszlo

On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote: You KNOW this is going to be a juicy commit message when the first line contains a 2-digit footnote ;) Makes sense. A shame that it requires 3 functions, and that we have to duplicate so much work that libc would normally do for us if we don't care about async-safety, but I think the result is worth it. Accidental double space. Yeah - if you are actively changing out binaries on PATH at the same time as anything else running in earnest on a production system, you've got more issues than just libnbd's potential race. _Technically_, PATH= should behave identically to PATH=.; if you have an executable named './ls' in your working directory, bash WILL execute it! $ mkdir -p /tmp/blah $ cd /tmp/blah $ cp /bin/ls . $ PATH= ls -a . .. ls But whether you want to tweak this patch to actually comply with that corner case is up to you (no one in their right mind should set PATH to empty). Below, I'll refer to this as [1]. Very useful bibliography. The interface looks good for now (if you can't tell, I'm doing a stream-of-consciousness review, and may have further comments about the interface once I see the implementation and/or usage). Are the FIXME comments still necessary? I know we discussed about getenv() being technically non-async-safe in POSIX, but that in all practical aspects, we consider that a multi-threaded app should be smart enough to never modify the environment in parallel with calling into a library like libnbd. At best, you could reword the comment to get rid of FIXME and instead just state our reasoning for why we don't bother with locking the environment: glibc and any other high-quality libc will not be modifying environ during getenv(). Clever, and definitely worth that comment. Nice. [1] This might need tweaking if you treat empty PATH or _CS_PATH as equivalent to "." instead of an unconditional ENOENT. [1] And this simplifies if empty PATH can imply searching in ".". Are we guaranteed that if we get to this fallback, pathnames[i] does not begin with '-' or '+'? Or is it possible to write PATH=/usr/bin:+my/evil/relative/path:... and/or the user to attempt to execute '+my/evil/name' which skips the PATH search because the binary name contains '/', such that the resulting argument to /bin/sh might need to be protected with an extra "--" argument to avoid being misinterpreted as options to the shell rather than the script name to be executed? Is the current working directory something we change between fork() and exec()? If so, should we canonicalize the entries in pathnames[] to always start with '/', so that the child process can reposition directories at will without changing the meaning of the pre-scanned candidates? (Note that canonicalizing names would automatically resolve my above question about safety in the presence of an unusual PATH or binary name beginning with [-+]). [2] Should the interface take the proposed argv[] here pre-fork(), or are we trying to keep this interface generic enough where the child may alter some of the argv post-fork()? On the other hand, I already know we are altering the environment post-fork() (setting LISTEN_PID= cannot be done pre-fork, for starters), so keeping the selection of argv solely in the child rather than making the parent generate it pre-fork seems like the right division of labor. Another point where we may want consistency on whether we do space after cast. [1] Here is where you would code up an empty PATH as being equivalent to ".", if we think it's worth having, probably by just dropping this condition and letting the do-loop below handle it. Since we are in the parent process during this _init() function, can't we just use asprintf() instead of concatenating it all out by hand? [2] Here is where we would want to consider whether to (semi-)canonicalize any relative names into a corresponding absolute prefix, to avoid issues with a relative PATH element starting with [-+]. Clever way to avoid turning PATH=/:/usr/bin:... into an attempt to execute "//binary" which is NOT the same as "/binary". But also possible if you use asprintf(..., "...%.*s...", ..., (int) sep_copy_size, "/", ...) or declare sep_copy_size as int to begin with (because we know it will only be 0 or 1). [2] and here we'd have to canonicalize 'file' itself if we are worrying about the child changing directories and/or starting with a character that could change the behavior of /bin/sh if mistaken as an option instead of a script name. Should we prepopulate sh_argv[0] as a placeholder that can then be easily swapped to ctx->pathnames[i], where the child first attempts execve(pathnames[i], argv+1, env) with the ENOEXEC fallback to argv[0]=pathnames[i]; execve("/bin/sh", argv, env), rather than having to shuffle things in the child process? [3] [3] Oh, I see - the shuffle is still probably needed, because ctx->sh_argv is different than user's argv which does not have a pre-allocated filler slot that we can just swap out. Unless we decide that the _init() function should take argv itself rather than making the child do the work of shifting everything from one array to another, but then we are inconsistent for grabbing argv in the parent and env in the child. Overall looks like nice work. Based on my review, you may decide to make further tweaks, but I didn't see any hard reason why we couldn't use the patch as-written if you don't think my review warrants changes. Thus: Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Wed, Feb 15, 2023 at 03:11:38PM +0100, Laszlo Ersek wrote: Is that actually a problem in the real world ? There are various things not technically listed as async-signal-safe by POSIX, which all sane impls none the less make safe, or are practically safe in any sane program using them. IIUC with execvp the risk would be that setenv makes any use of the environment potentially unsafe, and possibly execvp might use malloc which is also technically unsafe. Both of these factors would technically make your replacement unsafe too. Apps simply shouldn't ever call setenv once threads are created, and malloc() is safe in any impl that is relevant these days. With regards, Daniel -- |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :| |: https://libvirt.org -o- https://fstop138.berrange.com :| |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|

Correctly report "asprintf" in the error message when asprintf() fails. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- generator/states-connect-socket-activation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 3b621b8be44f..c46a0bf5c0a3 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -127,7 +127,7 @@ CONNECT_SA.START: if (asprintf (&h->sact_sockpath, "%s/sock", h->sact_tmpdir) == -1) { SET_NEXT_STATE (%.DEAD); - set_error (errno, "strdup"); + set_error (errno, "asprintf"); return 0; }

On Wed, Feb 15, 2023 at 03:11:40PM +0100, Laszlo Ersek wrote: At the end of the day, "malloc" would work too (we have several places where we aren't precise on WHAT allocation failed, but where it's obvious from the accompanying ENOMEM strerror() message that it was an allocation failure). But when it's easy to be precise, go for it. Reviewed-by: Eric Blake <eblake(a)redhat.com&gt; -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

On Wed, Feb 15, 2023 at 03:11:41PM +0100, Laszlo Ersek wrote: Do you actually need to cast this? Why move this out of the function? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

On 2/17/23 19:36, Eric Blake wrote: No, that's not the right explanation. Elsewhere in the code, we do not use *assignments* for prepping our vector structures with "empty_vector". Instead, everywhere else in the code, we use *initialization*. Those are different contexts. If you expand the "empty_vector" macro in the below: string_vector v1 = empty_vector; string_vector v2; v2 = empty_vector; we get: string_vector v1 = { .ptr = NULL, .len = 0, .cap = 0 }; string_vector v2; v2 = { .ptr = NULL, .len = 0, .cap = 0 }; For "v1", we have a definition that is also an initialization, so the initializer { .ptr = NULL, .len = 0, .cap = 0 } is fine there. For "v2", we have a definition that is not an initialization, and we have an additional *assignment*. For the right hand side of the assignment, the initializer { .ptr = NULL, .len = 0, .cap = 0 } is *not* fine. However, if we write: v2 = (string_vector){ .ptr = NULL, .len = 0, .cap = 0 }; then the RHS of the assignment becomes a C99 *compound literal*, which the initializer { .ptr = NULL, .len = 0, .cap = 0 } is a *proper part of*. The compound literal (string_vector){ .ptr = NULL, .len = 0, .cap = 0 }; is a nameless object with automatic storage duration. It's something like: string_vector v2; string_vector nameless_empty = { .ptr = NULL, .len = 0, .cap = 0 }; v2 = nameless_empty; ... I have a few more comments here: - I generally prefer to *avoid* initialization (especially initialization where the initializer is complex, like a function call!), and use assignments that are performed as late (i.e., as narrowly) as possible. In my opinion, this conveys information about data flow. (I don't like arguments such as "let's initialize this just to be sure" -- we should *know* when a variable is first consumed!) Anyway, in the present case, this argument of mine is not relevant, because the object in question (*env) already exists, we're not *defining* it in the first place (~ definition is such a declaration that allocates space), so we cannot initialize it either (in the C language sense); we can only assign another structure to it, or access its fields, or use memset(). C code that predates C99 would certainly use individual field accesses: env->ptr = NULL; env->len = 0; env->cap = 0; or call memset: memset (env, 0, sizeof *env); or use a small "initializer function" that would thinly wrap one of the above variants. However, in C99, we have compound literals, and that's a great way for reusing the "empty_vector" macro in a (structure) *assignment*. I agree, to an extent. The particular "malloc" string literal is somewhat inexact at this point, but semantically OK. It will become more inexact only later. However, my main point here is that this set_error() call does not belong in the callee. It belongs in the caller. First because the callee does lots of things (and will do more). Second, because the caller already has explicit set_error() calls on all other error branches. Yes, that's it, basically. Thanks, Laszlo

On Tue, Feb 21, 2023 at 02:17:15PM +0100, Laszlo Ersek wrote: OK it's not a cast, but struct assignment is well defined so is the change necessary? The pattern is actually that we call set_error once on each error path [which is surprisingly hard to get right -- we've even tried to write verifier tools for this in the past]. If a function f() calls function g(), where the g() will call set_error, then there's no need for function f() to call set_error on that path. That applies even if there are other disjoint paths where function f() calls set_error, eg. because f() calls malloc directly. I'm unconvinced, couldn't you change the original message to be something like this? set_error (errno, "prepare_socket_activation_environment: malloc"); Adding missing calls to set_error is good, no problem with that. Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

On Tue, Feb 21, 2023 at 05:11:53PM +0100, Laszlo Ersek wrote: So I agree with this one, but ... ... this was my question and ... Right, good point, so ACK to this change, thanks for explaining it to me slowly! Can we keep the first bit (moving the zeroing of *env), and drop this change that moves set_error out of the function? Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On Tue, Feb 21, 2023 at 01:24:17PM +0000, Richard W.M. Jones wrote: Struct assignment requires a source struct. Either a named one (which we do not have here without declaring one), or via a compound literal (which resembles a cast). It sounds like it would be nice to have a type-specific macro, as in 'empty_string_vector', 'empty_int_vector', and so on, per each use of DEFINE_VECTOR_TYPE() - but while our one macro can easily declare other type-specific functions, you can't really use the preprocessor to define further type-specific macros. So having to tell the user to supply their own type name when reusing the type-agnostic initializer portion of a compound literal is not too bad in the long run, although it might be worth documenting the practice in vector.h. -- Eric Blake, Principal Software Engineer Red Hat, Inc. +1-919-301-3266 Virtualization: qemu.org | libvirt.org

F_SETFD takes an "int", so it stands to reason that FD_CLOEXEC expands to an "int". In turn, it's bad hygiene to manipulate the sign bit of (signed) integers with bit operations: ~FD_CLOEXEC Convert FD_CLOEXEC to "unsigned int" for the bitwise complement operator: ~(unsigned)FD_CLOEXEC The bitwise complement then results in an "unsigned int". "Flags" (of type "int", and having, per POSIX, a non-negative value returned by fcntl(F_GETFD)) will be automatically converted to "unsigned int" by the usual arithmetic conversions for the bitwise AND operator: flags & ~(unsigned)FD_CLOEXEC We could pass the result of *that* to fcntl(), due to (a) the value being in range for "signed int" ("flags" is a non-negative "int", and we're only clearing a value bit), and (b) non-negative "int" values being represented identically by "unsigned int" (C99 6.2.5 p9). But, for clarity, let's cast the result to "int" explicitly: (int)(flags & ~(unsigned)FD_CLOEXEC) Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-U5 generator/states-connect-socket-activation.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index b5e146539cc8..729f37d897fb 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -181,11 +181,11 @@ CONNECT_SA.START: int flags = fcntl (s, F_GETFD, 0); if (flags == -1) { nbd_internal_fork_safe_perror ("fcntl: F_GETFD"); _exit (126); } - if (fcntl (s, F_SETFD, flags & ~FD_CLOEXEC) == -1) { + if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) { nbd_internal_fork_safe_perror ("fcntl: F_SETFD"); _exit (126); } }

It's bad hygiene to just assume dup2(), close() and signal() will succeed, especially considering the inconsistency that we do check fcntl(). Check all return values. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-U12 generator/states-connect-socket-activation.c | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 729f37d897fb..766f46177bf3 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -161,50 +161,59 @@ CONNECT_SA.START: } pid = fork (); if (pid == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); close (s); string_vector_empty (&env); return 0; } if (pid == 0) { /* child - run command */ if (s != FIRST_SOCKET_ACTIVATION_FD) { - dup2 (s, FIRST_SOCKET_ACTIVATION_FD); - close (s); + if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) { + nbd_internal_fork_safe_perror ("dup2"); + _exit (126); + } + if (close (s) == -1) { + nbd_internal_fork_safe_perror ("close"); + _exit (126); + } } else { /* We must unset CLOEXEC on the fd. (dup2 above does this * implicitly because CLOEXEC is set on the fd, not on the * socket). */ int flags = fcntl (s, F_GETFD, 0); if (flags == -1) { nbd_internal_fork_safe_perror ("fcntl: F_GETFD"); _exit (126); } if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) { nbd_internal_fork_safe_perror ("fcntl: F_SETFD"); _exit (126); } } char buf[32]; const char *v = nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf); strcpy (&env.ptr[0][PREFIX_LENGTH], v); /* Restore SIGPIPE back to SIG_DFL. */ - signal (SIGPIPE, SIG_DFL); + if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { + nbd_internal_fork_safe_perror ("signal"); + _exit (126); + } environ = env.ptr; execvp (h->argv.ptr[0], h->argv.ptr); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } /* Parent. */ close (s);

The CONNECT_SA.START handler constructs several resources; some of those are needed permanently if the entire handler succeeds, while others are needed only temporarily, for constructing the permanent objects. Currently, the various error branches in the handler deal with resource release individually, leading to code duplication and dispersed responsibilities; if we want to construct a new resource, then, under the existent pattern, we'll have to modify multiple error branches to release it as well. In my opinion, the best pattern for dealing with this scenario is the following structure: - Place cascading error handling labels at the end of the function -- an error handling slide in effect. The order of destruction on this slide is the inverse of construction. - Whenever an operation fails mid-construction, enter the slide such that precisely the thus far constructed objects be freed. Note that the goto statements and the destination labels nest properly. - The very last construction step needs no rollback, because the last successful step completes the entire function / handler. When this happens, *commit* by (a) outputting the permanent resources, (b) setting a top-level "final result" variable to "success" (aka "ownership of permanent resources transferred to the caller"), and (c) falling through to the slide. On the slide, use the "final result" variable to skip the release of those resources that have been output as permanent ones. This way, each resource is freed in exactly one location, and whenever an error occurs, no resource is even *checked* for rollback if its construction could not be tried, or completed, in the first place. The introduction of a new resource needs one properly placed construction step and one matchingly placed error handling label. The restructuring highlights the following leak: whenever any construction step after bind() fails, the UNIX domain socket address (i.e., the pathname in the filesystem) is leaked. (The filesystem object may well be removed once the application decides to call nbd_close(), but until then, after the CONNECT_SA.START handler returns with failure, unusable (half-constructed) resources linger around indefinitely. A library API should either fully succeed or fully fail.) We'll plug the leak later in this series. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-W generator/states-connect-socket-activation.c | 82 ++++++++++++-------- 1 file changed, 48 insertions(+), 34 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index 766f46177bf3..164b8e6828f2 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -96,132 +96,146 @@ prepare_socket_activation_environment (string_vector *env) STATE_MACHINE { CONNECT_SA.START: + enum state next; + char *tmpdir; + char *sockpath; int s; struct sockaddr_un addr; string_vector env; pid_t pid; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); + next = %.DEAD; + /* Use /tmp instead of TMPDIR because we must ensure the path is * short enough to store in the sockaddr_un. On some platforms this * may cause problems so we may need to revisit it. XXX */ - h->sact_tmpdir = strdup ("/tmp/libnbdXXXXXX"); - if (h->sact_tmpdir == NULL) { - SET_NEXT_STATE (%.DEAD); + tmpdir = strdup ("/tmp/libnbdXXXXXX"); + if (tmpdir == NULL) { set_error (errno, "strdup"); - return 0; + goto done; } - if (mkdtemp (h->sact_tmpdir) == NULL) { - SET_NEXT_STATE (%.DEAD); + + if (mkdtemp (tmpdir) == NULL) { set_error (errno, "mkdtemp"); - /* Avoid cleanup in nbd_close. */ - free (h->sact_tmpdir); - h->sact_tmpdir = NULL; - return 0; + goto free_tmpdir; } - if (asprintf (&h->sact_sockpath, "%s/sock", h->sact_tmpdir) == -1) { - SET_NEXT_STATE (%.DEAD); + if (asprintf (&sockpath, "%s/sock", tmpdir) == -1) { set_error (errno, "asprintf"); - return 0; + goto rmdir_tmpdir; } s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false); if (s == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "socket"); - return 0; + goto free_sockpath; } addr.sun_family = AF_UNIX; - memcpy (addr.sun_path, h->sact_sockpath, strlen (h->sact_sockpath) + 1); + memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1); if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) { - SET_NEXT_STATE (%.DEAD); - set_error (errno, "bind: %s", h->sact_sockpath); - close (s); - return 0; + set_error (errno, "bind: %s", sockpath); + goto close_socket; } if (listen (s, SOMAXCONN) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "listen"); - close (s); - return 0; + goto close_socket; } if (prepare_socket_activation_environment (&env) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "prepare_socket_activation_environment"); - close (s); - return 0; + goto close_socket; } pid = fork (); if (pid == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); - close (s); - string_vector_empty (&env); - return 0; + goto empty_env; } + if (pid == 0) { /* child - run command */ if (s != FIRST_SOCKET_ACTIVATION_FD) { if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) { nbd_internal_fork_safe_perror ("dup2"); _exit (126); } if (close (s) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } } else { /* We must unset CLOEXEC on the fd. (dup2 above does this * implicitly because CLOEXEC is set on the fd, not on the * socket). */ int flags = fcntl (s, F_GETFD, 0); if (flags == -1) { nbd_internal_fork_safe_perror ("fcntl: F_GETFD"); _exit (126); } if (fcntl (s, F_SETFD, (int)(flags & ~(unsigned)FD_CLOEXEC)) == -1) { nbd_internal_fork_safe_perror ("fcntl: F_SETFD"); _exit (126); } } char buf[32]; const char *v = nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf); strcpy (&env.ptr[0][PREFIX_LENGTH], v); /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); } environ = env.ptr; execvp (h->argv.ptr[0], h->argv.ptr); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } - /* Parent. */ - close (s); - string_vector_empty (&env); + /* Parent -- we're done; commit. */ + h->sact_tmpdir = tmpdir; + h->sact_sockpath = sockpath; h->pid = pid; h->connaddrlen = sizeof addr; memcpy (&h->connaddr, &addr, h->connaddrlen); - SET_NEXT_STATE (%^CONNECT.START); + next = %^CONNECT.START; + + /* fall through, for releasing the temporaries */ + +empty_env: + string_vector_empty (&env); + +close_socket: + close (s); + +free_sockpath: + if (next == %.DEAD) + free (sockpath); + +rmdir_tmpdir: + if (next == %.DEAD) + rmdir (tmpdir); + +free_tmpdir: + if (next == %.DEAD) + free (tmpdir); + +done: + SET_NEXT_STATE (next); return 0; } /* END STATE MACHINE */

On 2/15/23 17:53, Richard W.M. Jones wrote: Ah, thank you for pointing that out! I'll capture this in the commit message. Laszlo

On Wed, Feb 15, 2023 at 03:11:45PM +0100, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

On Wed, Feb 15, 2023 at 03:11:46PM +0100, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

On Wed, Feb 15, 2023 at 03:11:47PM +0100, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On Wed, Feb 15, 2023 at 03:11:48PM +0100, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com nbdkit - Flexible, fast NBD server with plugins https://gitlab.com/nbdkit/nbdkit

On Wed, Feb 15, 2023 at 03:11:49PM +0100, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into KVM guests. http://libguestfs.org/virt-v2v

On Wed, Feb 15, 2023 at 03:11:50PM +0100, Laszlo Ersek wrote: Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://people.redhat.com/~rjones/virt-top

On Wed, Feb 15, 2023 at 03:11:51PM +0100, Laszlo Ersek wrote: For use of exit code 126, see: https://listman.redhat.com/archives/libguestfs/2019-September/022737.html https://listman.redhat.com/archives/libguestfs/2019-September/022739.html https://www.gnu.org/software/bash/manual/html_node/Exit-Status.html Reviewed-by: Richard W.M. Jones <rjones(a)redhat.com&gt; Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com virt-builder quickly builds VMs from scratch http://libguestfs.org/virt-builder.1.html

This patch implements the same ideas as those presented in the prior patch socket activation: centralize resource release of this same series, only for CONNECT_COMMAND.START. The one temporary resource we have here is sv[1]. Similarly to the above-cited patch, the present patch also exposes a resource leak on the error path. Namely, when any construction step fails in the parent after fork(), the child process is leaked. (The child process may well be killed and reaped once the application decides to call nbd_close(), but until then, after the CONNECT_COMMAND.START handler returns with failure, unusable (half-constructed) resources linger around indefinitely. A library API should either fully succeed or fully fail.) We are going to plug this leak in a subsequent patch. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-W generator/states-connect.c | 52 +++++++++++--------- 1 file changed, 30 insertions(+), 22 deletions(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index d2476dc11c60..bd78f890a2c5 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -238,96 +238,104 @@ CONNECT_TCP.NEXT_ADDRESS: return 0; CONNECT_COMMAND.START: + enum state next; int sv[2]; pid_t pid; int flags; + struct socket *sock; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); + + next = %.DEAD; + if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); - return 0; + goto done; } /* A process is effectively in an unusable state if any of STDIN_FILENO * (fd#0), STDOUT_FILENO (fd#1) and STDERR_FILENO (fd#2) don't exist. If they * exist however, then the socket pair created above will not intersect with * the fd set { 0, 1, 2 }. This is relevant for the child-side dup2() logic * below. */ assert (sv[0] > STDERR_FILENO); assert (sv[1] > STDERR_FILENO); pid = fork (); if (pid == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "fork"); - close (sv[0]); - close (sv[1]); - return 0; + goto close_socket_pair; } + if (pid == 0) { /* child - run command */ if (close (sv[0]) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } if (dup2 (sv[1], STDIN_FILENO) == -1 || dup2 (sv[1], STDOUT_FILENO) == -1) { nbd_internal_fork_safe_perror ("dup2"); _exit (126); } NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO); NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO); if (close (sv[1]) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); } execvp (h->argv.ptr[0], h->argv.ptr); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } - /* Parent. */ - close (sv[1]); - - h->pid = pid; - - /* Only the parent-side end of the socket pair must be set to non-blocking, + /* Parent. + * + * Only the parent-side end of the socket pair must be set to non-blocking, * because the child may not be expecting a non-blocking socket. */ flags = fcntl (sv[0], F_GETFL, 0); if (flags == -1 || fcntl (sv[0], F_SETFL, flags|O_NONBLOCK) == -1) { - SET_NEXT_STATE (%.DEAD); set_error (errno, "fcntl"); - close (sv[0]); - return 0; + goto close_socket_pair; } - h->sock = nbd_internal_socket_create (sv[0]); - if (!h->sock) { - SET_NEXT_STATE (%.DEAD); + sock = nbd_internal_socket_create (sv[0]); + if (!sock) /* nbd_internal_socket_create() calls set_error() internally */ - close (sv[0]); - return 0; - } + goto close_socket_pair; + + /* Commit. */ + h->pid = pid; + h->sock = sock; /* The sockets are connected already, we can jump directly to * receiving the server magic. */ - SET_NEXT_STATE (%^MAGIC.START); + next = %^MAGIC.START; + + /* fall through, for releasing the temporaries */ + +close_socket_pair: + if (next == %.DEAD) + close (sv[0]); + close (sv[1]); + +done: + SET_NEXT_STATE (next); return 0; } /* END STATE MACHINE */

Per POSIX, execvp() is not safe to call in a child process forked from a multi-threaded process. We can now replace the execvp() call in the child process with a call to our fork-safe (async-signal-safe) variant. Prepare our internal execvpe context on the parent's construction path, use the context in the child, and release the context in the parent on the way out, regardless of whether the handler as a whole succeeded or not. (The context is only a temporary resource.) Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-U7 generator/states-connect.c | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/generator/states-connect.c b/generator/states-connect.c index 283481bb7f91..4fc01f3b46cb 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -32,14 +32,16 @@ #include <signal.h> #include <netdb.h> #include <netinet/in.h> #include <netinet/tcp.h> #include <sys/types.h> #include <sys/socket.h> +extern char **environ; + /* Disable Nagle's algorithm on the socket, but don't fail. */ static void disable_nagle (int sock) { const int flag = 1; setsockopt (sock, IPPROTO_TCP, TCP_NODELAY, &flag, sizeof flag); @@ -239,14 +241,15 @@ CONNECT_TCP.NEXT_ADDRESS: CONNECT_COMMAND.START: enum state next; bool parentfd_transferred; int sv[2]; int flags; struct socket *sock; + struct execvpe execvpe_ctx; pid_t pid; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); next = %.DEAD; @@ -278,18 +281,24 @@ CONNECT_COMMAND.START: sock = nbd_internal_socket_create (sv[0]); if (!sock) /* nbd_internal_socket_create() calls set_error() internally */ goto close_socket_pair; parentfd_transferred = true; + if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0], h->argv.len) == + -1) { + set_error (errno, "nbd_internal_execvpe_init"); + goto close_high_level_socket; + } + pid = fork (); if (pid == -1) { set_error (errno, "fork"); - goto close_high_level_socket; + goto uninit_execvpe; } if (pid == 0) { /* child - run command */ if (close (sv[0]) == -1) { nbd_internal_fork_safe_perror ("close"); _exit (126); } @@ -307,15 +316,15 @@ CONNECT_COMMAND.START: /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); } - execvp (h->argv.ptr[0], h->argv.ptr); + (void)nbd_internal_fork_safe_execvpe (&execvpe_ctx, &h->argv, environ); nbd_internal_fork_safe_perror (h->argv.ptr[0]); if (errno == ENOENT) _exit (127); else _exit (126); } @@ -326,14 +335,17 @@ CONNECT_COMMAND.START: /* The sockets are connected already, we can jump directly to * receiving the server magic. */ next = %^MAGIC.START; /* fall through, for releasing the temporaries */ +uninit_execvpe: + nbd_internal_execvpe_uninit (&execvpe_ctx); + close_high_level_socket: if (next == %.DEAD) sock->ops->close (sock); close_socket_pair: assert (next == %.DEAD || parentfd_transferred); if (!parentfd_transferred)

In the future, we'd like to add systemd socket activation environment variables such that: - their values may not be constants (not even for pre-allocating space), - they may be optional, - regardless of some optional variables being added or not, the positions of those that we do add can be captured, for later reference. Generalize prepare_socket_activation_environment() accordingly. Write the child PID to "LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx" with the new "capturing" interface. Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- Notes: context:-U6 generator/states-connect-socket-activation.c | 160 +++++++++++++++----- 1 file changed, 125 insertions(+), 35 deletions(-) diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index cb3b1c6f4ede..94d641cde6d9 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -27,84 +27,168 @@ #include <errno.h> #include <assert.h> #include <sys/socket.h> #include <sys/un.h> #include "internal.h" +#include "compiler-macros.h" +#include "unique-name.h" +#include "array-size.h" +#include "checked-overflow.h" /* This is baked into the systemd socket activation API. */ #define FIRST_SOCKET_ACTIVATION_FD 3 -/* == strlen ("LISTEN_PID=") | strlen ("LISTEN_FDS=") */ -#define PREFIX_LENGTH 11 +/* Describes a systemd socket activation environment variable. */ +struct sact_var { + const char *prefix; /* variable name and equal sign */ + size_t prefix_len; + const char *value; + size_t value_len; +}; + +/* Determine the length of a string, using "sizeof" whenever possible. + * + * Do not use this macro on an argument that has side effects, as no guarantees + * are given regarding the number of times the argument may be evaluated. + * TYPE_IS_ARRAY(s) itself may contribute a different number of evaluations + * dependent on whether "s" has variably modified type, and then the conditional + * operator either evaluates "sizeof s" (which contributes 0 or 1 evaluations, + * dependent on whether "s" has variably modified type) or strlen(s) (which + * contributes 1 evaluation). Also note that the argument of the "sizeof" + * operator is *only* parenthesized because "s" is a macro parameter here. +*/ +#define STRLEN1(s) ((TYPE_IS_ARRAY (s) ? sizeof (s) - 1 : strlen (s))) + +/* Push a new element to an array of "sact_var" structures. + * + * "vars" is the array to extend. "num_vars" (of type (size_t *)) points to the + * number of elements that the array, on input, contains; (*num_vars) is + * increased by one on output. "prefix" and "value" serve as the values for + * setting the fields in the new element. "ofs" (of type (size_t *)) may be + * NULL; if it isn't, then on output, (*ofs) is set to the input value of + * (*num_vars): the offset of the just-pushed element. + * + * Avoid arguments with side-effects here as well. + */ +#define SACT_VAR_PUSH(vars, num_vars, prefix, value, ofs) \ + SACT_VAR_PUSH1 ((vars), (num_vars), (prefix), (value), (ofs), \ + NBDKIT_UNIQUE_NAME (_ofs)) +#define SACT_VAR_PUSH1(vars, num_vars, prefix, value, ofs, ofs1) \ + do { \ + size_t *ofs1; \ + \ + assert (*(num_vars) < ARRAY_SIZE (vars)); \ + ofs1 = (ofs); \ + if (ofs1 != NULL) \ + *ofs1 = *(num_vars); \ + (vars)[(*(num_vars))++] = (struct sact_var){ (prefix), STRLEN1 (prefix), \ + (value), STRLEN1 (value) }; \ + } while (0) extern char **environ; -/* Prepare environment for calling execvp when doing systemd socket - * activation. Takes the current environment and copies it. Removes - * any existing LISTEN_PID or LISTEN_FDS and replaces them with new - * variables. env[0] is "LISTEN_PID=..." which is filled in by - * CONNECT_SA.START, and env[1] is "LISTEN_FDS=1". +/* Prepare environment for calling execvp when doing systemd socket activation. + * Takes the current environment and copies it. Removes any existing socket + * activation variables and replaces them with new ones. Variables in "sact_var" + * will be placed at the front of "env", preserving the order from "sact_var". */ static int -prepare_socket_activation_environment (string_vector *env) +prepare_socket_activation_environment (string_vector *env, + const struct sact_var *sact_var, + size_t num_vars) { - char *p; + const struct sact_var *var_end; + char *new_var; + const struct sact_var *var; size_t i; *env = (string_vector)empty_vector; - /* Reserve slots env[0] and env[1]. */ - p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"); - if (p == NULL) - goto err; - if (string_vector_append (env, p) == -1) { - free (p); - goto err; - } - p = strdup ("LISTEN_FDS=1"); - if (p == NULL) - goto err; - if (string_vector_append (env, p) == -1) { - free (p); - goto err; + /* Set the exclusive limit for loops over "sact_var". */ + var_end = sact_var + num_vars; + + /* New environment variable being constructed for "env". */ + new_var = NULL; + + /* Copy "sact_var" to the front of "env". */ + for (var = sact_var; var < var_end; ++var) { + size_t new_var_size; + char *p; + + /* Calculate the size of the "NAME=value" string. */ + if (ADD_OVERFLOW (var->prefix_len, var->value_len, &new_var_size) || + ADD_OVERFLOW (new_var_size, 1u, &new_var_size)) { + errno = EOVERFLOW; + goto err; + } + + /* Allocate and format "NAME=value". */ + new_var = malloc (new_var_size); + if (new_var == NULL) + goto err; + p = new_var; + + memcpy (p, var->prefix, var->prefix_len); + p += var->prefix_len; + + memcpy (p, var->value, var->value_len); + p += var->value_len; + + *p++ = '\0'; + + /* Push "NAME=value" to the vector. */ + if (string_vector_append (env, new_var) == -1) + goto err; + /* Ownership transferred. */ + new_var = NULL; } - /* Append the current environment, but remove LISTEN_PID, LISTEN_FDS. */ + /* Append the current environment to "env", but remove "sact_var". */ for (i = 0; environ[i] != NULL; ++i) { - if (strncmp (environ[i], "LISTEN_PID=", PREFIX_LENGTH) != 0 && - strncmp (environ[i], "LISTEN_FDS=", PREFIX_LENGTH) != 0) { - char *copy = strdup (environ[i]); - if (copy == NULL) - goto err; - if (string_vector_append (env, copy) == -1) { - free (copy); - goto err; - } + for (var = sact_var; var < var_end; ++var) { + if (strncmp (environ[i], var->prefix, var->prefix_len) == 0) + break; } + /* Drop known socket activation variable from the current environment. */ + if (var < var_end) + continue; + + new_var = strdup (environ[i]); + if (new_var == NULL) + goto err; + + if (string_vector_append (env, new_var) == -1) + goto err; + /* Ownership transferred. */ + new_var = NULL; } /* The environ must be NULL-terminated. */ if (string_vector_append (env, NULL) == -1) goto err; return 0; err: + free (new_var); string_vector_empty (env); return -1; } STATE_MACHINE { CONNECT_SA.START: enum state next; char *tmpdir; char *sockpath; int s; struct sockaddr_un addr; struct execvpe execvpe_ctx; + size_t num_vars; + struct sact_var sact_var[2]; + size_t pid_ofs; string_vector env; pid_t pid; assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); @@ -152,13 +236,18 @@ CONNECT_SA.START: if (nbd_internal_execvpe_init (&execvpe_ctx, h->argv.ptr[0], h->argv.len) == -1) { set_error (errno, "nbd_internal_execvpe_init"); goto unlink_sockpath; } - if (prepare_socket_activation_environment (&env) == -1) { + num_vars = 0; + SACT_VAR_PUSH (sact_var, &num_vars, + "LISTEN_PID=", "xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs); + SACT_VAR_PUSH (sact_var, &num_vars, + "LISTEN_FDS=", "1", NULL); + if (prepare_socket_activation_environment (&env, sact_var, num_vars) == -1) { set_error (errno, "prepare_socket_activation_environment"); goto uninit_execvpe; } pid = fork (); if (pid == -1) { @@ -193,13 +282,14 @@ CONNECT_SA.START: } } char buf[32]; const char *v = nbd_internal_fork_safe_itoa ((long) getpid (), buf, sizeof buf); - strcpy (&env.ptr[0][PREFIX_LENGTH], v); + NBD_INTERNAL_FORK_SAFE_ASSERT (strlen (v) <= sact_var[pid_ofs].value_len); + strcpy (env.ptr[pid_ofs] + sact_var[pid_ofs].prefix_len, v); /* Restore SIGPIPE back to SIG_DFL. */ if (signal (SIGPIPE, SIG_DFL) == SIG_ERR) { nbd_internal_fork_safe_perror ("signal"); _exit (126); }

From: "Richard W.M. Jones" <rjones(a)redhat.com&gt; To allow us to name the socket passed down to the NBD server when calling nbd_connect_systemd_socket_activation(3), we need to add the field to the handle and add access functions. [Laszlo's note: originally posted by Rich at <https://listman.redhat.com/archives/libguestfs/2023-January/030557.html>;. I've renamed "sa_name" to "sact_name", due to <signal.h> reserving symbols with the "sa_" prefix. This corresponds to earlier patches in this series, such as 'socket activation: rename sa_(tmpdir|sockpath) to sact_(tmpdir|sockpath)' and 'ocaml: rename "sa_u" to "saddr_u"'.] Message-Id: <20230130225521.1771496-4-rjones(a)redhat.com&gt; Signed-off-by: Laszlo Ersek <lersek(a)redhat.com&gt; --- lib/internal.h | 1 + generator/API.ml | 49 +++++++++++++++++ lib/handle.c | 56 ++++++++++++++++++++ 3 files changed, 106 insertions(+) diff --git a/lib/internal.h b/lib/internal.h index 8c4e32013e40..a63fbe05641c 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -101,6 +101,7 @@ struct nbd_handle { _Atomic uintptr_t private_data; char *export_name; /* Export name, never NULL. */ + char *sact_name; /* Socket activation name, can be NULL. */ /* TLS settings. */ int tls; /* 0 = disable, 1 = enable, 2 = require */ diff --git a/generator/API.ml b/generator/API.ml index 25a612a2e864..08fc80960b15 100644 --- a/generator/API.ml +++ b/generator/API.ml @@ -2036,15 +2036,62 @@ "connect_systemd_socket_activation", { When the NBD handle is closed the server subprocess is killed. + +=head3 Socket name + +The socket activation protocol lets you optionally give +the socket a name. If used, the name is passed to the +NBD server using the C<LISTEN_FDNAMES> environment +variable. To provide a socket name, call +L<nbd_set_socket_activation_name(3)> before calling +the connect function. " ^ blocking_connect_call_description; see_also = [Link "aio_connect_systemd_socket_activation"; Link "connect_command"; Link "kill_subprocess"; Link "set_opt_mode"; + Link "set_socket_activation_name"; + Link "get_socket_activation_name"; ExternalLink ("qemu-nbd", 1); URLLink "http://0pointer.de/blog/projects/socket-activation.html"]; example = Some "examples/open-qcow2.c"; }; + "set_socket_activation_name", { + default_call with + args = [ String "socket_name" ]; ret = RErr; + shortdesc = "set the socket activation name"; + longdesc = "\ +When running an NBD server using +L<nbd_connect_systemd_socket_activation(3)> you can optionally +name the socket. Call this function before connecting to the +server. + +Some servers such as L<qemu-storage-daemon(1)> +can use this information to associate the socket with a name +used on the command line, but most servers will ignore it. +The name is passed through the C<LISTEN_FDNAMES> environment +variable. + +The parameter C<socket_name> can be a short alphanumeric string. +If it is set to the empty string (also the default when the handle +is created) then no name is passed to the server."; + see_also = [Link "connect_systemd_socket_activation"; + Link "get_socket_activation_name"]; + }; + + "get_socket_activation_name", { + default_call with + args = []; ret = RString; + shortdesc = "get the socket activation name"; + longdesc = "\ +Return the socket name used when you call +L<nbd_connect_systemd_socket_activation(3)> on the same +handle. By default this will return the empty string +meaning that no name is passed to the server."; + see_also = [Link "connect_systemd_socket_activation"; + Link "set_socket_activation_name"]; + }; + "is_read_only", { default_call with args = []; ret = RBool; @@ -3844,6 +3891,8 @@ "get_uri", { "aio_opt_structured_reply", (1, 16); "opt_starttls", (1, 16); "aio_opt_starttls", (1, 16); + "set_socket_activation_name", (1, 16); + "get_socket_activation_name", (1, 16); (* These calls are proposed for a future version of libnbd, but * have not been added to any released version so far. diff --git a/lib/handle.c b/lib/handle.c index fb92f16e6c91..d78372cb0e07 100644 --- a/lib/handle.c +++ b/lib/handle.c @@ -28,6 +28,7 @@ #include <sys/types.h> #include <sys/wait.h> +#include "ascii-ctype.h" #include "internal.h" static void @@ -159,6 +160,7 @@ nbd_close (struct nbd_handle *h) waitpid (h->pid, NULL, 0); free (h->export_name); + free (h->sact_name); free (h->tls_certificates); free (h->tls_username); free (h->tls_psk_file); @@ -197,6 +199,60 @@ nbd_unlocked_get_handle_name (struct nbd_handle *h) return copy; } +int +nbd_unlocked_set_socket_activation_name (struct nbd_handle *h, + const char *name) +{ + size_t i, len; + char *new_name; + + len = strlen (name); + + /* Setting it to empty string stores NULL in the handle. */ + if (len == 0) { + free (h->sact_name); + h->sact_name = NULL; + return 0; + } + + /* Check the proposed name is short and alphanumeric. */ + if (len > 32) { + set_error (ENAMETOOLONG, "socket activation name should be " + "<= 32 characters"); + return -1; + } + for (i = 0; i < len; ++i) { + if (! ascii_isalnum (name[i])) { + set_error (EINVAL, "socket activation name should contain " + "only alphanumeric ASCII characters"); + return -1; + } + } + + new_name = strdup (name); + if (!new_name) { + set_error (errno, "strdup"); + return -1; + } + + free (h->sact_name); + h->sact_name = new_name; + return 0; +} + +char * +nbd_unlocked_get_socket_activation_name (struct nbd_handle *h) +{ + char *copy = strdup (h->sact_name ? h->sact_name : ""); + + if (!copy) { + set_error (errno, "strdup"); + return NULL; + } + + return copy; +} + uintptr_t nbd_unlocked_set_private_data (struct nbd_handle *h, uintptr_t data) {