Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 00/29] pass LISTEN_FDNAMES with systemd socket activation
Rich posted v2 at:
https://listman.redhat.com/archives/libguestfs/2023-January/030555.html
This is my stab at the feature, with a number of changes (that I felt
were prerequisites) prepended. The series is structured as follows. Note
that almost every segment of the series strongly depends on prior
segment(s); in other words, the patches are in reasonably strict
dependency order.
- Patches #1 through #4 are generic, mostly mechanical, tree-wide
cleanups, dealing with whitespace and reserved identifiers.
- Patch #5 extracts a vector usage pattern we use 11 times in the tree
to a new vector API called "name##_empty".
- Patches #6 through #8 introduce an assert() variant that is
async-signal-safe. This means we can make assertions in a child
process before reaching one of the exec functions, even if the parent
process was multi-threaded at the time of fork().
- Patches #9 and #10 introduce an execvpe() variant that is
async-signal-safe.
- Patches #11 through #25 implement the same series of cleanups *twice*,
once for CONNECT_SA.START ("socket activation"), and another time
for
CONNECT_COMMAND.START. The table below describes this sub-structure:
change summary CONNECT_SA CONNECT_COMMAND
---------------------------------------------- ---------- ---------------
minor thinkos and warts #11 - #13 #18 - #21
check syscalls for errors in the child process #14 #22
centralize resource release #15 #23
plug resource leak on error #16 #24
replace execvp() call with fork-safe variant #17 #25
- Patches #26 through #29 implement the LISTEN_FDNAMES passing. This
sub-series includes two patches from Rich's v2 series (with proper
attribution).
The series buils, and passes "make check" and "make
check-valgrind", at
every stage.
In the Notes sections of some of the patches, there are lines of the
form "context:-U<number>" or "context:-W". I've
always wanted to control
the number of context lines at the *individual* patch level -- for
reviewing some patches, the default 3 is just fine, but for reviewing
many other patches, significantly larger contexts are beneficial. I've
finally bitten the bullet and written a (rather awkward) script that
parses this "context:" note out of each patch (where "-W" is
the short
form of "--function--context") and applies it to formatting.
That's why
the context size varies over the series.
The larger context size may make it harder to apply the series from the
list if the master branch advances meanwhile. I don't expect this to
happen, but in any case, I've passed "--base=master" to
git-format-patch, so that the base commit (the current HEAD of the
master branch) be noted at the end of the cover letter. That way the
series can be applied precisely, and then rebased with a separate local
step (because git has more information that way).
Laszlo
Laszlo Ersek (27):
use space consistently in function and function-like macro invocations
generator/C.ml: use space consistently in func. and func.-like macro
calls
socket activation: rename sa_(tmpdir|sockpath) to
sact_(tmpdir|sockpath)
ocaml: rename "sa_u" to "saddr_u"
vector: (mostly) factor out DEFINE_VECTOR_EMPTY
lib/utils: introduce xwrite() as a more robust write()
lib/utils: add async-signal-safe assert()
lib/utils: add unit test for async-signal-safe assert()
lib/utils: introduce async-signal-safe execvpe()
lib/utils: add unit tests for async-signal-safe execvpe()
socket activation: fix error message upon asprintf() failure
socket activation: clean up responsibilities of prep.sock.act.env.()
socket activation: avoid manipulating the sign bit
socket activation: check syscalls for errors in the child process
socket activation: centralize resource release
socket activation: plug AF_UNIX socket address (filesystem) leak on
error
socket activation: replace execvp() call with fork-safe variant
CONNECT_COMMAND.START: fix small comment thinko about socket pair
usage
CONNECT_COMMAND.START: set the NBD error when fcntl() fails
CONNECT_COMMAND.START: use symbolic constants for fd#0 and fd#1
CONNECT_COMMAND.START: sanitize close() calls in the child process
CONNECT_COMMAND.START: check syscalls for errors in the child process
CONNECT_COMMAND.START: centralize resource release
CONNECT_COMMAND.START: plug child process leak on error
CONNECT_COMMAND.START: replace execvp() call with fork-safe variant
socket activation: generalize environment construction
socket activation: set LISTEN_FDNAMES
Richard W.M. Jones (2):
common/include: Copy ascii-ctype functions from nbdkit
generator: Add APIs to get/set the socket activation socket name
.gitignore | 4 +
common/include/Makefile.am | 6 +
common/include/array-size.h | 2 +-
common/include/ascii-ctype.h | 75 ++++
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/include/test-array-size.c | 26 +-
common/include/test-ascii-ctype.c | 88 ++++
common/utils/const-string-vector.h | 2 +-
common/utils/nbdkit-string.h | 2 +-
common/utils/string-vector.h | 3 +-
common/utils/test-human-size.c | 10 +-
common/utils/test-vector.c | 7 +-
common/utils/vector.h | 45 +-
configure.ac | 5 +
copy/file-ops.c | 2 +-
copy/main.c | 2 +-
copy/nbdcopy.h | 2 +-
dump/dump.c | 2 +-
examples/copy-libev.c | 30 +-
examples/list-exports.c | 2 +-
examples/strict-structured-reads.c | 2 +-
examples/threaded-reads-and-writes.c | 2 +-
fuse/nbdfuse.c | 2 +-
generator/API.ml | 49 +++
generator/C.ml | 20 +-
generator/states-connect-socket-activation.c | 294 +++++++++----
generator/states-connect.c | 123 ++++--
info/main.c | 2 +-
info/show.c | 3 +-
interop/interop.c | 2 +-
lib/Makefile.am | 48 ++-
lib/errors.c | 6 +-
lib/handle.c | 80 +++-
lib/internal.h | 112 +++--
lib/nbd-protocol.h | 12 +-
lib/opt.c | 4 +-
lib/test-fork-safe-assert.c | 63 +++
lib/test-fork-safe-assert.sh | 31 ++
lib/test-fork-safe-execvpe.c | 117 ++++++
lib/test-fork-safe-execvpe.sh | 270 ++++++++++++
lib/uri.c | 2 +-
lib/utils.c | 430 +++++++++++++++++++-
ocaml/helpers.c | 8 +-
ocaml/nbd-c.h | 6 +-
tests/eflags.c | 6 +-
tests/get-size.c | 4 +-
tests/newstyle-limited.c | 6 +-
tests/oldstyle.c | 4 +-
ublk/nbdublk.c | 4 +-
ublk/tgt.c | 20 +-
54 files changed, 1771 insertions(+), 350 deletions(-)
create mode 100644 common/include/ascii-ctype.h
create mode 100644 common/include/test-ascii-ctype.c
create mode 100644 lib/test-fork-safe-assert.c
create mode 100755 lib/test-fork-safe-assert.sh
create mode 100644 lib/test-fork-safe-execvpe.c
create mode 100755 lib/test-fork-safe-execvpe.sh
base-commit: 5a02c7d2cc6a201f9e5531c0c20c2f3c22b805a2
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 01/29] use space consistently in function and function-like macro invocations
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 at redhat.com>
---
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;
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 02/29] generator/C.ml: use space consistently in func. and func.-like macro calls
Apply the ideas in the previous patch to the C-language bindings
generator.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
generator/C.ml | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/generator/C.ml b/generator/C.ml
index f9171996dde0..07c924c48ccf 100644
--- a/generator/C.ml
+++ b/generator/C.ml
@@ -250,15 +250,15 @@ let
*)
(match ret with
| RString ->
- pr "\n LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(__builtin_free)";
+ pr "\n LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (__builtin_free)";
| _ -> ()
);
(* Output __attribute__((nonnull)) for the function parameters:
* eg. struct nbd_handle *, int, char *
* => [ true, false, true ]
- * => LIBNBD_ATTRIBUTE_NONNULL(1,3)
- * => __attribute__((nonnull(1,3)))
+ * => LIBNBD_ATTRIBUTE_NONNULL (1, 3)
+ * => __attribute__ ((nonnull (1, 3)))
*)
let nns : bool list [ true ] (* struct nbd_handle * *)
@@ -267,7 +267,7 @@ let
let nns = List.mapi (fun i b -> (i+1, b)) nns in
let nns = filter_map (fun (i, b) -> if b then Some i else None) nns in
let nns : string list = List.map string_of_int nns in
- pr "\n LIBNBD_ATTRIBUTE_NONNULL(%s);\n" (String.concat
"," nns)
+ pr "\n LIBNBD_ATTRIBUTE_NONNULL (%s);\n" (String.concat ",
" nns)
let rec print_cbarg_list ?(wrap = false) ?maxcol ?types ?(parens = true)
cbargs @@ -400,22 +400,22 @@ let
pr "extern \"C\" {\n";
pr "#endif\n";
pr "\n";
- pr "#if defined(__GNUC__)\n";
+ pr "#if defined (__GNUC__)\n";
pr "#define LIBNBD_GCC_VERSION \\\n";
pr " (__GNUC__ * 10000 + __GNUC_MINOR__ * 100 +
__GNUC_PATCHLEVEL__)\n";
pr "#endif\n";
pr "\n";
pr "#ifndef LIBNBD_ATTRIBUTE_NONNULL\n";
- pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
- pr "#define LIBNBD_ATTRIBUTE_NONNULL(...)
__attribute__((__nonnull__(__VA_ARGS__)))\n";
+ pr "#if defined (__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
+ pr "#define LIBNBD_ATTRIBUTE_NONNULL(...) __attribute__ ((__nonnull__
(__VA_ARGS__)))\n";
pr "#else\n";
pr "#define LIBNBD_ATTRIBUTE_NONNULL(...)\n";
pr "#endif\n";
pr "#endif /* ! defined LIBNBD_ATTRIBUTE_NONNULL */\n";
pr "\n";
- pr "#if defined(__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
+ pr "#if defined (__GNUC__) && LIBNBD_GCC_VERSION >= 120000 /*
gcc >= 12.0 */\n";
pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn) \\\n";
- pr " __attribute__((__malloc__, __malloc__(fn, 1)))\n";
+ pr " __attribute__ ((__malloc__, __malloc__ (fn, 1)))\n";
pr "#else\n";
pr "#define LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(fn)\n";
pr "#endif\n";
@@ -454,7 +454,7 @@ let
pr "#define LIBNBD_HAVE_NBD_CLOSE 1\n";
pr "\n";
pr "extern struct nbd_handle *nbd_create (void)\n";
- pr " LIBNBD_ATTRIBUTE_ALLOC_DEALLOC(nbd_close);\n";
+ pr " LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (nbd_close);\n";
pr "#define LIBNBD_HAVE_NBD_CREATE 1\n";
pr "\n";
pr "extern const char *nbd_get_error (void);\n";
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 03/29] socket activation: rename sa_(tmpdir|sockpath) to sact_(tmpdir|sockpath)
<signal.h> in POSIX reserves the "sa_" prefix:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02
Let's use "sact_" instead.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
lib/internal.h | 4 ++--
generator/states-connect-socket-activation.c | 16 ++++++++--------
lib/handle.c | 12 ++++++------
3 files changed, 16 insertions(+), 16 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h
index c5827f4f0037..0b5f793011b8 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -278,8 +278,8 @@ struct nbd_handle {
/* When using systemd socket activation, this directory and socket
* must be deleted, and the pid above must be killed.
*/
- char *sa_tmpdir;
- char *sa_sockpath;
+ char *sact_tmpdir;
+ char *sact_sockpath;
/* When connecting to TCP ports, these fields are used. */
char *hostname, *port;
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 9a83834915db..7138e7638e30 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -111,22 +111,22 @@ CONNECT_SA.START:
* short enough to store in the sockaddr_un. On some platforms this
* may cause problems so we may need to revisit it. XXX
*/
- h->sa_tmpdir = strdup ("/tmp/libnbdXXXXXX");
- if (h->sa_tmpdir == NULL) {
+ h->sact_tmpdir = strdup ("/tmp/libnbdXXXXXX");
+ if (h->sact_tmpdir == NULL) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "strdup");
return 0;
}
- if (mkdtemp (h->sa_tmpdir) == NULL) {
+ if (mkdtemp (h->sact_tmpdir) == NULL) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "mkdtemp");
/* Avoid cleanup in nbd_close. */
- free (h->sa_tmpdir);
- h->sa_tmpdir = NULL;
+ free (h->sact_tmpdir);
+ h->sact_tmpdir = NULL;
return 0;
}
- if (asprintf (&h->sa_sockpath, "%s/sock", h->sa_tmpdir)
== -1) {
+ if (asprintf (&h->sact_sockpath, "%s/sock",
h->sact_tmpdir) == -1) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "strdup");
return 0;
@@ -140,10 +140,10 @@ CONNECT_SA.START:
}
addr.sun_family = AF_UNIX;
- memcpy (addr.sun_path, h->sa_sockpath, strlen (h->sa_sockpath) + 1);
+ memcpy (addr.sun_path, h->sact_sockpath, strlen (h->sact_sockpath) +
1);
if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
SET_NEXT_STATE (%.DEAD);
- set_error (errno, "bind: %s", h->sa_sockpath);
+ set_error (errno, "bind: %s", h->sact_sockpath);
close (s);
return 0;
}
diff --git a/lib/handle.c b/lib/handle.c
index 4a186f8fa946..dfd8c2e5cdb9 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -141,15 +141,15 @@ nbd_close (struct nbd_handle *h)
free_cmd_list (h->cmds_done);
string_vector_iter (&h->argv, (void *) free);
free (h->argv.ptr);
- if (h->sa_sockpath) {
+ if (h->sact_sockpath) {
if (h->pid > 0)
kill (h->pid, SIGTERM);
- unlink (h->sa_sockpath);
- free (h->sa_sockpath);
+ unlink (h->sact_sockpath);
+ free (h->sact_sockpath);
}
- if (h->sa_tmpdir) {
- rmdir (h->sa_tmpdir);
- free (h->sa_tmpdir);
+ if (h->sact_tmpdir) {
+ rmdir (h->sact_tmpdir);
+ free (h->sact_tmpdir);
}
free (h->hostname);
free (h->port);
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 04/29] ocaml: rename "sa_u" to "saddr_u"
<signal.h> in POSIX reserves the "sa_" prefix:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_02_02
Let's use "saddr_" instead.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
ocaml/helpers.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/ocaml/helpers.c b/ocaml/helpers.c
index 6568755bc664..42bcf997a237 100644
--- a/ocaml/helpers.c
+++ b/ocaml/helpers.c
@@ -140,19 +140,19 @@ nbd_internal_unix_sockaddr_to_sa (value sockaddrv,
socklen_t *len)
{
CAMLparam1 (sockaddrv);
- union sock_addr_union sa_u;
+ union sock_addr_union saddr_u;
socklen_param_type sl; /* this is really an int or socklen_t */
memset (ss, 0, sizeof *ss);
#ifdef HAVE_CAML_UNIX_GET_SOCKADDR
- caml_unix_get_sockaddr (sockaddrv, &sa_u, &sl);
+ caml_unix_get_sockaddr (sockaddrv, &saddr_u, &sl);
#else
/* OCaml <= 4.14 exports this unnamespaced symbol. */
- get_sockaddr (sockaddrv, &sa_u, &sl);
+ get_sockaddr (sockaddrv, &saddr_u, &sl);
#endif
assert (sl <= sizeof *ss);
- memcpy (ss, &sa_u, sl);
+ memcpy (ss, &saddr_u, sl);
*len = sl;
CAMLreturn0;
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 05/29] vector: (mostly) factor out DEFINE_VECTOR_EMPTY
The "name##_iter" function is used 11 times in libnbd; in all those
cases,
"name" is "string_vector", and the "f" callback is
"free":
string_vector_iter (..., (void *) free);
Casting "free" to (void*) is ugly. (Well-defined by POSIX, but still.)
Furthermore, in all 11 cases, the freeing of the vector's strings is
immediately followed by the release of the vector's array-of-pointers too.
(This additional step is not expressed consistently across libnbd: it's
sometimes spelled as free(vec.ptr), sometimes as
string_vector_reset(&vec).)
Remove the generic "name##_iter" function definition, and introduce
"name##_empty", which performs both steps at the same time. Convert
the
call sites. (Note that the converted code performs more cleanup steps in
some cases than strictly necessary, but the extra work is harmless, and
arguably beneficial for clarity / consistency.)
Expose the "name##_empty" function definition with a new, separate
macro:
DEFINE_VECTOR_EMPTY(). The existent DEFINE_VECTOR_TYPE() macro permits
such element types that are not pointers, or are pointers to const- and/or
volatile-qualified objects. Whereas "name##_empty" requires that the
elements be pointers to dynamically allocated, non-const, non-volatile
objects.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
common/utils/string-vector.h | 1 +
common/utils/vector.h | 27 +++++++++++++-------
generator/states-connect-socket-activation.c | 9 +++----
lib/handle.c | 12 +++------
lib/utils.c | 6 ++---
common/utils/test-vector.c | 3 +--
info/show.c | 3 +--
7 files changed, 30 insertions(+), 31 deletions(-)
diff --git a/common/utils/string-vector.h b/common/utils/string-vector.h
index aa33fd48ceb5..78d0b4f9bf10 100644
--- a/common/utils/string-vector.h
+++ b/common/utils/string-vector.h
@@ -38,5 +38,6 @@
#include "vector.h"
DEFINE_VECTOR_TYPE (string_vector, char *);
+DEFINE_VECTOR_EMPTY (string_vector);
#endif /* STRING_VECTOR_H */
diff --git a/common/utils/vector.h b/common/utils/vector.h
index fb2482c853ff..14bf5b0ddbc0 100644
--- a/common/utils/vector.h
+++ b/common/utils/vector.h
@@ -150,15 +150,6 @@
v->len = v->cap = 0; \
} \
\
- /* Iterate over the vector, calling f() on each element. */ \
- static inline void __attribute__ ((__unused__)) \
- name##_iter (name *v, void (*f) (type elem)) \
- { \
- size_t i; \
- for (i = 0; i < v->len; ++i) \
- f (v->ptr[i]); \
- } \
- \
/* Sort the elements of the vector. */ \
static inline void __attribute__ ((__unused__)) \
name##_sort (name *v, \
@@ -180,6 +171,24 @@
#define empty_vector { .ptr = NULL, .len = 0, .cap = 0 }
+/* This macro should only be used if:
+ * - the vector contains pointers, and
+ * - the pointed-to objects are:
+ * - neither const- nor volatile-qualified, and
+ * - allocated with malloc() or equivalent.
+ */
+#define DEFINE_VECTOR_EMPTY(name) \
+ /* Call free() on each element of the vector, then reset the vector. \
+ */ \
+ static inline void __attribute__ ((__unused__)) \
+ name##_empty (name *v) \
+ { \
+ size_t i; \
+ for (i = 0; i < v->len; ++i) \
+ free (v->ptr[i]); \
+ name##_reset (v); \
+ }
+
struct generic_vector {
void *ptr;
size_t len;
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 7138e7638e30..3b621b8be44f 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -91,8 +91,7 @@ prepare_socket_activation_environment (string_vector *env)
err:
set_error (errno, "malloc");
- string_vector_iter (env, (void *) free);
- free (env->ptr);
+ string_vector_empty (env);
return -1;
}
@@ -166,8 +165,7 @@ CONNECT_SA.START:
SET_NEXT_STATE (%.DEAD);
set_error (errno, "fork");
close (s);
- string_vector_iter (&env, (void *) free);
- free (env.ptr);
+ string_vector_empty (&env);
return 0;
}
if (pid == 0) { /* child - run command */
@@ -210,8 +208,7 @@ CONNECT_SA.START:
/* Parent. */
close (s);
- string_vector_iter (&env, (void *) free);
- free (env.ptr);
+ string_vector_empty (&env);
h->pid = pid;
h->connaddrlen = sizeof addr;
diff --git a/lib/handle.c b/lib/handle.c
index dfd8c2e5cdb9..fb92f16e6c91 100644
--- a/lib/handle.c
+++ b/lib/handle.c
@@ -128,8 +128,7 @@ nbd_close (struct nbd_handle *h)
/* Free user callbacks first. */
nbd_unlocked_clear_debug_callback (h);
- string_vector_iter (&h->querylist, (void *) free);
- free (h->querylist.ptr);
+ string_vector_empty (&h->querylist);
free (h->bs_entries);
nbd_internal_reset_size_and_flags (h);
for (i = 0; i < h->meta_contexts.len; ++i)
@@ -139,8 +138,7 @@ nbd_close (struct nbd_handle *h)
free_cmd_list (h->cmds_to_issue);
free_cmd_list (h->cmds_in_flight);
free_cmd_list (h->cmds_done);
- string_vector_iter (&h->argv, (void *) free);
- free (h->argv.ptr);
+ string_vector_empty (&h->argv);
if (h->sact_sockpath) {
if (h->pid > 0)
kill (h->pid, SIGTERM);
@@ -164,8 +162,7 @@ nbd_close (struct nbd_handle *h)
free (h->tls_certificates);
free (h->tls_username);
free (h->tls_psk_file);
- string_vector_iter (&h->request_meta_contexts, (void *) free);
- free (h->request_meta_contexts.ptr);
+ string_vector_empty (&h->request_meta_contexts);
free (h->hname);
pthread_mutex_destroy (&h->lock);
free (h);
@@ -379,8 +376,7 @@ nbd_unlocked_get_meta_context (struct nbd_handle *h, size_t
i)
int
nbd_unlocked_clear_meta_contexts (struct nbd_handle *h)
{
- string_vector_iter (&h->request_meta_contexts, (void *) free);
- string_vector_reset (&h->request_meta_contexts);
+ string_vector_empty (&h->request_meta_contexts);
return 0;
}
diff --git a/lib/utils.c b/lib/utils.c
index c229ebfc6106..bba4b3846e77 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -93,8 +93,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv)
return -1;
}
- string_vector_iter (&h->argv, (void *) free);
- string_vector_reset (&h->argv);
+ string_vector_empty (&h->argv);
if (nbd_internal_copy_string_list (&h->argv, argv) == -1) {
set_error (errno, "realloc");
@@ -110,8 +109,7 @@ nbd_internal_set_argv (struct nbd_handle *h, char **argv)
int
nbd_internal_set_querylist (struct nbd_handle *h, char **queries)
{
- string_vector_iter (&h->querylist, (void *) free);
- string_vector_reset (&h->querylist);
+ string_vector_empty (&h->querylist);
if (queries) {
if (nbd_internal_copy_string_list (&h->querylist, queries) == -1) {
diff --git a/common/utils/test-vector.c b/common/utils/test-vector.c
index 55c8ebeb8a1e..05ac5cec3dba 100644
--- a/common/utils/test-vector.c
+++ b/common/utils/test-vector.c
@@ -151,8 +151,7 @@ test_string_vector (void)
printf ("%s\n", v.ptr[i]);
assert (i == 10);
- string_vector_iter (&v, (void*)free);
- string_vector_reset (&v);
+ string_vector_empty (&v);
}
static void
diff --git a/info/show.c b/info/show.c
index 4bf596715cf9..e6c9b9bf1243 100644
--- a/info/show.c
+++ b/info/show.c
@@ -275,8 +275,7 @@ show_one_export (struct nbd_handle *nbd, const char *desc,
fprintf (fp, "\t},\n");
}
- string_vector_iter (&contexts, (void *) free);
- free (contexts.ptr);
+ string_vector_empty (&contexts);
free (content);
free (export_name);
free (export_desc);
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 06/29] lib/utils: introduce xwrite() as a more robust write()
While nbd_internal_fork_safe_perror() must indeed call write(), and
arguably justifiedly ignores the return value of write(), we can still
make the write operations slightly more robust. Let's do that by
introducing xwrite():
- don't call write() with nbyte > SSIZE_MAX, for avoiding
implementation-defined behavior,
- handle partial writes,
- cope with EINTR and EAGAIN errors. (A busy loop around EAGAIN on a
non-blocking file is not great in the general case, but it's good enough
for writing out diagnostics before giving up.)
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
lib/utils.c | 39 ++++++++++++++++++--
1 file changed, 35 insertions(+), 4 deletions(-)
diff --git a/lib/utils.c b/lib/utils.c
index bba4b3846e77..7fb16f6402d1 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -25,6 +25,7 @@
#include <ctype.h>
#include <errno.h>
#include <fcntl.h>
+#include <limits.h>
#include "minmax.h"
@@ -181,6 +182,36 @@ nbd_internal_fork_safe_itoa (long v, char *buf, size_t
bufsize)
#pragma GCC diagnostic ignored "-Wunused-result"
#endif
+/* "Best effort" function for writing out a buffer to a file
descriptor.
+ * Chunking with SSIZE_MAX (for avoiding implementation-defined behavior),
+ * partial writes, and EINTR and EAGAIN failures are handled internally. No
+ * value is returned; only use this for writing diagnostic data on error paths,
+ * when giving up on a higher-level action anyway. Note that this function is
+ * supposed to remain async-signal-safe.
+ */
+static void
+xwrite (int fd, const void *buf, size_t count)
+{
+ const unsigned char *pos;
+ size_t left;
+
+ pos = buf;
+ left = count;
+ while (left > 0) {
+ ssize_t written;
+
+ do
+ written = write (fd, pos, MIN (left, SSIZE_MAX));
+ while (written == -1 && (errno == EINTR || errno == EAGAIN));
+
+ if (written == -1)
+ return;
+
+ pos += written;
+ left -= written;
+ };
+}
+
/* Fork-safe version of perror. ONLY use this after fork and before
* exec, the rest of the time use set_error().
*/
@@ -191,8 +222,8 @@ nbd_internal_fork_safe_perror (const char *s)
const char *m = NULL;
char buf[32];
- write (2, s, strlen (s));
- write (2, ": ", 2);
+ xwrite (2, s, strlen (s));
+ xwrite (2, ": ", 2);
#ifdef HAVE_STRERRORDESC_NP
m = strerrordesc_np (errno);
#else
@@ -202,8 +233,8 @@ nbd_internal_fork_safe_perror (const char *s)
#endif
if (!m)
m = nbd_internal_fork_safe_itoa ((long) errno, buf, sizeof buf);
- write (2, m, strlen (m));
- write (2, "\n", 1);
+ xwrite (2, m, strlen (m));
+ xwrite (2, "\n", 1);
/* Restore original errno in case it was disturbed by the system
* calls above.
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 07/29] lib/utils: add async-signal-safe assert()
Add an assert() variant that we may call between fork() and exec*().
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-U13
lib/internal.h | 13 ++++++++++++
lib/utils.c | 22 ++++++++++++++++++++
2 files changed, 35 insertions(+)
diff --git a/lib/internal.h b/lib/internal.h
index 0b5f793011b8..9c9021ed366e 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -517,14 +517,27 @@ extern char *nbd_internal_printable_string (const char
*str)
extern char *nbd_internal_printable_string_list (char **list)
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
* according to the nonblock parameter.
*/
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);
+extern void nbd_internal_fork_safe_assert (int result, const char *file,
+ long line, const char *func,
+ const char *assertion)
+ LIBNBD_ATTRIBUTE_NONNULL (2, 4, 5);
+
+#ifdef NDEBUG
+#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) ((void)0)
+#else
+#define NBD_INTERNAL_FORK_SAFE_ASSERT(expression) \
+ (nbd_internal_fork_safe_assert ((expression) != 0, __FILE__, __LINE__, \
+ __func__, #expression))
+#endif
+
#endif /* LIBNBD_INTERNAL_H */
diff --git a/lib/utils.c b/lib/utils.c
index 7fb16f6402d1..5dd00f82e073 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -431,13 +431,35 @@ nbd_internal_socketpair (int domain, int type, int
protocol, int *fds)
if (ret == 0) {
for (i = 0; i < 2; i++) {
if (fcntl (fds[i], F_SETFD, FD_CLOEXEC) == -1) {
close (fds[0]);
close (fds[1]);
return -1;
}
}
}
#endif
return ret;
}
+
+void
+nbd_internal_fork_safe_assert (int result, const char *file, long line,
+ const char *func, const char *assertion)
+{
+ const char *line_out;
+ char line_buf[32];
+
+ if (result)
+ return;
+
+ xwrite (STDERR_FILENO, file, strlen (file));
+ xwrite (STDERR_FILENO, ":", 1);
+ line_out = nbd_internal_fork_safe_itoa (line, line_buf, sizeof line_buf);
+ xwrite (STDERR_FILENO, line_out, strlen (line_out));
+ xwrite (STDERR_FILENO, ": ", 2);
+ xwrite (STDERR_FILENO, func, strlen (func));
+ xwrite (STDERR_FILENO, ": Assertion `", 13);
+ xwrite (STDERR_FILENO, assertion, strlen (assertion));
+ xwrite (STDERR_FILENO, "' failed.\n", 10);
+ abort ();
+}
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 08/29] lib/utils: add unit test for async-signal-safe assert()
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 at redhat.com>
---
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 at
lists.fedoraproject.org/thread/WJHGG2OO7ABNAYICGA5WQZ2Q34Q2FEHU/
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
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 09/29] lib/utils: introduce async-signal-safe execvpe()
execvp() [01] is powerful:
- it performs PATH search [02] if necessary,
- it falls back to executing the file with the shell as a shell script in
case the underlying execv() or execve() fails with ENOEXEC.
However, execvp() is not async-signal-safe [03], and so we shouldn't call
it in a child process forked from a multi-threaded parent process [04],
which the libnbd client application may well be.
Introduce three APIs for safely emulating execvp() with execve():
- nbd_internal_execvpe_init(): to be called in the parent process, before
fork(). Allocate and format candidate pathnames for execution with
execve(), based on PATH. Allocate a buffer for the longer command line
that the shell script fallback needs.
- nbd_internal_fork_safe_execvpe(): to be called in the child process. Try
the candidates formatted previously in sequence with execve(), as long
as execve() fails with errors related to pathname resolution /
inode-level file type / execution permission. Stop iterating on fatal
errors; if the fatal error is ENOEXEC, activate the shell script
fallback. As a small added feature, take the environment from an
explicit parameter (effectively imitating the GNU-specific execvpe()
function -- cf. commit dc64ac5cdd0b, "states: Use execvp instead of
execvpe", 2019-11-15).
- nbd_internal_execvpe_uninit(): to be called in the parent process, after
fork(). Release the resources allocated with
nbd_internal_execvpe_init().
The main idea with the above distribution of responsibilities is that any
pre-scanning of PATH -- which was suggested by Eric Blake -- should not
include activities that are performed by execve() anyway. I've considered
and abandoned two approaches:
- During scanning (i.e., in nbd_internal_execvpe_init()), check each
candidate with access(X_OK) [05].
I dropped this because the informative APPLICATION USAGE section of the
same specification [06] advises against it, saying "[a]n application
should instead attempt the action itself and handle the [EACCES] error
that occurs if the file is not accessible".
- During scanning, open each candidate with O_EXEC [07] until one open()
succeeds. Save the sole file descriptor for
nbd_internal_fork_safe_execvpe(). In the latter, call fexecve() [08],
which is free of all error conditions that necessitate iteration over
candidates. Still implement the ENOEXEC (shell script) fallback.
I dropped this because (a) fexecve() is a quite recent interface
(highlighted by Eric); (b) for an application to open a file for
execution with fexecve(), it's more robust to pass O_EXEC to open() than
not to pass it, per APPLICATION USAGE [09], but on Linux/glibc, O_EXEC
does not seem supported, only O_PATH does [10].
Thus the chosen approach -- pre-generate filenames -- contains a small
TOCTTOU race (highlighted by Eric) after all, but it should be harmless.
Implementation-defined details:
- If PATH searching is necessary, but PATH is absent [01], then fall back
to confstr(_CS_PATH) [11], similarly to Linux/glibc execvpe() [12].
If PATH is set but empty ("set to null") [02], or PATH is unset and
confstr(_CS_PATH) fails or returns no information or returns the empty
string, we fail the initial scanning (!) with ENOENT. This is consistent
with bash's behavior on Linux/glibc:
$ PATH= ls
bash: ls: No such file or directory
Details chosen for unspecified behavior:
- Hardwire "/bin/sh" as the shell's pathname [01].
[01] https://pubs.opengroup.org/onlinepubs/9699919799/functions/execvp.html
[02]
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap08.html#tag_08_03
[03]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/V2_chap02.html#tag_15_04_03_03
[04] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fork.html
[05] https://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html
[06]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/access.html#tag_16_09_07
[07] https://pubs.opengroup.org/onlinepubs/9699919799/functions/open.html
[08] https://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html
[09]
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fexecve.html#tag_16_111_07
[10] https://man7.org/linux/man-pages/man2/open.2.html
[11] https://pubs.opengroup.org/onlinepubs/9699919799/functions/confstr.html
[12] https://man7.org/linux/man-pages/man3/execvpe.3.html
Suggested-by: Eric Blake <eblake at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
lib/internal.h | 22 ++
lib/utils.c | 351 ++++++++++++++++++++
2 files changed, 373 insertions(+)
diff --git a/lib/internal.h b/lib/internal.h
index 9c9021ed366e..8c4e32013e40 100644
--- a/lib/internal.h
+++ b/lib/internal.h
@@ -368,6 +368,18 @@ struct command {
uint32_t error; /* Local errno value */
};
+struct execvpe {
+ string_vector pathnames;
+
+ /* Note: "const_string_vector" is not a good type for
"sh_argv" below. Even if
+ * we reserved enough space in a "const_string_vector",
+ * const_string_vector_append() would still not be async-signal-safe, due to
+ * the underlying const_string_vector_insert() calling assert().
+ */
+ char **sh_argv;
+ size_t num_sh_args;
+};
+
/* Test if a callback is "null" or not, and set it to null. */
#define CALLBACK_IS_NULL(cb) ((cb).callback == NULL && (cb).free ==
NULL)
#define CALLBACK_IS_NOT_NULL(cb) (! CALLBACK_IS_NULL ((cb)))
@@ -540,4 +552,14 @@ extern void nbd_internal_fork_safe_assert (int result,
const char *file,
__func__, #expression))
#endif
+extern int nbd_internal_execvpe_init (struct execvpe *ctx, const char *file,
+ size_t num_args)
+ LIBNBD_ATTRIBUTE_NONNULL (1, 2);
+extern void nbd_internal_execvpe_uninit (struct execvpe *ctx)
+ LIBNBD_ATTRIBUTE_NONNULL (1);
+extern int nbd_internal_fork_safe_execvpe (struct execvpe *ctx,
+ const string_vector *argv,
+ char * const *envp)
+ LIBNBD_ATTRIBUTE_NONNULL (1, 2, 3);
+
#endif /* LIBNBD_INTERNAL_H */
diff --git a/lib/utils.c b/lib/utils.c
index 5dd00f82e073..9a96ed6ed674 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -28,6 +28,7 @@
#include <limits.h>
#include "minmax.h"
+#include "checked-overflow.h"
#include "internal.h"
@@ -463,3 +464,353 @@ nbd_internal_fork_safe_assert (int result, const char
*file, long line,
xwrite (STDERR_FILENO, "' failed.\n", 10);
abort ();
}
+
+/* Returns the value of the PATH environment variable -- falling back to
+ * confstr(_CS_PATH) if PATH is absent -- as a dynamically allocated string. On
+ * failure, sets "errno" and returns NULL.
+ */
+static char *
+get_path (void)
+ LIBNBD_ATTRIBUTE_ALLOC_DEALLOC (free)
+{
+ char *path;
+ bool env_path_found;
+ size_t path_size, path_size2;
+
+ /* FIXME: lock the environment. */
+ path = getenv ("PATH");
+ if ((env_path_found = (path != NULL)))
+ path = strdup (path);
+ /* FIXME: unlock the environment. */
+
+ if (env_path_found) {
+ /* This handles out-of-memory as well. */
+ return path;
+ }
+
+ errno = 0;
+ path_size = confstr (_CS_PATH, NULL, 0);
+ if (path_size == 0) {
+ /* If _CS_PATH does not have a configuration-defined value, just store
+ * ENOENT to "errno".
+ */
+ if (errno == 0)
+ errno = ENOENT;
+
+ return NULL;
+ }
+
+ path = malloc (path_size);
+ if (path == NULL)
+ return NULL;
+
+ path_size2 = confstr (_CS_PATH, path, path_size);
+ assert (path_size2 == path_size);
+ return path;
+}
+
+/* nbd_internal_execvpe_init() and nbd_internal_fork_safe_execvpe() together
+ * present an execvp() alternative that is async-signal-safe.
+ *
+ * nbd_internal_execvpe_init() may only be called before fork(), for filling in
+ * the caller-allocated, uninitialized "ctx" structure. If
+ * nbd_internal_execvpe_init() succeeds, then fork() may be called.
+ * Subsequently, in the child process, nbd_internal_fork_safe_execvpe() may be
+ * called with the inherited "ctx" structure, while in the parent
process,
+ * nbd_internal_execvpe_uninit() must be called to uninitialize (evacuate) the
+ * "ctx" structure.
+ *
+ * On failure, "ctx" will not have been modified, "errno"
is set, and -1 is
+ * returned. Failures include:
+ *
+ * - Errors forwarded from underlying functions such as strdup(), confstr(),
+ * malloc(), string_vector_append().
+ *
+ * - ENOENT: "file" is an empty string.
+ *
+ * - ENOENT: "file" does not contain a <slash> "/"
character, the PATH
+ * environment variable is not set, and confstr() doesn't
associate a
+ * configuration-defined value with _CS_PATH.
+ *
+ * - ENOENT: "file" does not contain a <slash> "/"
character, and: (a) the PATH
+ * environment variable is set to the empty string, or (b) PATH is
not
+ * set, and confstr() outputs the empty string for _CS_PATH.
+ *
+ * - EOVERFLOW: the sizes or counts of necessary objects could not be
expressed.
+ *
+ * - EINVAL: "num_args" is less than 2.
+ *
+ * On success, the "ctx" structure will have been filled in, and 0 is
returned.
+ *
+ * - "pathnames" member:
+ *
+ * - All strings pointed-to by elements of the "pathnames"
string_vector
+ * member are owned by "pathnames".
+ *
+ * - If "file" contains a <slash> "/" character,
then the sole entry in
+ * "pathnames" is a copy of "file".
+ *
+ * - If "file" does not contain a <slash> "/"
character:
+ *
+ * Let "system path" be defined as the value of the PATH
environment
+ * variable, if the latter exists, and as the value output by confstr() for
+ * _CS_PATH otherwise. Per the ENOENT specifications above, "system
path" is
+ * a non-empty string. Let "system path" further be of the form
+ *
+ * <prefix_0> [n = 1]
+ *
+ * or
+ *
+ * <prefix_0>:<prefix_1>:...:<prefix_(n-1)> [n >= 2]
+ *
+ * where for each 0 <= i < n, <prefix_i> does not contain the
<colon> ":"
+ * character. In the (n = 1) case, <prefix_0> is never empty (see
ENOENT
+ * above), while in the (n >= 2) case, any individual <prefix_i>
may or may
+ * not be empty.
+ *
+ * The "pathnames" string_vector member has n elements; for each
0 <= i < n,
+ * element i is of the form
+ *
+ * suffix(curdir(<prefix_i>))file
+ *
+ * where
+ *
+ * curdir(x) := "." if x = ""
+ * curdir(x) := x otherwise
+ *
+ * and
+ *
+ * suffix(x) := x if "x" ends with a <slash>
"/"
+ * suffix(x) := x "/" otherwise
+ *
+ * This transformation implements the POSIX XBD / PATH environment variable
+ * semantics, creating candidate pathnames for execution by
+ * nbd_internal_fork_safe_execvpe(). nbd_internal_fork_safe_execvpe() will
+ * iterate over the candidate pathnames with execve() until execve()
+ * succeeds, or fails with an error that is due to neither pathname
+ * resolution, nor the candidate not being a regular file, nor the
candidate
+ * lacking execution permission.
+ *
+ * - The "sh_argv" array member will have at least (num_args + 1)
elements
+ * allocated, and none populated.
+ *
+ * (The minimum value of "num_args" is 2 -- see EINVAL above.
According to
+ * POSIX, "[t]he argument /arg0/ should point to a filename string that
is
+ * associated with the process being started by one of the /exec/
functions",
+ * plus "num_args" includes the null pointer that terminates the
argument
+ * list.)
+ *
+ * This allocation is made in anticipation of execve() failing for a
+ * particular candidate inside nbd_internal_fork_safe_execvpe() with ENOEXEC
+ * ("[t]he new process image file has the appropriate access permission
but
+ * has an unrecognized format"). While that failure terminates the
iteration,
+ * the failed call
+ *
+ * execve (pathnames[i],
+ * { argv[0], argv[1], ..., NULL }, // (num_args >= 2) elements
+ * { envp[0], envp[1], ..., NULL })
+ *
+ * must be repeated as
+ *
+ * execve (<shell-path>,
+ * { argv[0], pathnames[i], // ((num_args + 1) >= 3)
+ argv[1], ..., NULL }, // elements
+ * { envp[0], envp[1], ..., NULL })
+ *
+ * for emulating execvp(). The allocation in the "sh_argv" member
makes it
+ * possible just to *link* the original "argv" elements and the
"pathnames[i]"
+ * candidate into the right positions.
+ *
+ * (POSIX leaves the shell pathname unspecified; "/bin/sh" should
be good
+ * enough.)
+ *
+ * The shell *binary* will see itself being executed under the name
"argv[0]",
+ * will receive "pathnames[i]" as the pathname of the shell
*script* to read
+ * and interpret ("command_file" in POSIX terminology), will expose
+ * "pathnames[i]" as the positional parameter $0 to the script, and
will
+ * forward "argv[1]" and the rest to the script as positional
parameters $1
+ * and onward.
+ */
+int
+nbd_internal_execvpe_init (struct execvpe *ctx, const char *file,
+ size_t num_args)
+{
+ int rc;
+ char *sys_path;
+ string_vector pathnames;
+ char *pathname;
+ size_t num_sh_args;
+ char **sh_argv;
+ size_t sh_argv_bytes;
+
+ rc = -1;
+
+ if (file[0] == '\0') {
+ errno = ENOENT;
+ return rc;
+ }
+
+ /* First phase. */
+ sys_path = NULL;
+ pathnames = (string_vector)empty_vector;
+
+ if (strchr (file, '/') == NULL) {
+ size_t file_len;
+ const char *sys_path_element, *scan;
+ bool finish;
+
+ sys_path = get_path ();
+ if (sys_path == NULL)
+ return rc;
+
+ if (sys_path[0] == '\0') {
+ errno = ENOENT;
+ goto free_sys_path;
+ }
+
+ pathname = NULL;
+ file_len = strlen (file);
+ sys_path_element = sys_path;
+ scan = sys_path;
+ do {
+ assert (sys_path_element <= scan);
+ finish = (*scan == '\0');
+ if (finish || *scan == ':') {
+ const char *sys_path_copy_start;
+ size_t sys_path_copy_size;
+ size_t sep_copy_size;
+ size_t pathname_size;
+ char *p;
+
+ if (scan == sys_path_element) {
+ sys_path_copy_start = ".";
+ sys_path_copy_size = 1;
+ } else {
+ sys_path_copy_start = sys_path_element;
+ sys_path_copy_size = scan - sys_path_element;
+ }
+
+ assert (sys_path_copy_size >= 1);
+ sep_copy_size = (sys_path_copy_start[sys_path_copy_size - 1] !=
'/');
+
+ if (ADD_OVERFLOW (sys_path_copy_size, sep_copy_size,
&pathname_size) ||
+ ADD_OVERFLOW (pathname_size, file_len, &pathname_size) ||
+ ADD_OVERFLOW (pathname_size, 1u, &pathname_size)) {
+ errno = EOVERFLOW;
+ goto empty_pathnames;
+ }
+
+ pathname = malloc (pathname_size);
+ if (pathname == NULL)
+ goto empty_pathnames;
+ p = pathname;
+
+ memcpy (p, sys_path_copy_start, sys_path_copy_size);
+ p += sys_path_copy_size;
+
+ memcpy (p, "/", sep_copy_size);
+ p += sep_copy_size;
+
+ memcpy (p, file, file_len);
+ p += file_len;
+
+ *p++ = '\0';
+
+ if (string_vector_append (&pathnames, pathname) == -1)
+ goto empty_pathnames;
+ /* Ownership transferred. */
+ pathname = NULL;
+
+ sys_path_element = scan + 1;
+ }
+
+ ++scan;
+ } while (!finish);
+ } else {
+ pathname = strdup (file);
+ if (pathname == NULL)
+ return rc;
+
+ if (string_vector_append (&pathnames, pathname) == -1)
+ goto empty_pathnames;
+ /* Ownership transferred. */
+ pathname = NULL;
+ }
+
+ /* Second phase. */
+ if (num_args < 2) {
+ errno = EINVAL;
+ goto empty_pathnames;
+ }
+ if (ADD_OVERFLOW (num_args, 1u, &num_sh_args) ||
+ MUL_OVERFLOW (num_sh_args, sizeof *sh_argv, &sh_argv_bytes)) {
+ errno = EOVERFLOW;
+ goto empty_pathnames;
+ }
+ sh_argv = malloc (sh_argv_bytes);
+ if (sh_argv == NULL)
+ goto empty_pathnames;
+
+ /* Commit. */
+ ctx->pathnames = pathnames;
+ ctx->sh_argv = sh_argv;
+ ctx->num_sh_args = num_sh_args;
+ rc = 0;
+ /* Fall through, for freeing temporaries. */
+
+empty_pathnames:
+ if (rc == -1) {
+ free (pathname);
+ string_vector_empty (&pathnames);
+ }
+
+free_sys_path:
+ free (sys_path);
+
+ return rc;
+}
+
+void
+nbd_internal_execvpe_uninit (struct execvpe *ctx)
+{
+ free (ctx->sh_argv);
+ ctx->num_sh_args = 0;
+ string_vector_empty (&ctx->pathnames);
+}
+
+int
+nbd_internal_fork_safe_execvpe (struct execvpe *ctx, const string_vector *argv,
+ char * const *envp)
+{
+ size_t pathname_idx;
+
+ NBD_INTERNAL_FORK_SAFE_ASSERT (ctx->pathnames.len > 0);
+
+ pathname_idx = 0;
+ do {
+ (void)execve (ctx->pathnames.ptr[pathname_idx], argv->ptr, envp);
+ if (errno != EACCES && errno != ELOOP && errno !=
ENAMETOOLONG &&
+ errno != ENOENT && errno != ENOTDIR)
+ break;
+
+ ++pathname_idx;
+ } while (pathname_idx < ctx->pathnames.len);
+
+ if (errno == ENOEXEC) {
+ char **sh_argp;
+ size_t argv_idx;
+
+ NBD_INTERNAL_FORK_SAFE_ASSERT (ctx->num_sh_args >= argv->len);
+ NBD_INTERNAL_FORK_SAFE_ASSERT (ctx->num_sh_args - argv->len == 1);
+
+ sh_argp = ctx->sh_argv;
+ *sh_argp++ = argv->ptr[0];
+ *sh_argp++ = ctx->pathnames.ptr[pathname_idx];
+ for (argv_idx = 1; argv_idx < argv->len; ++argv_idx)
+ *sh_argp++ = argv->ptr[argv_idx];
+
+ (void)execve ("/bin/sh", ctx->sh_argv, envp);
+ }
+
+ return -1;
+}
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 10/29] lib/utils: add unit tests for async-signal-safe execvpe()
Don't try to test async-signal-safety, but strive to exercise as many as
possible paths through nbd_internal_execvpe_init() and
nbd_internal_fork_safe_execvpe().
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
lib/test-fork-safe-execvpe.c | 117 +++++++++
lib/Makefile.am | 10 +
lib/test-fork-safe-execvpe.sh | 270 ++++++++++++++++++++
.gitignore | 1 +
4 files changed, 398 insertions(+)
diff --git a/lib/test-fork-safe-execvpe.c b/lib/test-fork-safe-execvpe.c
new file mode 100644
index 000000000000..09b91102b613
--- /dev/null
+++ b/lib/test-fork-safe-execvpe.c
@@ -0,0 +1,117 @@
+ /* 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 <errno.h>
+#include <stdio.h>
+#include <stdlib.h>
+#include <string.h>
+
+#include "internal.h"
+
+extern char **environ;
+
+/* This is a perror() replacement that makes the error message more
+ * machine-readable, for a select few error numbers. Do not use it as a general
+ * error() replacement, only upon nbd_internal_execvpe_init() and
+ * nbd_internal_fork_safe_execvpe() failure.
+ */
+static void
+xperror (const char *s)
+{
+ const char *err;
+
+ if (s != NULL && *s != '\0')
+ (void)fprintf (stderr, "%s: ", s);
+
+ switch (errno) {
+ case EACCES:
+ err = "EACCES";
+ break;
+ case ELOOP:
+ err = "ELOOP";
+ break;
+ case ENOENT:
+ err = "ENOENT";
+ break;
+ case ENOTDIR:
+ err = "ENOTDIR";
+ break;
+ default:
+ err = strerror (errno);
+ }
+ (void)fprintf (stderr, "%s\n", err);
+}
+
+int
+main (int argc, char **argv)
+{
+ struct execvpe ctx;
+ const char *prog_file;
+ string_vector prog_argv;
+ size_t i;
+
+ if (argc < 3) {
+ fprintf (stderr, "%1$s: usage: %1$s program-to-exec argv0 ...\n",
argv[0]);
+ return EXIT_FAILURE;
+ }
+
+ prog_file = argv[1];
+
+ /* For the argv of the program to execute, we need to drop our argv[0] (= our
+ * own name) and argv[1] (= the program we need to execute), and to tack on a
+ * terminating null pointer. Note that "argc" does not include the
terminating
+ * NULL.
+ */
+ prog_argv = (string_vector)empty_vector;
+ if (string_vector_reserve (&prog_argv, argc - 2 + 1) == -1) {
+ perror ("string_vector_reserve");
+ return EXIT_FAILURE;
+ }
+
+ for (i = 2; i < argc; ++i)
+ (void)string_vector_append (&prog_argv, argv[i]);
+ (void)string_vector_append (&prog_argv, NULL);
+
+ if (nbd_internal_execvpe_init (&ctx, prog_file, prog_argv.len) == -1) {
+ xperror ("nbd_internal_execvpe_init");
+ goto reset_prog_argv;
+ }
+
+ /* Print out the generated candidates. */
+ for (i = 0; i < ctx.pathnames.len; ++i)
+ (void)fprintf (stdout, "%s\n", ctx.pathnames.ptr[i]);
+
+ if (fflush (stdout) == EOF) {
+ perror ("fflush");
+ goto uninit_execvpe;
+ }
+
+ (void)nbd_internal_fork_safe_execvpe (&ctx, &prog_argv, environ);
+ xperror ("nbd_internal_fork_safe_execvpe");
+ /* fall through */
+
+uninit_execvpe:
+ nbd_internal_execvpe_uninit (&ctx);
+
+reset_prog_argv:
+ string_vector_reset (&prog_argv);
+
+ return EXIT_FAILURE;
+}
diff --git a/lib/Makefile.am b/lib/Makefile.am
index 1f6501ceb49a..65a2f49e4baa 100644
--- a/lib/Makefile.am
+++ b/lib/Makefile.am
@@ -33,6 +33,7 @@ EXTRA_DIST = \
$(generator_built) \
libnbd.syms \
test-fork-safe-assert.sh \
+ test-fork-safe-execvpe.sh \
$(NULL)
lib_LTLIBRARIES = libnbd.la
@@ -104,10 +105,12 @@ pkgconfig_DATA = libnbd.pc
TESTS = \
test-fork-safe-assert.sh \
+ test-fork-safe-execvpe.sh \
$(NULL)
check_PROGRAMS = \
test-fork-safe-assert \
+ test-fork-safe-execvpe \
$(NULL)
test_fork_safe_assert_SOURCES = \
@@ -116,3 +119,10 @@ test_fork_safe_assert_SOURCES = \
test-fork-safe-assert.c \
utils.c \
$(NULL)
+
+test_fork_safe_execvpe_SOURCES = \
+ $(top_srcdir)/common/utils/vector.c \
+ errors.c \
+ test-fork-safe-execvpe.c \
+ utils.c \
+ $(NULL)
diff --git a/lib/test-fork-safe-execvpe.sh b/lib/test-fork-safe-execvpe.sh
new file mode 100755
index 000000000000..ac9c48bb83c6
--- /dev/null
+++ b/lib/test-fork-safe-execvpe.sh
@@ -0,0 +1,270 @@
+#!/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
+
+# Determine the absolute pathname of the execvpe helper binary. The
"realpath"
+# utility is not in POSIX, but Linux, FreeBSD and OpenBSD all have it.
+bname=$(basename -- "$0" .sh)
+dname=$(dirname -- "$0")
+execvpe=$(realpath -- "$dname/$bname")
+
+# This is an elaborate way to control the PATH variable around the $execvpe
+# helper binary as narrowly as possible.
+#
+# If $1 is "_", then the $execvpe helper binary is invoked with PATH
unset.
+# Otherwise, the binary is invoked with PATH set to $1.
+#
+# $2 and onward are passed to $execvpe; note that $2 becomes *both*
+# "program-to-exec" for the helper *and* argv[0] for the program
executed by the
+# helper.
+#
+# The command itself (including the PATH setting) is written to "cmd"
(for error
+# reporting purposes only); the standard output and error are saved in
"out" and
+# "err" respectively; the exit status is written to
"status". This function
+# should never fail; if it does, then that's a bug in this unit test
script, or
+# the disk is full etc.
+run()
+{
+ local pathctl=$1
+ local program=$2
+ local exit_status
+
+ shift 1
+
+ if test _ = "$pathctl"; then
+ printf 'unset PATH; %s %s %s\n' "$execvpe"
"$program" "$*" >cmd
+ set +e
+ (
+ unset PATH
+ "$execvpe" "$program" "$@" >out
2>err
+ )
+ exit_status=$?
+ set -e
+ else
+ printf 'PATH=%s %s %s %s\n' "$pathctl"
"$execvpe" "$program" "$*" >cmd
+ set +e
+ PATH=$pathctl "$execvpe" "$program" "$@"
>out 2>err
+ exit_status=$?
+ set -e
+ fi
+ printf '%d\n' $exit_status >status
+}
+
+# After "run" returns, the following three functions can verify the
result.
+#
+# Check if the helper binary failed in nbd_internal_execvpe_init().
+#
+# $1 is the error number (a macro name such as ENOENT) that's expected of
+# nbd_internal_execvpe_init().
+init_fail()
+{
+ local expected_error="nbd_internal_execvpe_init: $1"
+ local cmd=$(< cmd)
+ local err=$(< err)
+ local status=$(< status)
+
+ if test 0 -eq "$status"; then
+ printf "'%s' should have failed\\n" "$cmd"
>&2
+ return 1
+ fi
+ if test x"$err" != x"$expected_error"; then
+ printf "'%s' should have failed with '%s', got
'%s'\\n" \
+ "$cmd" "$expected_error" "$err"
>&2
+ return 1
+ fi
+}
+
+# Check if the helper binary failed in nbd_internal_fork_safe_execvpe().
+#
+# $1 is the output (the list of candidate pathnames) that
+# nbd_internal_execvpe_init() is expected to produce; with inner
<newline>
+# characters replaced with <comma> characters, and the last
<newline> stripped.
+#
+# $2 is the error number (a macro name such as ENOENT) that's expected of
+# nbd_internal_fork_safe_execvpe().
+execve_fail()
+{
+ local expected_output=$1
+ local expected_error="nbd_internal_fork_safe_execvpe: $2"
+ local cmd=$(< cmd)
+ local out=$(< out)
+ local err=$(< err)
+ local status=$(< status)
+
+ if test 0 -eq "$status"; then
+ printf "'%s' should have failed\\n" "$cmd"
>&2
+ return 1
+ fi
+ if test x"$err" != x"$expected_error"; then
+ printf "'%s' should have failed with '%s', got
'%s'\\n" \
+ "$cmd" "$expected_error" "$err"
>&2
+ return 1
+ fi
+ out=${out//$'\n'/,}
+ if test x"$out" != x"$expected_output"; then
+ printf "'%s' should have output '%s', got
'%s'\\n" \
+ "$cmd" "$expected_output" "$out"
>&2
+ return 1
+ fi
+}
+
+# Check if the helper binary and the program executed by it succeeded.
+#
+# $1 is the output (the list of candidate pathnames) that
+# nbd_internal_execvpe_init() is expected to produce, followed by any output
+# expected of the program that's executed by the helper; with inner
<newline>
+# characters replaced with <comma> characters, and the last
<newline> stripped.
+success()
+{
+ local expected_output=$1
+ local cmd=$(< cmd)
+ local out=$(< out)
+ local status=$(< status)
+
+ if test 0 -ne "$status"; then
+ printf "'%s' should have succeeded\\n"
"$cmd" >&2
+ return 1
+ fi
+ out=${out//$'\n'/,}
+ if test x"$out" != x"$expected_output"; then
+ printf "'%s' should have output '%s', got
'%s'\\n" \
+ "$cmd" "$expected_output" "$out"
>&2
+ return 1
+ fi
+}
+
+# Create a temporary directory and change the working directory to it.
+tmpd=$(mktemp -d)
+trap 'rm -r -- "$tmpd"' EXIT
+cd "$tmpd"
+
+# If the "file" parameter of execvpe() is an empty string, then we
must fail --
+# in nbd_internal_execvpe_init() -- regardless of PATH.
+run _ ""; init_fail ENOENT
+run "" ""; init_fail ENOENT
+run . ""; init_fail ENOENT
+
+# Create subdirectories for triggering non-fatal internal error conditions of
+# execvpe(). (Almost) every subdirectory will contain one entry, called
"f".
+#
+# Create a directory that's empty.
+mkdir empty
+
+# Create a directory with a named pipe (FIFO) in it.
+mkdir fifo
+mkfifo fifo/f
+
+# Create a directory with a non-executable file in it.
+mkdir nxregf
+touch nxregf/f
+
+# Create a symlink loop.
+ln -s symlink symlink
+
+# Create a directory with a (most likely) binary executable in it.
+mkdir bin
+expr_pathname=$(command -p -v expr)
+cp -- "$expr_pathname" bin/f
+
+# Create a directory with an executable shell script that does not contain a
+# shebang (#!). The script will print $0 and $1, and not depend on PATH for it.
+mkdir sh
+printf "command -p printf '%%s %%s\\\\n' \"\$0\"
\"\$1\"\\n" >sh/f
+chmod u+x sh/f
+
+# In the tests below, invoke each "f" such that the "file"
parameter of
+# execvpe() contain a <slash> character.
+#
+# Therefore, PATH does not matter. Set it to the empty string. (Which in this
+# implementation would cause nbd_internal_execvpe_init() to fail with ENOENT,
if
+# the "file" parameter didn't contain a <slash>.)
+run "" empty/f; execve_fail empty/f ENOENT
+run "" fifo/f; execve_fail fifo/f EACCES
+run "" nxregf/f; execve_fail nxregf/f EACCES
+run "" nxregf/f/; execve_fail nxregf/f/ ENOTDIR
+run "" symlink/f; execve_fail symlink/f ELOOP
+
+# This runs "expr 1 + 1".
+run "" bin/f 1 + 1; success bin/f,2
+
+# This triggers the ENOEXEC branch in nbd_internal_fork_safe_execvpe().
+# nbd_internal_fork_safe_execvpe() will first try
+#
+# execve("sh/f", {"sh/f", "arg1", NULL}, envp)
+#
+# hitting ENOEXEC. Then it will successfully call
+#
+# execve("/bin/sh", {"sh/f", "sh/f",
"arg1", NULL}, envp)
+#
+# The shell script will get "sh/f" for $0 and "arg1" for
$1, and print those
+# out.
+run "" sh/f arg1; success sh/f,"sh/f arg1"
+
+# In the tests below, the "file" parameter of execvpe() will not
contain a
+# <slash> character.
+#
+# Show that PATH matters that way -- first, trigger an ENOENT in
+# nbd_internal_execvpe_init() by setting PATH to the empty string.
+run "" expr 1 + 1; init_fail ENOENT
+
+# Fall back to confstr(_CS_PATH) in nbd_internal_execvpe_init(), by unsetting
+# PATH. Verify the generated candidates by invoking "getconf PATH"
here, and
+# appending "/expr" to each prefix.
+expected_output=$(
+ path=$(command -p getconf PATH)
+ IFS=:
+ for p in $path; do
+ printf '%s/%s\n' $p expr
+ done
+ command -p expr 1 + 1
+)
+run _ expr 1 + 1; success "${expected_output//$'\n'/,}"
+
+# Continue with tests where the "file" parameter of execvpe() does
not contain a
+# <slash> character, but now set PATH to explicit prefix lists.
+#
+# Show that, if the last candidate fails execve() with an error number that
+# would not be fatal otherwise, we do get that error number.
+run empty:fifo:nxregf:symlink f
+execve_fail empty/f,fifo/f,nxregf/f,symlink/f ELOOP
+
+# Put a single prefix in PATH, such that it lead to a successful execution.
This
+# exercises two things at the same time: (a) that nbd_internal_execvpe_init()
+# produces *one* candidate (i.e., that no <colon> is seen), and (b) that
+# nbd_internal_fork_safe_execvpe() succeeds for the *last* candidate. Repeat
the
+# test with "expr" (called "f" under "bin") and
the shell script (called "f"
+# under "sh", triggering the ENOEXEC branch).
+run bin f 1 + 1; success bin/f,2
+run sh f arg1; success sh/f,"sh/f arg1"
+
+# Demonstrate that the order of candidates matters. The first invocation finds
+# "expr" (called "f" under "bin"), the second
invocation finds the shell script
+# under "sh" (triggering the ENOEXEC branch).
+run empty:bin:sh f 1 + 1; success empty/f,bin/f,sh/f,2
+run empty:sh:bin f arg1; success empty/f,sh/f,bin/f,"sh/f arg1"
+
+# Check the expansion of zero-length prefixes in PATH to ".", plus
the
+# (non-)insertion of the "/" separator.
+run a/: f; execve_fail a/f,./f ENOENT
+run :a/ f; execve_fail ./f,a/f ENOENT
+run : f; execve_fail ./f,./f ENOENT
+pushd bin
+run : f 1 + 1; success ./f,./f,2
+popd
+run :a/:::b/: f; execve_fail ./f,a/f,./f,./f,b/f,./f ENOENT
diff --git a/.gitignore b/.gitignore
index cd27a4ed7557..5c42060dc0dd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -126,6 +126,7 @@ Makefile.in
/lib/states.h
/lib/test-fork-safe-assert
/lib/test-fork-safe-assert.err
+/lib/test-fork-safe-execvpe
/lib/unlocked.h
/libtool
/ltmain.sh
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 11/29] socket activation: fix error message upon asprintf() failure
Correctly report "asprintf" in the error message when asprintf()
fails.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
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;
}
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 12/29] socket activation: clean up responsibilities of prep.sock.act.env.()
prepare_socket_activation_environment() is a construction function that is
supposed to fill in a string_vector object from the ground up. Right now
it has its responsibilities mixed up in two ways:
- it expects the caller to pass in a previously re-set string_vector,
- if it fails, it calls set_error() internally (with a blanket reference
to "malloc").
Fix both warts:
- pass in an *uninitialized* (only allocated) string vector from the
caller, and initialize it in prepare_socket_activation_environment(),
- move the set_error() call out to the caller.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
generator/states-connect-socket-activation.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index c46a0bf5c0a3..b5e146539cc8 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -51,7 +51,7 @@ prepare_socket_activation_environment (string_vector *env)
char *p;
size_t i;
- assert (env->len == 0);
+ *env = (string_vector)empty_vector;
/* Reserve slots env[0] and env[1]. */
p = strdup ("LISTEN_PID=xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx");
@@ -90,7 +90,6 @@ prepare_socket_activation_environment (string_vector *env)
return 0;
err:
- set_error (errno, "malloc");
string_vector_empty (env);
return -1;
}
@@ -99,7 +98,7 @@ STATE_MACHINE {
CONNECT_SA.START:
int s;
struct sockaddr_un addr;
- string_vector env = empty_vector;
+ string_vector env;
pid_t pid;
assert (!h->sock);
@@ -156,6 +155,7 @@ CONNECT_SA.START:
if (prepare_socket_activation_environment (&env) == -1) {
SET_NEXT_STATE (%.DEAD);
+ set_error (errno, "prepare_socket_activation_environment");
close (s);
return 0;
}
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 13/29] socket activation: avoid manipulating the sign bit
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 at redhat.com>
---
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);
}
}
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 14/29] socket activation: check syscalls for errors in the child process
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 at redhat.com>
---
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);
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 15/29] socket activation: centralize resource release
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 at redhat.com>
---
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 */
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 16/29] socket activation: plug AF_UNIX socket address (filesystem) leak on error
A prior patch highlighted the following leak: whenever any construction
step after bind() fails, the UNIX domain socket address (i.e., the
pathname in the filesystem) is leaked. Plug the leak by inserting a new
error handling section that unlinks the pathname.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-U12
generator/states-connect-socket-activation.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 164b8e6828f2..81bb850c7d8c 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -136,30 +136,30 @@ CONNECT_SA.START:
goto free_sockpath;
}
addr.sun_family = AF_UNIX;
memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
set_error (errno, "bind: %s", sockpath);
goto close_socket;
}
if (listen (s, SOMAXCONN) == -1) {
set_error (errno, "listen");
- goto close_socket;
+ goto unlink_sockpath;
}
if (prepare_socket_activation_environment (&env) == -1) {
set_error (errno, "prepare_socket_activation_environment");
- goto close_socket;
+ goto unlink_sockpath;
}
pid = fork ();
if (pid == -1) {
set_error (errno, "fork");
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");
@@ -211,24 +211,28 @@ CONNECT_SA.START:
h->sact_sockpath = sockpath;
h->pid = pid;
h->connaddrlen = sizeof addr;
memcpy (&h->connaddr, &addr, h->connaddrlen);
next = %^CONNECT.START;
/* fall through, for releasing the temporaries */
empty_env:
string_vector_empty (&env);
+unlink_sockpath:
+ if (next == %.DEAD)
+ unlink (sockpath);
+
close_socket:
close (s);
free_sockpath:
if (next == %.DEAD)
free (sockpath);
rmdir_tmpdir:
if (next == %.DEAD)
rmdir (tmpdir);
free_tmpdir:
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 17/29] socket activation: replace execvp() call with fork-safe variant
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 at redhat.com>
---
Notes:
context:-U11
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 81bb850c7d8c..cb3b1c6f4ede 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -93,22 +93,23 @@ prepare_socket_activation_environment (string_vector *env)
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;
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
@@ -140,25 +141,31 @@ CONNECT_SA.START:
memcpy (addr.sun_path, sockpath, strlen (sockpath) + 1);
if (bind (s, (struct sockaddr *) &addr, sizeof addr) == -1) {
set_error (errno, "bind: %s", sockpath);
goto close_socket;
}
if (listen (s, SOMAXCONN) == -1) {
set_error (errno, "listen");
goto unlink_sockpath;
}
+ 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) {
set_error (errno, "prepare_socket_activation_environment");
- goto unlink_sockpath;
+ goto uninit_execvpe;
}
pid = fork ();
if (pid == -1) {
set_error (errno, "fork");
goto empty_env;
}
if (pid == 0) { /* child - run command */
if (s != FIRST_SOCKET_ACTIVATION_FD) {
if (dup2 (s, FIRST_SOCKET_ACTIVATION_FD) == -1) {
@@ -189,45 +196,47 @@ 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);
/* 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);
+ (void)nbd_internal_fork_safe_execvpe (&execvpe_ctx, &h->argv,
env.ptr);
nbd_internal_fork_safe_perror (h->argv.ptr[0]);
if (errno == ENOENT)
_exit (127);
else
_exit (126);
}
/* 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);
next = %^CONNECT.START;
/* fall through, for releasing the temporaries */
empty_env:
string_vector_empty (&env);
+uninit_execvpe:
+ nbd_internal_execvpe_uninit (&execvpe_ctx);
+
unlink_sockpath:
if (next == %.DEAD)
unlink (sockpath);
close_socket:
close (s);
free_sockpath:
if (next == %.DEAD)
free (sockpath);
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 18/29] CONNECT_COMMAND.START: fix small comment thinko about socket pair usage
A comment currently states that "*the* socket must be set to non-blocking
only in the parent" (emphasis mine). This is a thinko: it implies that we
have just one file descriptor, and that the underlying file description
(which is manipulated by fcntl()'s F_GETFL and F_SETFL) is shared by the
parent and the child processes. That's not the case: we have *two* sockets
here (a socket pair), one of which is used by the parent exclusively, and
the other is used by the child exclusively.
Clarify the comment: say that we set the parent-side end of the socket
pair to non-blocking.
This clarification will play a role in a subsequent patch.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-U5
generator/states-connect.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index 8a286e6630ff..95a96b9114c6 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -281,11 +281,11 @@ CONNECT_COMMAND.START:
/* Parent. */
close (sv[1]);
h->pid = pid;
- /* The socket must be set to non-blocking only in the 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) {
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 19/29] CONNECT_COMMAND.START: set the NBD error when fcntl() fails
Furthermore, explain in a comment that *not* calling set_error() upon
nbd_internal_socket_create() failing is intentional, and not an omission.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-W
generator/states-connect.c | 2 ++
1 file changed, 2 insertions(+)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index 95a96b9114c6..83d7f316828e 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -240,71 +240,73 @@ CONNECT_TCP.NEXT_ADDRESS:
CONNECT_COMMAND.START:
int sv[2];
pid_t pid;
int flags;
assert (!h->sock);
assert (h->argv.ptr);
assert (h->argv.ptr[0]);
if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "socketpair");
return 0;
}
pid = fork ();
if (pid == -1) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "fork");
close (sv[0]);
close (sv[1]);
return 0;
}
if (pid == 0) { /* child - run command */
close (0);
close (1);
close (sv[0]);
dup2 (sv[1], 0);
dup2 (sv[1], 1);
close (sv[1]);
/* Restore SIGPIPE back to SIG_DFL. */
signal (SIGPIPE, SIG_DFL);
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,
* 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;
}
h->sock = nbd_internal_socket_create (sv[0]);
if (!h->sock) {
SET_NEXT_STATE (%.DEAD);
+ /* nbd_internal_socket_create() calls set_error() internally */
close (sv[0]);
return 0;
}
/* The sockets are connected already, we can jump directly to
* receiving the server magic.
*/
SET_NEXT_STATE (%^MAGIC.START);
return 0;
} /* END STATE MACHINE */
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 20/29] CONNECT_COMMAND.START: use symbolic constants for fd#0 and fd#1
Refer to fd#0 and fd#1 with the symbolic constants STDIN_FILENO and
STDOUT_FILENO from <unistd.h>, for better readability.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
generator/states-connect.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index 83d7f316828e..f0020d6f3b26 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -260,11 +260,11 @@ CONNECT_COMMAND.START:
return 0;
}
if (pid == 0) { /* child - run command */
- close (0);
- close (1);
+ close (STDIN_FILENO);
+ close (STDOUT_FILENO);
close (sv[0]);
- dup2 (sv[1], 0);
- dup2 (sv[1], 1);
+ dup2 (sv[1], STDIN_FILENO);
+ dup2 (sv[1], STDOUT_FILENO);
close (sv[1]);
/* Restore SIGPIPE back to SIG_DFL. */
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 21/29] CONNECT_COMMAND.START: sanitize close() calls in the child process
In the child process:
- we have no need for the parent-side end of the socket pair (sv[0]),
- we duplicate the child-side end of the socket pair (sv[1]) to both of
the child's fd#0 and fd#1, and then close sv[1].
close (STDIN_FILENO);
close (STDOUT_FILENO);
close (sv[0]);
dup2 (sv[1], STDIN_FILENO);
dup2 (sv[1], STDOUT_FILENO);
close (sv[1]);
This code silently assumes that sv[1] falls outside of the the fd set
{0,1} -- put differently, the code assumes that each dup2() call will
duplicate sv[1] to a file descriptor that is *different* from sv[1].
Let's investigate (a) if the logic is correct under said assumption, and
(b) what happened if the assumption were wrong, (c) whether the assumption
is valid to make.
(a) Under the assumption, the initial two close() calls turn out to be
superfluous. That's because dup2 (oldfd, newfd) closes "newfd"
first,
if "newfd" is open and *differs* from "oldfd".
(b) If the assumption were wrong -- that is, if sv[1] equaled 0 or 1 --,
then the logic would misbehave, at several levels.
First, one of the initial close() calls would cause sv[1] to be
closed, thereby breaking both dup2() calls.
Second, even if we removed the first two close() calls, the final
close() would still be wrong. In this case, one of the dup2() calls
would be a no-op (oldfd == newfd), and the other dup2() would work
correctly. However, the final close would retroactively invalidate the
one dup2() call that had behaved as a no-op.
(c) Now let's see whether we need to fix issue (b); that is, whether the
assumption that sv[1] differs from both 0 and 1 is valid to make.
This assumption is actually valid. ISO C guarantees that the stdin,
stdout and stderr I/O streams are open at program startup, and POSIX
translates the same requirement to file descriptors. In particular,
the *informative* rationale in POSIX tallies its *normative*
requirements as follows:
https://pubs.opengroup.org/onlinepubs/9699919799/xrat/V4_xsh_chap02.html#tag_22_02_05
| B.2.5 Standard I/O Streams
|
| Although the ISO C standard guarantees that, at program start-up,
| /stdin/ is open for reading and /stdout/ and /stderr/ are open for
| writing, this guarantee is contingent (as are all guarantees made by
| the ISO C and POSIX standards) on the program being executed in a
| conforming environment. Programs executed with file descriptor 0 not
| open for reading or with file descriptor 1 or 2 not open for writing
| are executed in a non-conforming environment. Application writers
| are warned (in [exec], [posix_spawn], and [Redirection]) not to
| execute a standard utility or a conforming application with file
| descriptor 0 not open for reading or with file descriptor 1 or 2 not
| open for writing.
The proper way for "disabling" these file descriptors (as
described by
XRAT as well) is to redirect them from/to "/dev/null".
Now, if the libnbd client application closed file descriptors 0 and/or
1 after its startup, it would effectively invalidate itself. Consider
the (informative) APPLICATION USAGE section of the fclose() spec:
https://pubs.opengroup.org/onlinepubs/9699919799/functions/fclose.html#tag_16_121_07
| Since after the call to /fclose()/ any use of stream results in
| undefined behavior, /fclose()/ should not be used on /stdin/,
| /stdout/, or /stderr/ except immediately before process termination
| (see XBD [Process Termination]), so as to avoid triggering undefined
| behavior in other standard interfaces that rely on these streams. If
| there are any [atexit()] handlers registered by the application,
| such a call to /fclose()/ should not occur until the last handler is
| finishing. Once /fclose()/ has been used to close /stdin/, /stdout/,
| or /stderr/, there is no standard way to reopen any of these
| streams.
|
| Use of [freopen()] to change /stdin/, /stdout/, or /stderr/ instead
| of closing them avoids the danger of a file unexpectedly being
| opened as one of the special file descriptors STDIN_FILENO,
| STDOUT_FILENO, or STDERR_FILENO at a later time in the application.
Thus, the assumption is deemed valid.
Therefore:
- While valid, the assumption is not trivial. So, assert it in the child
process. Furthermore, because regular assert()'s in the parent process
may be easier to read for the user, assert a slightly more comprehensive
predicate about socketpair()'s output there, too.
- Remove the first two close() calls, which are superfluous.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-U6
generator/states-connect.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index f0020d6f3b26..e08608336fd6 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -248,26 +248,35 @@ CONNECT_COMMAND.START:
if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "socketpair");
return 0;
}
+ /* 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;
}
if (pid == 0) { /* child - run command */
- close (STDIN_FILENO);
- close (STDOUT_FILENO);
close (sv[0]);
dup2 (sv[1], STDIN_FILENO);
dup2 (sv[1], STDOUT_FILENO);
+ NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDIN_FILENO);
+ NBD_INTERNAL_FORK_SAFE_ASSERT (sv[1] != STDOUT_FILENO);
close (sv[1]);
/* Restore SIGPIPE back to SIG_DFL. */
signal (SIGPIPE, SIG_DFL);
execvp (h->argv.ptr[0], h->argv.ptr);
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 22/29] CONNECT_COMMAND.START: check syscalls for errors in the child process
It's bad hygiene to just assume dup2(), close() and signal() will succeed.
Check all return values.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-U10
generator/states-connect.c | 22 +++++++++++++++-----
1 file changed, 17 insertions(+), 5 deletions(-)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index e08608336fd6..d2476dc11c60 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -262,29 +262,41 @@ CONNECT_COMMAND.START:
pid = fork ();
if (pid == -1) {
SET_NEXT_STATE (%.DEAD);
set_error (errno, "fork");
close (sv[0]);
close (sv[1]);
return 0;
}
if (pid == 0) { /* child - run command */
- close (sv[0]);
- dup2 (sv[1], STDIN_FILENO);
- dup2 (sv[1], STDOUT_FILENO);
+ 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);
- close (sv[1]);
+ if (close (sv[1]) == -1) {
+ nbd_internal_fork_safe_perror ("close");
+ _exit (126);
+ }
/* 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);
+ }
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. */
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 23/29] CONNECT_COMMAND.START: centralize resource release
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 at redhat.com>
---
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 */
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 24/29] CONNECT_COMMAND.START: plug child process leak on error
A prior patch highlighted the following leak: when any construction step
fails in the parent after fork(), the child process is leaked.
We could plug the leak by inserting a new error handling section for
killing the child process and reaping it with waitpid(). However, it would
raise some new questions (what signal to send, ensure the child not ignore
that signal, hope that whatever process image we execute in the child
handle that signal properly, etc).
Instead, rearrange the steps in the CONNECT_COMMAND.START handler so that
fork() be the last operation in the parent process, on the construction
path. If fork() succeeds, let the entire handler succeed. For this, move
the fcntl() and nbd_internal_socket_create() calls above fork().
(The hoisting of the fcntl() calls is where we rely on the earlier
observation that O_NONBLOCK only applies to the parent-side end of the
socket pair, so it is safe to do before forking.)
The trouble in turn is that nbd_internal_socket_create() takes ownership
of the parent-side end of the socket pair (sv[0]). So once that function
returns successfully, we can't close sv[0] even on the error path -- we
close the "higher level" socket, and that invalidates sv[0] at once.
Track
this ownership transfer with a new boolean flag therefore.
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
Notes:
context:-W
generator/states-connect.c | 48 +++++++++++---------
1 file changed, 27 insertions(+), 21 deletions(-)
diff --git a/generator/states-connect.c b/generator/states-connect.c
index bd78f890a2c5..283481bb7f91 100644
--- a/generator/states-connect.c
+++ b/generator/states-connect.c
@@ -239,103 +239,109 @@ CONNECT_TCP.NEXT_ADDRESS:
CONNECT_COMMAND.START:
enum state next;
+ bool parentfd_transferred;
int sv[2];
- pid_t pid;
int flags;
struct socket *sock;
+ pid_t pid;
assert (!h->sock);
assert (h->argv.ptr);
assert (h->argv.ptr[0]);
next = %.DEAD;
+ parentfd_transferred = false;
if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) {
set_error (errno, "socketpair");
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);
+ /* 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_error (errno, "fcntl");
+ goto close_socket_pair;
+ }
+
+ sock = nbd_internal_socket_create (sv[0]);
+ if (!sock)
+ /* nbd_internal_socket_create() calls set_error() internally */
+ goto close_socket_pair;
+ parentfd_transferred = true;
+
pid = fork ();
if (pid == -1) {
set_error (errno, "fork");
- goto close_socket_pair;
+ goto close_high_level_socket;
}
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.
- *
- * 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_error (errno, "fcntl");
- goto close_socket_pair;
- }
-
- sock = nbd_internal_socket_create (sv[0]);
- if (!sock)
- /* nbd_internal_socket_create() calls set_error() internally */
- goto close_socket_pair;
-
- /* Commit. */
+ /* Parent -- we're done; commit. */
h->pid = pid;
h->sock = sock;
/* The sockets are connected already, we can jump directly to
* receiving the server magic.
*/
next = %^MAGIC.START;
/* fall through, for releasing the temporaries */
-close_socket_pair:
+close_high_level_socket:
if (next == %.DEAD)
+ sock->ops->close (sock);
+
+close_socket_pair:
+ assert (next == %.DEAD || parentfd_transferred);
+ if (!parentfd_transferred)
close (sv[0]);
close (sv[1]);
done:
SET_NEXT_STATE (next);
return 0;
} /* END STATE MACHINE */
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 25/29] CONNECT_COMMAND.START: replace execvp() call with fork-safe variant
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 at redhat.com>
---
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)
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 26/29] socket activation: generalize environment construction
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 at redhat.com>
---
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);
}
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 27/29] common/include: Copy ascii-ctype functions from nbdkit
From: "Richard W.M. Jones" <rjones at redhat.com>
[Laszlo's note: "ascii-ctype.h" and "test-ascii-ctype.c"
match those of
nbdkit at commit 5fd4262ef07f. Originally posted by Rich at
<https://listman.redhat.com/archives/libguestfs/2023-January/030559.html>.]
Message-Id: <20230130225521.1771496-3-rjones at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
common/include/ascii-ctype.h | 75 +++++++++++++++++
common/include/Makefile.am | 6 ++
common/include/test-ascii-ctype.c | 88 ++++++++++++++++++++
.gitignore | 1 +
4 files changed, 170 insertions(+)
diff --git a/common/include/ascii-ctype.h b/common/include/ascii-ctype.h
new file mode 100644
index 000000000000..a55dee11b8b1
--- /dev/null
+++ b/common/include/ascii-ctype.h
@@ -0,0 +1,75 @@
+/* nbdkit
+ * Copyright (C) 2013-2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+/* Normal ctype functions are affected by the current locale. For
+ * example isupper() might recognize ? in some but not all locales.
+ * These functions match only 7 bit ASCII characters.
+ */
+
+#ifndef NBDKIT_ASCII_CTYPE_H
+#define NBDKIT_ASCII_CTYPE_H
+
+#define ascii_isalnum(c) (ascii_isalpha (c) || ascii_isdigit (c))
+
+#define ascii_isalpha(c) \
+ (((c) >= 'a' && (c) <= 'z') || ((c) >=
'A' && (c) <= 'Z'))
+
+#define ascii_isdigit(c) \
+ ((c) >= '0' && (c) <= '9')
+
+#define ascii_isspace(c) \
+ ((c) == '\t' || (c) == '\n' || (c) == '\f' || (c) ==
'\r' || (c) == ' ')
+
+#define ascii_isupper(c) \
+ ((c) >= 'A' && (c) <= 'Z')
+
+#define ascii_islower(c) \
+ ((c) >= 'a' && (c) <= 'z')
+
+/* See also hexdigit.h */
+#define ascii_isxdigit(c) \
+ ((c) == '0' || (c) == '1' || (c) == '2' || (c) ==
'3' || (c) == '4' || \
+ (c) == '5' || (c) == '6' || (c) == '7' || (c) ==
'8' || (c) == '9' || \
+ (c) == 'a' || (c) == 'b' || (c) == 'c' ||
\
+ (c) == 'd' || (c) == 'e' || (c) == 'f' ||
\
+ (c) == 'A' || (c) == 'B' || (c) == 'C' ||
\
+ (c) == 'D' || (c) == 'E' || (c) == 'F')
+
+#define ascii_tolower(c) \
+ (ascii_isupper ((c)) ? (c) + 32 : (c))
+
+#define ascii_toupper(c) \
+ (ascii_islower ((c)) ? (c) - 32 : (c))
+
+#define ascii_isprint(c) ((c) >= 32 && (c) <= 126)
+
+#endif /* NBDKIT_ASCII_CTYPE_H */
diff --git a/common/include/Makefile.am b/common/include/Makefile.am
index fa2633c25ac3..04553a367d84 100644
--- a/common/include/Makefile.am
+++ b/common/include/Makefile.am
@@ -20,6 +20,7 @@ include $(top_srcdir)/subdir-rules.mk
EXTRA_DIST = \
ansi-colours.h \
array-size.h \
+ ascii-ctype.h \
byte-swapping.h \
checked-overflow.h \
compiler-macros.h \
@@ -35,6 +36,7 @@ EXTRA_DIST = \
TESTS = \
test-array-size \
+ test-ascii-ctype \
test-checked-overflow \
test-ispowerof2 \
$(NULL)
@@ -44,6 +46,10 @@ test_array_size_SOURCES = test-array-size.c array-size.h
test_array_size_CPPFLAGS = -I$(srcdir)
test_array_size_CFLAGS = $(WARNINGS_CFLAGS)
+test_ascii_ctype_SOURCES = test-ascii-ctype.c ascii-ctype.h
+test_ascii_ctype_CPPFLAGS = -I$(srcdir)
+test_ascii_ctype_CFLAGS = $(WARNINGS_CFLAGS)
+
test_checked_overflow_SOURCES = test-checked-overflow.c checked-overflow.h
test_checked_overflow_CPPFLAGS = -I$(srcdir)
test_checked_overflow_CFLAGS = $(WARNINGS_CFLAGS)
diff --git a/common/include/test-ascii-ctype.c
b/common/include/test-ascii-ctype.c
new file mode 100644
index 000000000000..4fbb0259f83a
--- /dev/null
+++ b/common/include/test-ascii-ctype.c
@@ -0,0 +1,88 @@
+/* nbdkit
+ * Copyright (C) 2020 Red Hat Inc.
+ *
+ * Redistribution and use in source and binary forms, with or without
+ * modification, are permitted provided that the following conditions are
+ * met:
+ *
+ * * Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * * Redistributions in binary form must reproduce the above copyright
+ * notice, this list of conditions and the following disclaimer in the
+ * documentation and/or other materials provided with the distribution.
+ *
+ * * Neither the name of Red Hat nor the names of its contributors may be
+ * used to endorse or promote products derived from this software without
+ * specific prior written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY RED HAT AND CONTRIBUTORS ''AS
IS'' AND
+ * ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO,
+ * THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A
+ * PARTICULAR PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL RED HAT OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT
+ * LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF
+ * USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND
+ * ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY,
+ * OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT
+ * OF THE USE OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF
+ * SUCH DAMAGE.
+ */
+
+#include <config.h>
+
+#include <stdio.h>
+#include <stdlib.h>
+#undef NDEBUG /* Keep test strong even for nbdkit built without assertions */
+#include <assert.h>
+
+#include "ascii-ctype.h"
+
+int
+main (void)
+{
+ assert (ascii_isspace (' '));
+ assert (ascii_isspace ('\t'));
+ assert (ascii_isspace ('\n'));
+ assert (! ascii_isspace ('a'));
+
+ assert (ascii_isalpha ('a'));
+ assert (ascii_isalpha ('Z'));
+ assert (ascii_isalpha ('z'));
+ assert (! ascii_isalpha (' '));
+ assert (! ascii_isalpha ('0'));
+ { const char *s = "?"; assert (! ascii_isalpha (s[0])); }
+ { const char *s = "?"; assert (! ascii_isalpha (s[0])); }
+
+ assert (ascii_isdigit ('0'));
+ assert (ascii_isdigit ('9'));
+ { const char *s = "?"; assert (! ascii_isdigit (s[0])); } /* U+00D8
*/
+ { const char *s = "?"; assert (! ascii_isdigit (s[0])); } /* U+FF19
*/
+
+ assert (ascii_islower ('a'));
+ assert (ascii_islower ('z'));
+ assert (! ascii_islower ('Z'));
+ { const char *s = "?"; assert (! ascii_islower (s[0])); }
+
+ assert (ascii_isupper ('A'));
+ assert (ascii_isupper ('Z'));
+ assert (! ascii_isupper ('z'));
+ { const char *s = "?"; assert (! ascii_isupper (s[0])); }
+
+ assert (ascii_tolower ('A') == 'a');
+ assert (ascii_tolower ('Z') == 'z');
+ assert (ascii_tolower ('a') == 'a');
+ assert (ascii_tolower ('z') == 'z');
+ assert (ascii_tolower ('0') == '0');
+ { const char *s = "?"; assert (ascii_tolower (s[0]) == s[0]); }
+
+ assert (ascii_toupper ('a') == 'A');
+ assert (ascii_toupper ('z') == 'Z');
+ assert (ascii_toupper ('A') == 'A');
+ assert (ascii_toupper ('Z') == 'Z');
+ assert (ascii_toupper ('0') == '0');
+ { const char *s = "?"; assert (ascii_toupper (s[0]) == s[0]); }
+
+ exit (EXIT_SUCCESS);
+}
diff --git a/.gitignore b/.gitignore
index 5c42060dc0dd..af7a8285fec7 100644
--- a/.gitignore
+++ b/.gitignore
@@ -34,6 +34,7 @@ Makefile.in
/bash-completion/nbdinfo
/bash-completion/nbdublk
/common/include/test-array-size
+/common/include/test-ascii-ctype
/common/include/test-checked-overflow
/common/include/test-ispowerof2
/common/utils/test-human-size
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 28/29] generator: Add APIs to get/set the socket activation socket name
From: "Richard W.M. Jones" <rjones at redhat.com>
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 at redhat.com>
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
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)
{
Laszlo Ersek
2023-Feb-15 14:11 UTC
[Libguestfs] [libnbd PATCH v3 29/29] socket activation: set LISTEN_FDNAMES
When the user calls nbd_set_socket_activation_name before calling
nbd_connect_system_socket_activation, pass the name down to the server
through LISTEN_FDNAMES. This has no effect unless the new API has
been called to set the socket name to a non-empty string.
https://lists.nongnu.org/archive/html/qemu-devel/2023-01/msg06114.html
[Original commit message and upstream discussion reference by Rich Jones;
at
<https://listman.redhat.com/archives/libguestfs/2023-January/030558.html>
/ msgid <20230130225521.1771496-5-rjones at redhat.com>.]
Signed-off-by: Laszlo Ersek <lersek at redhat.com>
---
generator/states-connect-socket-activation.c | 5 ++++-
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/generator/states-connect-socket-activation.c
b/generator/states-connect-socket-activation.c
index 94d641cde6d9..150b5d6ac005 100644
--- a/generator/states-connect-socket-activation.c
+++ b/generator/states-connect-socket-activation.c
@@ -184,7 +184,7 @@ CONNECT_SA.START:
struct sockaddr_un addr;
struct execvpe execvpe_ctx;
size_t num_vars;
- struct sact_var sact_var[2];
+ struct sact_var sact_var[3];
size_t pid_ofs;
string_vector env;
pid_t pid;
@@ -244,6 +244,9 @@ CONNECT_SA.START:
"LISTEN_PID=",
"xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx", &pid_ofs);
SACT_VAR_PUSH (sact_var, &num_vars,
"LISTEN_FDS=", "1", NULL);
+ if (h->sact_name != NULL)
+ SACT_VAR_PUSH (sact_var, &num_vars,
+ "LISTEN_FDNAMES=", h->sact_name, NULL);
if (prepare_socket_activation_environment (&env, sact_var, num_vars) ==
-1) {
set_error (errno, "prepare_socket_activation_environment");
goto uninit_execvpe;