March 2021 - Spectrum Development

[PATCH ucspi-vsock] vsockclient: allow null application
by Alyssa Ross 24 Mar '21

24 Mar '21

I found myself running vsockclient 2 1024 true in a situation where it just mattered that a connection was made, and no data needed to be exchanged of it. That I found myself doing this is quite a strong indicator that the program to exec into should be optional. UCSPI-1996[1] doesn't specify what should happen if no application is provided, so I think this is a reasonable interpretation. I'm not sure if I feel the same for vsockserver(d), so I haven't made the corresponding change there. I'll wait to see if there's ever a need for it. [1]: https://cr.yp.to/proto/ucspi.txt --- vsockclient.c | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/vsockclient.c b/vsockclient.c index 934cfa0..456810d 100644 --- a/vsockclient.c +++ b/vsockclient.c @@ -41,11 +41,9 @@ int main(int argc, char *argv[]) } } - // Check there are enough positional arguments (two for the - // address and at least one to exec into). - if (optind > argc - 3) + // Find the address (cid and port) to connect to. + if (optind > argc - 2) ex_usage(); - if (getu32(argv[optind++], 0, UINT32_MAX, &rcid)) ex_usage(); if (getu32(argv[optind++], 0, UINT32_MAX, &rport)) @@ -69,6 +67,11 @@ int main(int argc, char *argv[]) "connect to %" PRIu32 ":%" PRIu32 " failed", rcid, rport); ilog("connected to %" PRIu32 ":%" PRIu32 "!", rcid, rport); + // If we have nothing to exec into, we're done. (Sometimes + // just connecting on its own is enough.) + if (optind == argc) + return 0; + // Set up the file descriptors to the well-known UCSPI numbers. if (dup2(fd, 6) == -1) diee(EX_OSERR, "dup2"); -- 2.30.0

2 3

[PATCH ucspi-vsock] Remove unused includes
by Alyssa Ross 24 Mar '21

24 Mar '21

--- vsock.c | 1 - vsockclient.c | 3 --- 2 files changed, 4 deletions(-) diff --git a/vsock.c b/vsock.c index ab8c4bd..159a51f 100644 --- a/vsock.c +++ b/vsock.c @@ -6,7 +6,6 @@ #include "vsock.h" #include <errno.h> -#include <sys/ioctl.h> #include <sys/socket.h> #include <linux/vm_sockets.h> diff --git a/vsockclient.c b/vsockclient.c index d6cc778..934cfa0 100644 --- a/vsockclient.c +++ b/vsockclient.c @@ -8,12 +8,9 @@ #include <stdio.h> #include <stdlib.h> #include <stdnoreturn.h> -#include <sys/socket.h> #include <sysexits.h> #include <unistd.h> -#include <linux/vm_sockets.h> - #include "env.h" #include "log.h" #include "num.h" -- 2.30.0

2 2

[PATCH ucspi-vsock] configure: reject invalid arguments
by Alyssa Ross 22 Mar '21

22 Mar '21

But we preserve the autoconf behaviour of only warning on invalid options. $ ./configure --test Warning: unrecognized options: --test $ ./configure test Error: unexpected argument: test --- configure | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/configure b/configure index 38c926c..938db10 100755 --- a/configure +++ b/configure @@ -29,8 +29,11 @@ for arg; do prefix="$(printf "%s" "$arg" | cut -d= -f2-)" elif printf "%s" "$arg" | grep -q "^--bindir="; then bindir="$(printf "%s" "$arg" | cut -d= -f2-)" - else + elif printf "%s" "$arg" | grep -q "^--"; then unrecognized="$unrecognized $arg" + else + echo "Error: unexpected argument: $arg" >&2 + exit 64 # EX_USAGE fi done -- 2.30.0

2 2

[PATCH ucspi-vsock] Set up clang-format
by Alyssa Ross 22 Mar '21

22 Mar '21

--- I'm not entirely happy with the automatic formatting, but I think it's worth it. .clang-format | 10 ++++++++++ num.c | 2 +- vsock.c | 4 ++-- vsockclient.c | 5 +++-- vsockserver-socketbinder.c | 8 ++++---- vsockserver.c | 27 +++++++++++++++------------ vsockserverd.c | 9 +++++---- 7 files changed, 40 insertions(+), 25 deletions(-) create mode 100644 .clang-format diff --git a/.clang-format b/.clang-format new file mode 100644 index 0000000..89313b0 --- /dev/null +++ b/.clang-format @@ -0,0 +1,10 @@ +# SPDX-License-Identifier: GPL-2.0-or-later +# SPDX-FileCopyrightText: 2021 Alyssa Ross <hi(a)alyssa.is> + +AlignOperands: DontAlign +BreakBeforeBinaryOperators: All +BreakBeforeBraces: Linux +ContinuationIndentWidth: 8 +Cpp11BracedListStyle: false +IndentWidth: 8 +UseTab: AlignWithSpaces diff --git a/num.c b/num.c index ac7f0a4..cef71ae 100644 --- a/num.c +++ b/num.c @@ -21,7 +21,7 @@ int getu32(const char *s, uint32_t min, uint32_t max, uint32_t *out) } int getul(const char *s, unsigned long min, unsigned long max, - unsigned long *out) + unsigned long *out) { char *endptr; diff --git a/vsock.c b/vsock.c index 6f1f466..ab8c4bd 100644 --- a/vsock.c +++ b/vsock.c @@ -18,8 +18,8 @@ static void fill_sockaddr(struct sockaddr_vm *addr, uint32_t cid, uint32_t port) addr->svm_port = port; } -static int fill_cid_and_port(const struct sockaddr_vm *addr, - uint32_t *cid, uint32_t *port) +static int fill_cid_and_port(const struct sockaddr_vm *addr, uint32_t *cid, + uint32_t *port) { // Check that this sockaddr info is actually for the socket // type we think it is, or we could get some very confusing diff --git a/vsockclient.c b/vsockclient.c index fbdd20e..d6cc778 100644 --- a/vsockclient.c +++ b/vsockclient.c @@ -23,7 +23,7 @@ noreturn static void ex_usage(void) { if (verbosity) fprintf(stderr, "Usage: %s [ -q | -Q | -v ] cid port prog...\n", - program_invocation_short_name); + program_invocation_short_name); exit(EX_USAGE); } @@ -63,7 +63,8 @@ int main(int argc, char *argv[]) // the same host with no backlog, so retry a few times. { int i = 0; - do fd = vsock_open(rcid, rport); + do + fd = vsock_open(rcid, rport); while (i++ < 3 && fd == -1 && errno == ETIMEDOUT); } if (fd == -1) diff --git a/vsockserver-socketbinder.c b/vsockserver-socketbinder.c index fdcdfa8..97c5442 100644 --- a/vsockserver-socketbinder.c +++ b/vsockserver-socketbinder.c @@ -5,13 +5,13 @@ #include <errno.h> #include <fcntl.h> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> -#include <stdint.h> #include <stdnoreturn.h> #include <string.h> -#include <sysexits.h> #include <sys/socket.h> +#include <sysexits.h> #include <unistd.h> #include <linux/vm_sockets.h> @@ -26,7 +26,7 @@ noreturn static void ex_usage(void) { if (verbosity) fprintf(stderr, "Usage: %s cid port prog...\n", - program_invocation_short_name); + program_invocation_short_name); exit(EX_USAGE); } @@ -54,7 +54,7 @@ int main(int argc, char *argv[]) // Parse the `cid' argument. if (!strcmp(argv[optind], "-1")) cid = VMADDR_CID_ANY; - else if (getu32(argv[optind], 0, UINT32_MAX, &cid)) + else if (getu32(argv[optind], 0, UINT32_MAX, &cid)) ex_usage(); optind++; diff --git a/vsockserver.c b/vsockserver.c index b717eee..e45c2ea 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -19,8 +19,9 @@ noreturn static void ex_usage(void) { if (verbosity) - fprintf(stderr, "Usage: %s [ -1 ] [ -q | -Q | -v ] cid port prog...\n", - program_invocation_short_name); + fprintf(stderr, + "Usage: %s [ -1 ] [ -q | -Q | -v ] cid port prog...\n", + program_invocation_short_name); exit(EX_USAGE); } @@ -40,9 +41,10 @@ int main(int argc, char *argv[]) // we've parsed the arguments, so we know what our verbosity // setting is, and consequently whether we should print an // error message about the allocation failure. - alloc_failed |= - argz_add(&binder_opts, &binder_opts_len, BINDIR "/vsockserver-socketbinder") || - argz_add(&daemon_opts, &daemon_opts_len, BINDIR "/vsockserverd"); + alloc_failed |= argz_add(&binder_opts, &binder_opts_len, + BINDIR "/vsockserver-socketbinder") + || argz_add(&daemon_opts, &daemon_opts_len, + BINDIR "/vsockserverd"); while ((opt = getopt(argc, argv, "+1qQv")) != -1) { char *arg = NULL; @@ -53,9 +55,9 @@ int main(int argc, char *argv[]) case 'Q': case 'v': set_verbosity(opt); - alloc_failed |= - asprintf(&arg, "-%c", opt) == -1 || - argz_add(&daemon_opts, &daemon_opts_len, arg); + alloc_failed |= asprintf(&arg, "-%c", opt) == -1 + || argz_add(&daemon_opts, &daemon_opts_len, + arg); free(arg); break; default: @@ -78,15 +80,16 @@ int main(int argc, char *argv[]) ex_usage(); // Add `cid' and `port' arguments to binder options. - if (argz_add(&binder_opts, &binder_opts_len, "--") || - argz_add(&binder_opts, &binder_opts_len, argv[optind++]) || - argz_add(&binder_opts, &binder_opts_len, argv[optind++])) + if (argz_add(&binder_opts, &binder_opts_len, "--") + || argz_add(&binder_opts, &binder_opts_len, argv[optind++]) + || argz_add(&binder_opts, &binder_opts_len, argv[optind++])) diee(EX_OSERR, "malloc"); // Add all of daemon_opts onto the end of binder_opts. It's // okay to multiply to find the size because if it would // overflow calloc would have failed earlier. - if (argz_append(&binder_opts, &binder_opts_len, daemon_opts, daemon_opts_len)) + if (argz_append(&binder_opts, &binder_opts_len, daemon_opts, + daemon_opts_len)) diee(EX_OSERR, "malloc"); free(daemon_opts); diff --git a/vsockserverd.c b/vsockserverd.c index 1abfff9..a056bf1 100644 --- a/vsockserverd.c +++ b/vsockserverd.c @@ -23,7 +23,7 @@ noreturn static void ex_usage(void) { if (verbosity) fprintf(stderr, "Usage: %s [ -1 ] [ -q | -Q | -v ] prog...\n", - program_invocation_short_name); + program_invocation_short_name); exit(EX_USAGE); } @@ -33,8 +33,8 @@ int main(int argc, char *argv[]) int opt; pid_t child; uint32_t lcid, lport, rcid, rport; - struct pollfd poll_fd = - { .fd = STDIN_FILENO, .events = POLLIN, .revents = 0 }; + struct pollfd poll_fd + = { .fd = STDIN_FILENO, .events = POLLIN, .revents = 0 }; while ((opt = getopt(argc, argv, "+1qQv")) != -1) { switch (opt) { @@ -93,7 +93,8 @@ int main(int argc, char *argv[]) ilog("connection from %" PRIu32 " port %" PRIu32, rcid, rport); switch (child = fork()) { - case -1: diee(EX_OSERR, "fork"); + case -1: + diee(EX_OSERR, "fork"); case 0: // Set up the connection socket on prog's // stdin and stdout. This has the happy side -- 2.30.0

2 2

[PATCH ucspi-vsock 0/2] Fix clang-tidy warnings
by Alyssa Ross 21 Mar '21

21 Mar '21

clang-tidy also produced this warning: /home/src/ucspi-vsock/repro.c:43:19: warning: unused parameter 'sig' [clang-diagnostic-unused-parameter] void sig_exit(int sig) { exit(EX_UNAVAILABLE); } ^ vsock.c:27:23: warning: The left operand of '!=' is a garbage value [clang-analyzer-core.UndefinedBinaryOperatorResult] if (addr->svm_family != AF_VSOCK) { ^ vsock.c:90:6: note: Assuming the condition is false if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:90:2: note: Taking false branch if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) ^ vsock.c:93:9: note: Calling 'fill_cid_and_port' return fill_cid_and_port(&addr, cid, port); ^ vsock.c:27:23: note: The left operand of '!=' is a garbage value if (addr->svm_family != AF_VSOCK) { But I think this is just warning me that the POSIX socket API violates the strict aliasing rule. (Which is true, but there's not a lot I can do about it...) It also tells me I should use memset_s instead of memset, but, well... https://en.wikipedia.org/wiki/C11_(C_standard_revision)#Criticism Alyssa Ross (2): exec: free argv if exec fails vsockserver: fix uninitialized variable exec.c | 5 ++++- vsockserver.c | 2 +- 2 files changed, 5 insertions(+), 2 deletions(-) -- 2.30.0

2 4

[PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd
by Alyssa Ross 21 Mar '21

21 Mar '21

This series breaks vsockserver apart into two smaller programs, vsockserver-socketbinder and vsockserverd. vsockserver-socketbinder binds and listens on an AF_VSOCK socket and then execs into another program with the socket as stdin. vsockserverd accepts connections to a socket provided on stdin, and for each connection runs a program with the connection socket on stdin and stdout. vsockserver is replaced with a program that builds an argv that just combines these two smaller programs and execs into it. This design is based on s6[1]'s s6-ipcserver, s6-ipcserver-socketbinder, and s6-ipcserverd programs. [1]: https://skarnet.org/software/s6/ [2]: https://skarnet.org/software/s6-rc/ Alyssa Ross (7): vsock: get cid and port instead of just cid vsock: check socket family before reading sockaddr vsockserver: switch to a non-blocking socket configure: create, to generate config.h log: add die function exec: add execzp() Extract vsockserver-socketbinder and vsockserverd .gitignore | 7 +- Makefile | 33 -------- Makefile.in | 47 +++++++++++ configure | 49 ++++++++++++ exec.c | 22 +++++ exec.h | 7 ++ log.c | 10 +++ log.h | 8 +- vsock.c | 32 ++++++-- vsock.h | 14 +++- vsockserver-socketbinder.c | 86 ++++++++++++++++++++ vsockserver.c | 137 +++++++++++++++----------------- vsockserver.c => vsockserverd.c | 73 +++++++++-------- 13 files changed, 368 insertions(+), 157 deletions(-) delete mode 100644 Makefile create mode 100644 Makefile.in create mode 100755 configure create mode 100644 exec.c create mode 100644 exec.h create mode 100644 vsockserver-socketbinder.c copy vsockserver.c => vsockserverd.c (55%) -- 2.30.0

2 14

[PATCH ucspi-vsock] Makefile: use $@ where possible
by Alyssa Ross 19 Mar '21

19 Mar '21

This is, unfortunately, the only "internal macro" POSIX requires to be available for target rules, and I do want to keep the Makefile POSIX-compatible. But this is at least a slight improvement. --- Makefile | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Makefile b/Makefile index 2045cdd..e05e32f 100644 --- a/Makefile +++ b/Makefile @@ -21,9 +21,9 @@ install: $(PROGRAMS) .PHONY: install vsockclient: vsockclient.o env.o log.o num.o vsock.o - $(CC) $(LDFLAGS) -o vsockclient vsockclient.o env.o log.o num.o vsock.o $(LDLIBS) + $(CC) $(LDFLAGS) -o $@ $@.o env.o log.o num.o vsock.o $(LDLIBS) vsockserver: vsockserver.o env.o log.o num.o vsock.o - $(CC) $(LDFLAGS) -o vsockserver vsockserver.o env.o log.o num.o vsock.o $(LDLIBS) + $(CC) $(LDFLAGS) -o $@ $@.o env.o log.o num.o vsock.o $(LDLIBS) vsockclient.o: env.h log.h num.h vsock.h vsockserver.o: env.h log.h num.h vsock.h -- 2.30.0

2 2

[PATCH ucspi-vsock] Makefile: stop repeating program list
by Alyssa Ross 18 Mar '21

18 Mar '21

Using a wildcard with rm -f means that if there are no object files to remove, rm will try to remove a non-existent file named '*.c', and then ignore the failure it gets by trying to do that. Which is fine. --- Makefile | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/Makefile b/Makefile index 1a4bb10..2045cdd 100644 --- a/Makefile +++ b/Makefile @@ -10,12 +10,14 @@ INSTALL_PROGRAM = $(INSTALL) prefix = /usr/local bindir = $(prefix)/bin -all: vsockclient vsockserver +PROGRAMS = vsockclient vsockserver + +all: $(PROGRAMS) .PHONY: all -install: vsockclient vsockserver +install: $(PROGRAMS) mkdir -p $(DESTDIR)$(bindir) - $(INSTALL_PROGRAM) vsockclient vsockserver $(DESTDIR)$(bindir) + $(INSTALL_PROGRAM) $(PROGRAMS) $(DESTDIR)$(bindir) .PHONY: install vsockclient: vsockclient.o env.o log.o num.o vsock.o @@ -27,6 +29,5 @@ vsockclient.o: env.h log.h num.h vsock.h vsockserver.o: env.h log.h num.h vsock.h clean: - rm -f env.o log.o num.o vsock.o \ - vsockclient.o vsockclient vsockserver.o vsockserver + rm -f $(PROGRAMS) *.o .PHONY: clean -- 2.30.0

2 4

[PATCH ucspi-vsock] num: rename from util
by Alyssa Ross 18 Mar '21

18 Mar '21

I don't want to have anything called "util", because that will inevitably become a dumping ground for all sorts of unrelated stuff. --- Makefile | 14 +++++++------- util.c => num.c | 2 +- util.h => num.h | 0 vsockclient.c | 2 +- vsockserver.c | 2 +- 5 files changed, 10 insertions(+), 10 deletions(-) rename util.c => num.c (97%) rename util.h => num.h (100%) diff --git a/Makefile b/Makefile index 07ff09b..1a4bb10 100644 --- a/Makefile +++ b/Makefile @@ -18,15 +18,15 @@ install: vsockclient vsockserver $(INSTALL_PROGRAM) vsockclient vsockserver $(DESTDIR)$(bindir) .PHONY: install -vsockclient: vsockclient.o env.o log.o util.o vsock.o - $(CC) $(LDFLAGS) -o vsockclient vsockclient.o env.o log.o util.o vsock.o $(LDLIBS) -vsockserver: vsockserver.o env.o log.o util.o vsock.o - $(CC) $(LDFLAGS) -o vsockserver vsockserver.o env.o log.o util.o vsock.o $(LDLIBS) +vsockclient: vsockclient.o env.o log.o num.o vsock.o + $(CC) $(LDFLAGS) -o vsockclient vsockclient.o env.o log.o num.o vsock.o $(LDLIBS) +vsockserver: vsockserver.o env.o log.o num.o vsock.o + $(CC) $(LDFLAGS) -o vsockserver vsockserver.o env.o log.o num.o vsock.o $(LDLIBS) -vsockclient.o: env.h log.h util.h vsock.h -vsockserver.o: env.h log.h util.h vsock.h +vsockclient.o: env.h log.h num.h vsock.h +vsockserver.o: env.h log.h num.h vsock.h clean: - rm -f env.o log.o util.o vsock.o \ + rm -f env.o log.o num.o vsock.o \ vsockclient.o vsockclient vsockserver.o vsockserver .PHONY: clean diff --git a/util.c b/num.c similarity index 97% rename from util.c rename to num.c index 2b39df1..ac7f0a4 100644 --- a/util.c +++ b/num.c @@ -3,7 +3,7 @@ #define _GNU_SOURCE -#include "util.h" +#include "num.h" #include <ctype.h> #include <stdlib.h> diff --git a/util.h b/num.h similarity index 100% rename from util.h rename to num.h diff --git a/vsockclient.c b/vsockclient.c index d25ab13..fbdd20e 100644 --- a/vsockclient.c +++ b/vsockclient.c @@ -16,7 +16,7 @@ #include "env.h" #include "log.h" -#include "util.h" +#include "num.h" #include "vsock.h" noreturn static void ex_usage(void) diff --git a/vsockserver.c b/vsockserver.c index 4c251b0..df9d334 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -19,7 +19,7 @@ #include "env.h" #include "log.h" -#include "util.h" +#include "num.h" #include "vsock.h" noreturn static void ex_usage(void) -- 2.30.0

2 2

[PATCH ucspi-vsock] Factor out set_verbosity()
by Alyssa Ross 18 Mar '21

18 Mar '21

This will save some cognitive load when reading main functions. --- log.c | 20 +++++++++++++++++++- log.h | 5 +++++ vsockclient.c | 6 +----- vsockserver.c | 6 +----- 4 files changed, 26 insertions(+), 11 deletions(-) diff --git a/log.c b/log.c index 385d39f..cdfacd6 100644 --- a/log.c +++ b/log.c @@ -1,15 +1,33 @@ // SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2020 Alyssa Ross <hi(a)alyssa.is> +// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi(a)alyssa.is> #define _GNU_SOURCE #include "log.h" #include <err.h> +#include <stdbool.h> #include <stdlib.h> enum verbosity verbosity = errors; +bool set_verbosity(int opt) +{ + switch (opt) { + case 'q': + verbosity = nothing; + return true; + case 'Q': + verbosity = errors; + return true; + case 'v': + verbosity = all; + return true; + } + + return false; +} + void veloge(const char *fmt, va_list args) { if (verbosity) diff --git a/log.h b/log.h index ea9e9cf..7b40e48 100644 --- a/log.h +++ b/log.h @@ -11,6 +11,11 @@ enum verbosity { extern enum verbosity verbosity; +// If opt is a character that matches a standard UCSPI command line +// verbosity option, sets the verbosity appropriately and returns +// true. Otherwise, returns false. +_Bool set_verbosity(int opt); + // Log an error message, followed by strerrno(errno), then exit with // status eval. _Noreturn void diee(int eval, const char *fmt, ...); diff --git a/vsockclient.c b/vsockclient.c index 91e1320..d25ab13 100644 --- a/vsockclient.c +++ b/vsockclient.c @@ -35,13 +35,9 @@ int main(int argc, char *argv[]) while ((opt = getopt(argc, argv, "+qQv")) != -1) { switch (opt) { case 'q': - verbosity = nothing; - break; case 'Q': - verbosity = errors; - break; case 'v': - verbosity = all; + set_verbosity(opt); break; default: ex_usage(); diff --git a/vsockserver.c b/vsockserver.c index 03d9838..4c251b0 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -43,13 +43,9 @@ int main(int argc, char *argv[]) notify = true; break; case 'q': - verbosity = nothing; - break; case 'Q': - verbosity = errors; - break; case 'v': - verbosity = all; + set_verbosity(opt); break; default: ex_usage(); -- 2.30.0

2 2