[libnbd PATCH v2 0/2] Paint the pipeline complete green
Last two fixes missing ;) Martin Kletzander (2): info: Require can_cache for info-can.sh macOS: Simple cloexec/nonblock fix lib/internal.h | 7 ++ generator/states-connect-socket-activation.c | 2 +- generator/states-connect.c | 11 +-- lib/utils.c | 79 ++++++++++++++++++++ fuzzing/libnbd-fuzz-wrapper.c | 4 + fuzzing/libnbd-libfuzzer-test.c | 4 + info/info-can.sh | 2 +- 7 files changed, 102 insertions(+), 7 deletions(-) -- 2.32.0
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- info/info-can.sh | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/info/info-can.sh b/info/info-can.sh index 4154e38e85ba..00ba98107a29 100755 --- a/info/info-can.sh +++ b/info/info-can.sh @@ -22,7 +22,7 @@ set -e set -x requires nbdkit --version -requires nbdkit sh --version +requires bash -c "nbdkit sh --dump-config | grep has_can_cache=1" # --is read-only and --can write are tested in info-is-read-only.sh -- 2.32.0
This is the most trivial way to fix the issue with macOS not having SOCK_CLOEXEC and SOCK_NONBLOCK. This is the only way to make it work on such platform(s) unless they are fixed. Signed-off-by: Martin Kletzander <mkletzan@redhat.com> Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- lib/internal.h | 7 ++ generator/states-connect-socket-activation.c | 2 +- generator/states-connect.c | 11 +-- lib/utils.c | 79 ++++++++++++++++++++ fuzzing/libnbd-fuzz-wrapper.c | 4 + fuzzing/libnbd-libfuzzer-test.c | 4 + 6 files changed, 101 insertions(+), 6 deletions(-) diff --git a/lib/internal.h b/lib/internal.h index 01f9d8ab5fea..12938aaa0444 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -467,4 +467,11 @@ extern char *nbd_internal_printable_buffer (const void *buf, size_t count); extern char *nbd_internal_printable_string (const char *str); extern char *nbd_internal_printable_string_list (char **list); +/* + * These are wrappers over socket(2) and socketpair(2) that work on macOS where + * SOCK_NONBLOCK and SOCK_CLOEXEC are not available. + */ +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); + #endif /* LIBNBD_INTERNAL_H */ diff --git a/generator/states-connect-socket-activation.c b/generator/states-connect-socket-activation.c index e601c9bb56be..8a2add312bc4 100644 --- a/generator/states-connect-socket-activation.c +++ b/generator/states-connect-socket-activation.c @@ -131,7 +131,7 @@ STATE_MACHINE { return 0; } - s = socket (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0); + s = nbd_internal_socket (AF_UNIX, SOCK_STREAM, 0, false); if (s == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socket"); diff --git a/generator/states-connect.c b/generator/states-connect.c index fcac86f36a34..8de12183d627 100644 --- a/generator/states-connect.c +++ b/generator/states-connect.c @@ -52,7 +52,7 @@ STATE_MACHINE { assert (!h->sock); family = h->connaddr.ss_family; - fd = socket (family, SOCK_STREAM|SOCK_NONBLOCK|SOCK_CLOEXEC, 0); + fd = nbd_internal_socket (family, SOCK_STREAM, 0, true); if (fd == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socket"); @@ -162,9 +162,10 @@ STATE_MACHINE { return -1; } - fd = socket (h->rp->ai_family, - h->rp->ai_socktype|SOCK_NONBLOCK|SOCK_CLOEXEC, - h->rp->ai_protocol); + fd = nbd_internal_socket (h->rp->ai_family, + h->rp->ai_socktype, + h->rp->ai_protocol, + true); if (fd == -1) { SET_NEXT_STATE (%NEXT_ADDRESS); return 0; @@ -227,7 +228,7 @@ STATE_MACHINE { assert (!h->sock); assert (h->argv.ptr); assert (h->argv.ptr[0]); - if (socketpair (AF_UNIX, SOCK_STREAM|SOCK_CLOEXEC, 0, sv) == -1) { + if (nbd_internal_socketpair (AF_UNIX, SOCK_STREAM, 0, sv) == -1) { SET_NEXT_STATE (%.DEAD); set_error (errno, "socketpair"); return 0; diff --git a/lib/utils.c b/lib/utils.c index 260fd6a25796..5902c6ab016c 100644 --- a/lib/utils.c +++ b/lib/utils.c @@ -24,6 +24,7 @@ #include <unistd.h> #include <ctype.h> #include <errno.h> +#include <fcntl.h> #include "minmax.h" @@ -258,3 +259,81 @@ nbd_internal_printable_string_list (char **list) return s; } + +int nbd_internal_socket(int domain, + int type, + int protocol, + bool nonblock) +{ + int fd; + + /* So far we do not know about any platform that has SOCK_CLOEXEC and lacks + * SOCK_NONBLOCK at the same time. + * + * The workaround for missing SOCK_CLOEXEC introduces a race which cannot be + * fixed until support for SOCK_CLOEXEC is added (or other fix is implemented). + */ +#ifndef SOCK_CLOEXEC + int flags; +#else + type |= SOCK_CLOEXEC; + if (nonblock) + type |= SOCK_NONBLOCK; +#endif + + fd = socket (domain, type, protocol); + +#ifndef SOCK_CLOEXEC + if (fd == -1) + return -1; + + if (fcntl (fd, F_SETFD, FD_CLOEXEC) == -1) { + close(fd); + return -1; + } + + if (nonblock) { + flags = fcntl (fd, F_GETFL, 0); + if (flags == -1 || + fcntl (fd, F_SETFL, flags|O_NONBLOCK) == -1) { + close(fd); + return -1; + } + } +#endif + + return fd; +} + +int +nbd_internal_socketpair (int domain, int type, int protocol, int *fds) +{ + int ret; + + /* + * Same as with nbd_internal_socket() this workaround for missing SOCK_CLOEXEC + * introduces a race which cannot be fixed until support for SOCK_CLOEXEC is + * added (or other fix is implemented). + */ +#ifndef SOCK_CLOEXEC + size_t i; +#else + type |= SOCK_CLOEXEC; +#endif + + ret = socketpair (domain, type, protocol, fds); + +#ifndef SOCK_CLOEXEC + 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; +} diff --git a/fuzzing/libnbd-fuzz-wrapper.c b/fuzzing/libnbd-fuzz-wrapper.c index 99a6d803258f..eb188094cca7 100644 --- a/fuzzing/libnbd-fuzz-wrapper.c +++ b/fuzzing/libnbd-fuzz-wrapper.c @@ -38,6 +38,10 @@ #include <libnbd.h> +#ifndef SOCK_CLOEXEC +#define SOCK_CLOEXEC 0 /* This file doesn't use exec */ +#endif + static void client (int s); static void server (int fd, int s); diff --git a/fuzzing/libnbd-libfuzzer-test.c b/fuzzing/libnbd-libfuzzer-test.c index 5ee29b877bdb..c8d6423bf149 100644 --- a/fuzzing/libnbd-libfuzzer-test.c +++ b/fuzzing/libnbd-libfuzzer-test.c @@ -38,6 +38,10 @@ #include <libnbd.h> +#ifndef SOCK_CLOEXEC +#define SOCK_CLOEXEC 0 /* This file doesn't use exec */ +#endif + static void client (int sock); static void server (const uint8_t *data, size_t size, int sock); -- 2.32.0
On Tue, Jul 27, 2021 at 01:31:05PM +0200, Martin Kletzander wrote:
This is the most trivial way to fix the issue with macOS not having SOCK_CLOEXEC and SOCK_NONBLOCK. This is the only way to make it work on such platform(s) unless they are fixed.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com> --- lib/internal.h | 7 ++ generator/states-connect-socket-activation.c | 2 +- generator/states-connect.c | 11 +-- lib/utils.c | 79 ++++++++++++++++++++ fuzzing/libnbd-fuzz-wrapper.c | 4 + fuzzing/libnbd-libfuzzer-test.c | 4 + 6 files changed, 101 insertions(+), 6 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h index 01f9d8ab5fea..12938aaa0444 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -467,4 +467,11 @@ extern char *nbd_internal_printable_buffer (const void *buf, size_t count); extern char *nbd_internal_printable_string (const char *str); extern char *nbd_internal_printable_string_list (char **list);
+/* + * These are wrappers over socket(2) and socketpair(2) that work on macOS where + * SOCK_NONBLOCK and SOCK_CLOEXEC are not available.
It's still a bit unclear. Could we say: /* 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. */ Also, can you wrap lines at ~72 chars.
+int nbd_internal_socket(int domain, + int type, + int protocol, + bool nonblock) +{ + int fd; + + /* So far we do not know about any platform that has SOCK_CLOEXEC and lacks + * SOCK_NONBLOCK at the same time. + * + * The workaround for missing SOCK_CLOEXEC introduces a race which cannot be + * fixed until support for SOCK_CLOEXEC is added (or other fix is implemented). + */
Line wrapping here.
+int +nbd_internal_socketpair (int domain, int type, int protocol, int *fds) +{ + int ret; + + /* + * Same as with nbd_internal_socket() this workaround for missing SOCK_CLOEXEC + * introduces a race which cannot be fixed until support for SOCK_CLOEXEC is + * added (or other fix is implemented). + */
And here. It's all good otherwise: ACK the series with the changes above. Thanks, Rich. -- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
On Tue, Jul 27, 2021 at 01:52:34PM +0100, Richard W.M. Jones wrote:
On Tue, Jul 27, 2021 at 01:31:05PM +0200, Martin Kletzander wrote:
This is the most trivial way to fix the issue with macOS not having SOCK_CLOEXEC and SOCK_NONBLOCK. This is the only way to make it work on such platform(s) unless they are fixed.
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Signed-off-by: Martin Kletzander <mkletzan@redhat.com>
Also need to remove this leftover from squashing changes =)
--- lib/internal.h | 7 ++ generator/states-connect-socket-activation.c | 2 +- generator/states-connect.c | 11 +-- lib/utils.c | 79 ++++++++++++++++++++ fuzzing/libnbd-fuzz-wrapper.c | 4 + fuzzing/libnbd-libfuzzer-test.c | 4 + 6 files changed, 101 insertions(+), 6 deletions(-)
diff --git a/lib/internal.h b/lib/internal.h index 01f9d8ab5fea..12938aaa0444 100644 --- a/lib/internal.h +++ b/lib/internal.h @@ -467,4 +467,11 @@ extern char *nbd_internal_printable_buffer (const void *buf, size_t count); extern char *nbd_internal_printable_string (const char *str); extern char *nbd_internal_printable_string_list (char **list);
+/* + * These are wrappers over socket(2) and socketpair(2) that work on macOS where + * SOCK_NONBLOCK and SOCK_CLOEXEC are not available.
It's still a bit unclear. Could we say:
/* 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. */
OK, I'll use that. I wanted to express why we need them and forgot to write what they do.
Also, can you wrap lines at ~72 chars.
Ah, I used 72 for mails and 80 for code, will change that.
+int nbd_internal_socket(int domain, + int type, + int protocol, + bool nonblock) +{ + int fd; + + /* So far we do not know about any platform that has SOCK_CLOEXEC and lacks + * SOCK_NONBLOCK at the same time. + * + * The workaround for missing SOCK_CLOEXEC introduces a race which cannot be + * fixed until support for SOCK_CLOEXEC is added (or other fix is implemented). + */
Line wrapping here.
+int +nbd_internal_socketpair (int domain, int type, int protocol, int *fds) +{ + int ret; + + /* + * Same as with nbd_internal_socket() this workaround for missing SOCK_CLOEXEC + * introduces a race which cannot be fixed until support for SOCK_CLOEXEC is + * added (or other fix is implemented). + */
And here.
It's all good otherwise: ACK the series with the changes above.
Thanks, unfortunately there is a need for an extra (unrelated) modification to finish this, but it is small and already prepared. I'll send it soon.
Thanks,
Rich.
-- Richard Jones, Virtualization Group, Red Hat http://people.redhat.com/~rjones Read my programming and virtualization blog: http://rwmj.wordpress.com Fedora Windows cross-compiler. Compile Windows programs, test, and build Windows installers. Over 100 libraries supported. http://fedoraproject.org/wiki/MinGW
participants (2)
-
Martin Kletzander -
Richard W.M. Jones