* Broken threading fix
2021-03-19 2:53 ` [PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross
@ 2021-03-19 3:23 Alyssa Ross
2021-03-19 2:56 ` [PATCH ucspi-vsock 1/7] vsock: get cid and port instead of just cid Alyssa Ross
` (6 more replies)
0 siblings, 7 replies; 15+ messages in thread
From: Alyssa Ross @ 2021-03-19 3:23 UTC (permalink / raw)
To: devel
[-- Attachment #1: Type: text/plain, Size: 381 bytes --]
This message is a clever attempt to fix the broken threading between the
cover letter and the patches.
The patches were accidentally sent as replies to a non-existent
message. I'm using that Message-ID as the Message-ID for this email,
and setting it as a reply to the cover letter, to hopefully link the two
threads back together.
Sorry for the interruption! Hope this works.
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 832 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread* [PATCH ucspi-vsock 1/7] vsock: get cid and port instead of just cid 2021-03-19 3:23 Broken threading fix Alyssa Ross @ 2021-03-19 2:56 ` Alyssa Ross 2021-03-19 2:53 ` [PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 2/7] vsock: check socket family before reading sockaddr Alyssa Ross ` (5 subsequent siblings) 6 siblings, 1 reply; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:56 UTC (permalink / raw) To: devel We can get both in the same system call, so we might as well, rather than having two different functions that make the system call twice. --- When we introduce vsockserverd, it will have to be able to figure out both of these values for the socket it is given to be able to set VSOCKLOCALCID and VSOCKLOCALPORT. vsock.c | 9 ++++++--- vsock.h | 5 +++-- vsockserver.c | 2 +- 3 files changed, 10 insertions(+), 6 deletions(-) diff --git a/vsock.c b/vsock.c index e6a173c..99945c3 100644 --- a/vsock.c +++ b/vsock.c @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2020 Alyssa Ross <hi@alyssa.is> +// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is> #define _GNU_SOURCE @@ -62,7 +62,7 @@ int vsock_open(uint32_t cid, uint32_t port) return fd; } -int vsock_get_port(int fd, uint32_t *port) +int vsock_get_cid_and_port(int fd, uint32_t *cid, uint32_t *port) { struct sockaddr_vm addr; socklen_t addrlen = sizeof addr; @@ -70,7 +70,10 @@ int vsock_get_port(int fd, uint32_t *port) if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) return -1; - *port = addr.svm_port; + if (cid) + *cid = addr.svm_cid; + if (port) + *port = addr.svm_port; return 0; } diff --git a/vsock.h b/vsock.h index e7d66c9..e7ffd62 100644 --- a/vsock.h +++ b/vsock.h @@ -1,5 +1,5 @@ // SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2020 Alyssa Ross <hi@alyssa.is> +// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is> #include <stdint.h> @@ -9,4 +9,5 @@ int vsock_accept(int sockfd, uint32_t *cid, uint32_t *port); int vsock_connect(int fd, uint32_t cid, uint32_t port); int vsock_open(uint32_t cid, uint32_t port); -int vsock_get_port(int fd, uint32_t *port); +// `cid' and `port' can be null. +int vsock_get_cid_and_port(int fd, uint32_t *cid, uint32_t *port); diff --git a/vsockserver.c b/vsockserver.c index df9d334..58cd063 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -80,7 +80,7 @@ int main(int argc, char *argv[]) diee(EX_OSERR, "listen"); if (lport == VMADDR_PORT_ANY) - if (vsock_get_port(fd, &lport) == -1) + if (vsock_get_cid_and_port(fd, NULL, &lport) == -1) diee(EX_OSERR, "getsockname"); if (notify) { -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd @ 2021-03-19 2:53 ` Alyssa Ross 2021-03-19 3:19 ` Alyssa Ross 0 siblings, 1 reply; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:53 UTC (permalink / raw) To: devel 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 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-19 2:53 ` [PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross @ 2021-03-19 3:19 ` Alyssa Ross 0 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 3:19 UTC (permalink / raw) To: devel [-- Attachment #1: Type: text/plain, Size: 2283 bytes --] Alyssa Ross <hi@alyssa.is> writes: > 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 Whoops, I messed up the threading. Implementation begins here: https://spectrum-os.org/lists/archives/spectrum-devel/20210319025648.17925-1-hi@alyssa.is/ [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 832 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 2/7] vsock: check socket family before reading sockaddr 2021-03-19 3:23 Broken threading fix Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 1/7] vsock: get cid and port instead of just cid Alyssa Ross @ 2021-03-19 2:56 ` Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 3/7] vsockserver: switch to a non-blocking socket Alyssa Ross ` (4 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:56 UTC (permalink / raw) To: devel Extracting a helper function for this has the nice side effect of making the `cid' and `port' parameters to vsock_accept nullable, which is nice for consistency with vsock_get_cid_and_port. --- This didn't matter so much in the world where the same program was responsible for both creating the socket and accepting connections on it, but now that we're splitting those up it's important that vsockserverd validates its input. vsock.c | 31 +++++++++++++++++++++++-------- vsock.h | 11 ++++++++++- 2 files changed, 33 insertions(+), 9 deletions(-) diff --git a/vsock.c b/vsock.c index 99945c3..6f1f466 100644 --- a/vsock.c +++ b/vsock.c @@ -5,6 +5,7 @@ #include "vsock.h" +#include <errno.h> #include <sys/ioctl.h> #include <sys/socket.h> @@ -17,6 +18,25 @@ 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) +{ + // Check that this sockaddr info is actually for the socket + // type we think it is, or we could get some very confusing + // data out of it. + if (addr->svm_family != AF_VSOCK) { + errno = EPROTOTYPE; + return -1; + } + + if (cid) + *cid = addr->svm_cid; + if (port) + *port = addr->svm_port; + + return 0; +} + int vsock_bind(int fd, uint32_t cid, uint32_t port) { struct sockaddr_vm addr = { 0 }; @@ -37,8 +57,8 @@ int vsock_accept(int sockfd, uint32_t *cid, uint32_t *port) if ((fd = accept(sockfd, (struct sockaddr *)&addr, &addr_size)) == -1) return -1; - *cid = addr.svm_cid; - *port = addr.svm_port; + if (fill_cid_and_port(&addr, cid, port) == -1) + return -1; return fd; } @@ -70,10 +90,5 @@ int vsock_get_cid_and_port(int fd, uint32_t *cid, uint32_t *port) if (getsockname(fd, (struct sockaddr *)&addr, &addrlen) == -1) return -1; - if (cid) - *cid = addr.svm_cid; - if (port) - *port = addr.svm_port; - - return 0; + return fill_cid_and_port(&addr, cid, port); } diff --git a/vsock.h b/vsock.h index e7ffd62..0b7d157 100644 --- a/vsock.h +++ b/vsock.h @@ -3,11 +3,20 @@ #include <stdint.h> +// All functions taking `cid'/`port' output parameters will fail with +// EPROTOTYPE if the given file descriptor is not an AF_VSOCK socket. +// The socket is left in an undefined state after this. +// +// `cid'/`port' output parameters can be NULL if the information is +// not required. +// +// A return value of -1 indicates failure. `errno' can be consulted for +// further information. + int vsock_bind(int fd, uint32_t cid, uint32_t port); int vsock_accept(int sockfd, uint32_t *cid, uint32_t *port); int vsock_connect(int fd, uint32_t cid, uint32_t port); int vsock_open(uint32_t cid, uint32_t port); -// `cid' and `port' can be null. int vsock_get_cid_and_port(int fd, uint32_t *cid, uint32_t *port); -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 3/7] vsockserver: switch to a non-blocking socket 2021-03-19 3:23 Broken threading fix Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 1/7] vsock: get cid and port instead of just cid Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 2/7] vsock: check socket family before reading sockaddr Alyssa Ross @ 2021-03-19 2:56 ` Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 4/7] configure: create, to generate config.h Alyssa Ross ` (3 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:56 UTC (permalink / raw) To: devel This is entirely an implementation detail, for the moment, but it becomes important when we split out vsockserver into two seperate programs, one for binding the socket, and one for accepting, like s6-networking. Non-blocking sockets are the default there, so it makes sense to share that behaviour. I added the comment about blockingness of the connection sockets just in case, but for now Linux is the only Unix with an AF_VSOCK implementation, so I don't expect it to become relevant any time soon. --- vsockserver.c | 20 +++++++++++++++++--- 1 file changed, 17 insertions(+), 3 deletions(-) diff --git a/vsockserver.c b/vsockserver.c index 58cd063..43307d2 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -4,7 +4,9 @@ #define _GNU_SOURCE #include <errno.h> +#include <fcntl.h> #include <inttypes.h> +#include <poll.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> @@ -33,7 +35,7 @@ noreturn static void ex_usage(void) int main(int argc, char *argv[]) { bool notify = false; - int opt, conn; + int opt; pid_t child; uint32_t lcid, lport, rcid, rport; @@ -73,6 +75,9 @@ int main(int argc, char *argv[]) if (fd == -1) diee(EX_OSERR, "socket"); + if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1) + diee(EX_OSERR, "fcntl"); + if (vsock_bind(fd, lcid, lport) == -1) diee(EX_OSERR, "bind"); @@ -93,7 +98,16 @@ int main(int argc, char *argv[]) ilog("listening as %" PRIu32 " on port %" PRIu32, lcid, lport); - while ((conn = vsock_accept(fd, &rcid, &rport)) != -1) { + struct pollfd poll_fd = { .fd = fd, .events = POLL_IN, .revents = 0 }; + while (poll(&poll_fd, 1, -1) != -1) { + // On Linux, conn will be blocking. On other + // platforms, this may not be the case. If other + // platforms are to be supported, we'd probably want + // to set O_NONBLOCK here. + int conn = vsock_accept(fd, &rcid, &rport); + if (conn == -1) + diee(EX_OSERR, "accept"); + setenvf("VSOCKREMOTECID", 1, "%" PRIu32, rcid); setenvf("VSOCKREMOTEPORT", 1, "%" PRIu32, rport); @@ -117,5 +131,5 @@ int main(int argc, char *argv[]) close(conn); } - diee(EX_OSERR, "accept"); + diee(EX_OSERR, "poll"); } -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 4/7] configure: create, to generate config.h 2021-03-19 3:23 Broken threading fix Alyssa Ross ` (2 preceding siblings ...) 2021-03-19 2:56 ` [PATCH ucspi-vsock 3/7] vsockserver: switch to a non-blocking socket Alyssa Ross @ 2021-03-19 2:56 ` Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 5/7] log: add die function Alyssa Ross ` (2 subsequent siblings) 6 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:56 UTC (permalink / raw) To: devel This will allow programs to refer to BINDIR to find other ucspi-vsock programs, which allows programs to be implemented in terms of each other. --- This is how the new vsockserver program will know where to find vsockserver-socketbinder and vsockserverd. .gitignore | 5 ++++- Makefile => Makefile.in | 12 ++++++++-- configure | 49 +++++++++++++++++++++++++++++++++++++++++ 3 files changed, 63 insertions(+), 3 deletions(-) rename Makefile => Makefile.in (78%) create mode 100755 configure diff --git a/.gitignore b/.gitignore index a63eff4..89a0408 100644 --- a/.gitignore +++ b/.gitignore @@ -1,6 +1,9 @@ # SPDX-License-Identifier: GPL-2.0-or-later -# SPDX-FileCopyrightText: 2020 Alyssa Ross <hi@alyssa.is> +# SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is> *.o +*.tmp vsockclient vsockserver +config.h +Makefile diff --git a/Makefile b/Makefile.in similarity index 78% rename from Makefile rename to Makefile.in index e05e32f..3260e85 100644 --- a/Makefile +++ b/Makefile.in @@ -7,8 +7,8 @@ CFLAGS = -Wall -Wextra -O -g INSTALL = install INSTALL_PROGRAM = $(INSTALL) -prefix = /usr/local -bindir = $(prefix)/bin +prefix = @PREFIX@ +bindir = @BINDIR@ PROGRAMS = vsockclient vsockserver @@ -20,6 +20,10 @@ install: $(PROGRAMS) $(INSTALL_PROGRAM) $(PROGRAMS) $(DESTDIR)$(bindir) .PHONY: install +config.h: configure + @echo "Error: config.h is outdated. Please re-run ./configure." >&2 + @exit 1 + vsockclient: vsockclient.o env.o log.o num.o vsock.o $(CC) $(LDFLAGS) -o $@ $@.o env.o log.o num.o vsock.o $(LDLIBS) vsockserver: vsockserver.o env.o log.o num.o vsock.o @@ -31,3 +35,7 @@ vsockserver.o: env.h log.h num.h vsock.h clean: rm -f $(PROGRAMS) *.o .PHONY: clean + +distclean: clean + rm -f config.h Makefile *.tmp +.PHONY: distclean diff --git a/configure b/configure new file mode 100755 index 0000000..38c926c --- /dev/null +++ b/configure @@ -0,0 +1,49 @@ +#!/bin/sh + +# SPDX-License-Identifier: GPL-2.0-or-later +# SPDX-FileCopyrightText: 2021 Alyssa Ross <hi@alyssa.is> + +set -ue + +prefix=/usr/local +bindir= + +unrecognized= + +for arg; do + if [ "$arg" = "--help" ]; then + cat <<EOF +Usage: $0 [OPTION]..." + +Options: + -h, --help + --prefix=PREFIX Default: /usr/local + --bindir=BINDIR Default: PREFIX/bin +EOF + exit + fi +done + +for arg; do + if printf "%s" "$arg" | grep -q "^--prefix="; then + prefix="$(printf "%s" "$arg" | cut -d= -f2-)" + elif printf "%s" "$arg" | grep -q "^--bindir="; then + bindir="$(printf "%s" "$arg" | cut -d= -f2-)" + else + unrecognized="$unrecognized $arg" + fi +done + +bindir="${bindir:-$prefix/bin}" + +echo "// Generated by $0${*:+ $*}" > config.h.tmp +echo "#define PREFIX \"$prefix\"" >> config.h.tmp +echo "#define BINDIR \"$bindir\"" >> config.h.tmp +mv config.h.tmp config.h + +sed -e "s#@PREFIX@#$prefix#g" -e "s#@BINDIR@#$bindir#g" Makefile.in > Makefile.tmp +mv Makefile.tmp Makefile + +if [ -n "$unrecognized" ]; then + echo "Warning: unrecognized options:$unrecognized" >&2 +fi -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 5/7] log: add die function 2021-03-19 3:23 Broken threading fix Alyssa Ross ` (3 preceding siblings ...) 2021-03-19 2:56 ` [PATCH ucspi-vsock 4/7] configure: create, to generate config.h Alyssa Ross @ 2021-03-19 2:56 ` Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 6/7] exec: add execzp() Alyssa Ross 2021-03-19 3:17 ` [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross 6 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:56 UTC (permalink / raw) To: devel This is useful if we want to provide a more specific error message than strerror can provide. --- log.c | 10 ++++++++++ log.h | 8 ++++---- 2 files changed, 14 insertions(+), 4 deletions(-) diff --git a/log.c b/log.c index cdfacd6..8a57105 100644 --- a/log.c +++ b/log.c @@ -62,6 +62,16 @@ void ilog(const char *fmt, ...) va_end(ap); } +void die(int eval, const char *fmt, ...) +{ + va_list ap; + va_start(ap, fmt); + velog(fmt, ap); + va_end(ap); + + exit(eval); +} + void diee(int eval, const char *fmt, ...) { va_list ap; diff --git a/log.h b/log.h index 7b40e48..706d42f 100644 --- a/log.h +++ b/log.h @@ -11,20 +11,20 @@ enum verbosity { extern enum verbosity verbosity; +// Functions with an `e' suffix additionally print strerrno(errno). + // 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. +// Log an error message then exit with status eval. +_Noreturn void die(int eval, const char *fmt, ...); _Noreturn void diee(int eval, const char *fmt, ...); // Log an error message. void elog(const char *fmt, ...); void velog(const char *fmt, va_list args); - -// Log an error message, followed by strerror(errno). void veloge(const char *fmt, va_list args); // Log an informative message. -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 6/7] exec: add execzp() 2021-03-19 3:23 Broken threading fix Alyssa Ross ` (4 preceding siblings ...) 2021-03-19 2:56 ` [PATCH ucspi-vsock 5/7] log: add die function Alyssa Ross @ 2021-03-19 2:56 ` Alyssa Ross 2021-03-19 3:17 ` [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross 6 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 2:56 UTC (permalink / raw) To: devel --- This will be used by the new version of vsockserver, which will use an argz vector to build up the command line it will exec into. exec.c | 22 ++++++++++++++++++++++ exec.h | 7 +++++++ 2 files changed, 29 insertions(+) create mode 100644 exec.c create mode 100644 exec.h diff --git a/exec.c b/exec.c new file mode 100644 index 0000000..cc55200 --- /dev/null +++ b/exec.c @@ -0,0 +1,22 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2021 Alyssa Ross <hi@alyssa.is> + +#define _GNU_SOURCE + +#include <argz.h> +#include <stdlib.h> +#include <unistd.h> + +#include "exec.h" + +// Like execvp, but takes an argz vector instead of an argv array. +int execzp(const char *file, const char *argz, size_t len) +{ + char **argv = calloc(argz_count(argz, len) + 1, sizeof(char *)); + if (!argv) + return -1; + + argz_extract(argz, len, argv); + + return execvp(file, argv); +} diff --git a/exec.h b/exec.h new file mode 100644 index 0000000..261e113 --- /dev/null +++ b/exec.h @@ -0,0 +1,7 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2021 Alyssa Ross <hi@alyssa.is> + +#include <stddef.h> + +// Like execvp, but takes an argz vector instead of an argv array. +int execzp(const char *file, const char *argz, size_t len); -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-19 3:23 Broken threading fix Alyssa Ross ` (5 preceding siblings ...) 2021-03-19 2:56 ` [PATCH ucspi-vsock 6/7] exec: add execzp() Alyssa Ross @ 2021-03-19 3:17 ` Alyssa Ross 2021-03-19 3:39 ` Cole Helbling 6 siblings, 1 reply; 15+ messages in thread From: Alyssa Ross @ 2021-03-19 3:17 UTC (permalink / raw) To: devel vsockserver-socketbinder creates and listens on a socket, and vsockserverd accepts connections and sets up file descriptors. vsockserver previously did both of these things in one big program, but now it just sets up a command line to run vsockserver-socketbinder followed by vsockserverd. Having two seperate programs allows one program to be used in situations where the other is not suitable (e.g. using vsockserver-socketbinder to create a socket in situations where accept behaviour more complex than vsockserverd can provide is required). This design is taken from s6[1], which uses the same design for its s6-ipcserver, s6-ipcserver-socketbinder, and s6-ipcserverd programs. [1]: https://skarnet.org/software/s6/ --- Because vsockserver.c is completely rewritten in this patch, the diff generated by git was a bit confusing, so the diff here has been lovingly hand-edited to group the hunks together more and make it easy to see the new code all at once. Being able to do this is one of the nice things about email patches. ;) The new implementations are much more thoroughly commented. Think that's my new style. :) .gitignore | 2 + Makefile.in | 14 ++- vsockserver-socketbinder.c | 86 ++++++++++++++++++ vsockserver.c | 149 ++++++++++++++------------------ vsockserver.c => vsockserverd.c | 65 ++++++-------- 5 files changed, 186 insertions(+), 130 deletions(-) create mode 100644 vsockserver-socketbinder.c copy vsockserver.c => vsockserverd.c (64%) diff --git a/.gitignore b/.gitignore index 89a0408..d29518a 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,8 @@ *.o *.tmp vsockclient +vsockserver-socketbinder +vsockserverd vsockserver config.h Makefile diff --git a/Makefile.in b/Makefile.in index 3260e85..0e65f1f 100644 --- a/Makefile.in +++ b/Makefile.in @@ -10,7 +10,7 @@ INSTALL_PROGRAM = $(INSTALL) prefix = @PREFIX@ bindir = @BINDIR@ -PROGRAMS = vsockclient vsockserver +PROGRAMS = vsockclient vsockserver-socketbinder vsockserverd vsockserver all: $(PROGRAMS) .PHONY: all @@ -26,11 +26,17 @@ config.h: configure vsockclient: vsockclient.o env.o log.o num.o vsock.o $(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 $@ $@.o env.o log.o num.o vsock.o $(LDLIBS) +vsockserver-socketbinder: vsockserver-socketbinder.o log.o num.o vsock.o + $(CC) $(LDFLAGS) -o $@ $@.o log.o num.o vsock.o $(LDLIBS) +vsockserverd: vsockserverd.o env.o log.o vsock.o + $(CC) $(LDFLAGS) -o $@ $@.o env.o log.o vsock.o $(LDLIBS) +vsockserver: vsockserver.o exec.o log.o + $(CC) $(LDFLAGS) -o $@ $@.o exec.o log.o $(LDLIBS) vsockclient.o: env.h log.h num.h vsock.h -vsockserver.o: env.h log.h num.h vsock.h +vsockserver-socketbinder.o: log.h num.h vsock.h +vsockserverd.o: env.h log.h vsock.h +vsockserver.o: config.h exec.h log.h clean: rm -f $(PROGRAMS) *.o diff --git a/vsockserver-socketbinder.c b/vsockserver-socketbinder.c new file mode 100644 index 0000000..598c01c --- /dev/null +++ b/vsockserver-socketbinder.c @@ -0,0 +1,86 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is> + +#define _GNU_SOURCE + +#include <errno.h> +#include <fcntl.h> +#include <stdio.h> +#include <stdlib.h> +#include <stdint.h> +#include <stdnoreturn.h> +#include <string.h> +#include <sysexits.h> +#include <sys/socket.h> +#include <unistd.h> + +#include <linux/vm_sockets.h> + +#include "log.h" +#include "num.h" +#include "vsock.h" + +noreturn static void ex_usage(void) +{ + if (verbosity) + fprintf(stderr, "Usage: %s cid port prog...\n", + program_invocation_short_name); + exit(EX_USAGE); +} + +int main(int argc, char *argv[]) +{ + int opt, fd; + uint32_t cid, port; + + // A skeleton of an option parser to reject any options that + // are given, so we can add options in the future without + // worrying about breaking backwards compatibility because + // they were previously interpreted as a first argument. + while ((opt = getopt(argc, argv, "+")) != -1) { + switch (opt) { + default: + ex_usage(); + } + } + + // Check there are enough positional arguments (two for the + // address and at least one to exec into). + if (optind > argc - 3) + ex_usage(); + + // Parse the `cid' argument. + if (!strcmp(argv[optind], "-1")) + cid = VMADDR_CID_ANY; + else if (getu32(argv[optind], 0, UINT32_MAX, &cid)) + ex_usage(); + optind++; + + // Parse the `port' argument. + if (!strcmp(argv[optind], "-1")) + port = VMADDR_PORT_ANY; + else if (getu32(argv[optind], 0, UINT32_MAX, &port)) + ex_usage(); + optind++; + + // Set up the socket. + fd = socket(AF_VSOCK, SOCK_STREAM, 0); + if (fd == -1) + diee(EX_OSERR, "socket"); + if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1) + diee(EX_OSERR, "fcntl"); + if (vsock_bind(fd, cid, port) == -1) + diee(EX_OSERR, "bind"); + if (listen(fd ,40) == -1) + diee(EX_OSERR, "listen"); + + // Place the socket at stdout. + if (dup2(fd, STDIN_FILENO) == -1) + diee(EX_OSERR, "dup2"); + if (fd != STDIN_FILENO) + close(fd); + + // Finally, exec into `prog'. + execvp(argv[optind], &argv[optind]); + diee(EX_OSERR, "execvp"); +} diff --git a/vsockserver.c b/vsockserver.c index 43307d2..f740a8a 100644 --- a/vsockserver.c +++ b/vsockserver.c @@ -1,28 +1,20 @@ // SPDX-License-Identifier: GPL-2.0-or-later -// SPDX-FileCopyrightText: 2020-2021 Alyssa Ross <hi@alyssa.is> +// SPDX-FileCopyrightText: 2021 Alyssa Ross <hi@alyssa.is> #define _GNU_SOURCE +#include <argz.h> #include <errno.h> -#include <fcntl.h> -#include <inttypes.h> -#include <poll.h> #include <stdbool.h> #include <stdio.h> #include <stdlib.h> #include <stdnoreturn.h> -#include <string.h> -#include <sys/socket.h> -#include <sys/wait.h> #include <sysexits.h> #include <unistd.h> -#include <linux/vm_sockets.h> - -#include "env.h" +#include "config.h" +#include "exec.h" #include "log.h" -#include "num.h" -#include "vsock.h" noreturn static void ex_usage(void) { @@ -34,102 +26,87 @@ noreturn static void ex_usage(void) int main(int argc, char *argv[]) { - bool notify = false; int opt; - pid_t child; - uint32_t lcid, lport, rcid, rport; + bool alloc_failed = false; + size_t binder_opts_len = 0, daemon_opts_len = 0; + char *binder_opts = NULL, *daemon_opts = NULL; + + // The heavy lifting here is done by vsockserver-socketbinder + // and vsockserverd. All this program does is munge argv + // appropriately to set the right options for each of those + // programs, and then exec into vsockserver-socketbinder. + + // If allocation fails, we need to keep going until after + // 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"); while ((opt = getopt(argc, argv, "+1qQv")) != -1) { + char *arg = NULL; + switch (opt) { case '1': - notify = true; - break; case 'q': case 'Q': case 'v': set_verbosity(opt); + alloc_failed |= + asprintf(&arg, "-%c", opt) == -1 || + argz_add(&daemon_opts, &daemon_opts_len, arg); + free(arg); break; default: ex_usage(); } } + // Now that verbosity is set, we can whether we were sitting + // on an allocation failure. + if (alloc_failed) + diee(EX_OSERR, "malloc"); + // Now we don't have to keep checking alloc_failed before + // doing anything, and we can deal with allocation failures + // after this point by just terminating immediately. + - // Check there are enough positional arguments (two for the - // address and at least one to exec into). + // Check there are `cid' and `port' arguments to pass to + // vsockserver-socketbinder, and at least one `prog' argument + // to pass to vsockserverd. if (optind > argc - 3) ex_usage(); - if (!strcmp(argv[optind], "-1")) - lcid = VMADDR_CID_ANY; - else if (getu32(argv[optind], 0, UINT32_MAX, &lcid)) - ex_usage(); - optind++; - - if (!strcmp(argv[optind], "-1")) - lport = VMADDR_PORT_ANY; - else if (getu32(argv[optind], 0, UINT32_MAX, &lport)) - ex_usage(); - optind++; - - int fd = socket(AF_VSOCK, SOCK_STREAM, 0); - if (fd == -1) - diee(EX_OSERR, "socket"); - - if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1) - diee(EX_OSERR, "fcntl"); - - if (vsock_bind(fd, lcid, lport) == -1) - diee(EX_OSERR, "bind"); - - if (listen(fd, 40) == -1) - diee(EX_OSERR, "listen"); - - if (lport == VMADDR_PORT_ANY) - if (vsock_get_cid_and_port(fd, NULL, &lport) == -1) - diee(EX_OSERR, "getsockname"); - - if (notify) { - printf("%" PRIu32 "\n", lport); - fclose(stdout); - } - - setenvf("VSOCKLOCALCID", 1, "%" PRIu32, lcid); - setenvf("VSOCKLOCALPORT", 1, "%" PRIu32, lport); - - ilog("listening as %" PRIu32 " on port %" PRIu32, lcid, lport); - - struct pollfd poll_fd = { .fd = fd, .events = POLL_IN, .revents = 0 }; - while (poll(&poll_fd, 1, -1) != -1) { - // On Linux, conn will be blocking. On other - // platforms, this may not be the case. If other - // platforms are to be supported, we'd probably want - // to set O_NONBLOCK here. - int conn = vsock_accept(fd, &rcid, &rport); - if (conn == -1) - diee(EX_OSERR, "accept"); - - setenvf("VSOCKREMOTECID", 1, "%" PRIu32, rcid); - setenvf("VSOCKREMOTEPORT", 1, "%" PRIu32, rport); - - ilog("connection from %" PRIu32 " port %" PRIu32, rcid, rport); - - switch (child = fork()) { - case -1: diee(EX_OSERR, "fork"); - case 0: - if (dup2(conn, STDIN_FILENO) == -1) - diee(EX_OSERR, "dup2"); - if (dup2(conn, STDOUT_FILENO) == -1) - diee(EX_OSERR, "dup2"); - if (conn != STDIN_FILENO && conn != STDOUT_FILENO) - close(conn); - execvp(argv[optind], &argv[optind]); - diee(EX_OSERR, "exec"); - } - - if (waitpid(child, NULL, 0) == -1) - diee(EX_OSERR, "waitpid"); - - close(conn); - } - diee(EX_OSERR, "poll"); + // 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++])) + 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)) + diee(EX_OSERR, "malloc"); + free(daemon_opts); + + // Append `prog' to binder_opts. Technically this should be + // part of daemon_opts, but then we'd just be copying it into + // one place to immediately copy it elsewhere. + for (int i = optind; i < argc; i++) + if (argz_add(&binder_opts, &binder_opts_len, argv[i])) + diee(EX_OSERR, "malloc"); + + if (verbosity == all) { + char *opt; + + // Log the full argv before we exec it. + fprintf(stderr, "%s: executing", program_invocation_short_name); + while ((opt = argz_next(binder_opts, binder_opts_len, opt))) + fprintf(stderr, " %s", opt); + fputc('\n', stderr); + } + + execzp(binder_opts, binder_opts, binder_opts_len); + diee(EX_OSERR, "execvp"); } diff --git a/vsockserver.c b/vsockserverd.c similarity index 64% copy from vsockserver.c copy to vsockserverd.c index 43307d2..1abfff9 100644 --- a/vsockserver.c +++ b/vsockserverd.c @@ -4,30 +4,25 @@ #define _GNU_SOURCE #include <errno.h> -#include <fcntl.h> #include <inttypes.h> #include <poll.h> #include <stdbool.h> +#include <stdint.h> #include <stdio.h> #include <stdlib.h> #include <stdnoreturn.h> -#include <string.h> -#include <sys/socket.h> #include <sys/wait.h> #include <sysexits.h> #include <unistd.h> -#include <linux/vm_sockets.h> - #include "env.h" #include "log.h" -#include "num.h" #include "vsock.h" noreturn static void ex_usage(void) { if (verbosity) - fprintf(stderr, "Usage: %s [ -1 ] [ -q | -Q | -v ] cid port prog...\n", + fprintf(stderr, "Usage: %s [ -1 ] [ -q | -Q | -v ] prog...\n", program_invocation_short_name); exit(EX_USAGE); } @@ -38,6 +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 }; while ((opt = getopt(argc, argv, "+1qQv")) != -1) { switch (opt) { @@ -54,60 +51,42 @@ 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) + // Check that there is something for us to exec into. + if (optind > argc - 1) ex_usage(); - if (!strcmp(argv[optind], "-1")) - lcid = VMADDR_CID_ANY; - else if (getu32(argv[optind], 0, UINT32_MAX, &lcid)) - ex_usage(); - optind++; - - if (!strcmp(argv[optind], "-1")) - lport = VMADDR_PORT_ANY; - else if (getu32(argv[optind], 0, UINT32_MAX, &lport)) - ex_usage(); - optind++; - - int fd = socket(AF_VSOCK, SOCK_STREAM, 0); - if (fd == -1) - diee(EX_OSERR, "socket"); - - if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1) - diee(EX_OSERR, "fcntl"); - - if (vsock_bind(fd, lcid, lport) == -1) - diee(EX_OSERR, "bind"); - - if (listen(fd, 40) == -1) - diee(EX_OSERR, "listen"); - - if (lport == VMADDR_PORT_ANY) - if (vsock_get_cid_and_port(fd, NULL, &lport) == -1) - diee(EX_OSERR, "getsockname"); + // Find out the CID and port of the socket. + if (vsock_get_cid_and_port(STDIN_FILENO, &lcid, &lport)) { + if (errno == ENOTSOCK || errno == EPROTOTYPE) + die(EX_USAGE, "stdin is not an AF_VSOCK socket"); + diee(EX_OSERR, "getsockname"); + } + // Notify the user of our port and indicate readiness. if (notify) { printf("%" PRIu32 "\n", lport); fclose(stdout); } + // Set our CID and port as environment variables so `prog' can + // know them. setenvf("VSOCKLOCALCID", 1, "%" PRIu32, lcid); setenvf("VSOCKLOCALPORT", 1, "%" PRIu32, lport); ilog("listening as %" PRIu32 " on port %" PRIu32, lcid, lport); - struct pollfd poll_fd = { .fd = fd, .events = POLL_IN, .revents = 0 }; + // Wait for a connection. while (poll(&poll_fd, 1, -1) != -1) { // On Linux, conn will be blocking. On other // platforms, this may not be the case. If other // platforms are to be supported, we'd probably want // to set O_NONBLOCK here. - int conn = vsock_accept(fd, &rcid, &rport); + int conn = vsock_accept(STDIN_FILENO, &rcid, &rport); if (conn == -1) diee(EX_OSERR, "accept"); + // Set the client's CID and port as environment + // variables so `prog' can know them. setenvf("VSOCKREMOTECID", 1, "%" PRIu32, rcid); setenvf("VSOCKREMOTEPORT", 1, "%" PRIu32, rport); @@ -116,12 +95,18 @@ int main(int argc, char *argv[]) switch (child = 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 + // effect of closing the listening socket in + // the child, since it's also on stdin. if (dup2(conn, STDIN_FILENO) == -1) diee(EX_OSERR, "dup2"); if (dup2(conn, STDOUT_FILENO) == -1) diee(EX_OSERR, "dup2"); if (conn != STDIN_FILENO && conn != STDOUT_FILENO) close(conn); + + // exec into `prog'. execvp(argv[optind], &argv[optind]); diee(EX_OSERR, "exec"); } -- 2.30.0 ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-19 3:17 ` [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross @ 2021-03-19 3:39 ` Cole Helbling 2021-03-20 20:24 ` Alyssa Ross 0 siblings, 1 reply; 15+ messages in thread From: Cole Helbling @ 2021-03-19 3:39 UTC (permalink / raw) To: Alyssa Ross, devel On Thu Mar 18, 2021 at 8:17 PM PDT, Alyssa Ross wrote: > vsockserver-socketbinder creates and listens on a socket, and > vsockserverd accepts connections and sets up file descriptors. > > vsockserver previously did both of these things in one big program, > but now it just sets up a command line to run vsockserver-socketbinder > followed by vsockserverd. Having two seperate programs allows one > program to be used in situations where the other is not > suitable (e.g. using vsockserver-socketbinder to create a socket in > situations where accept behaviour more complex than vsockserverd can > provide is required). > > This design is taken from s6[1], which uses the same design for its > s6-ipcserver, s6-ipcserver-socketbinder, and s6-ipcserverd programs. > > [1]: https://skarnet.org/software/s6/ > --- > > Because vsockserver.c is completely rewritten in this patch, the diff > generated by git was a bit confusing, so the diff here has been > lovingly hand-edited to group the hunks together more and make it easy > to see the new code all at once. Being able to do this is one of the > nice things about email patches. ;) > > The new implementations are much more thoroughly commented. Think > that's my new style. :) > > .gitignore | 2 + > Makefile.in | 14 ++- > vsockserver-socketbinder.c | 86 ++++++++++++++++++ > vsockserver.c | 149 ++++++++++++++------------------ > vsockserver.c => vsockserverd.c | 65 ++++++-------- > 5 files changed, 186 insertions(+), 130 deletions(-) > create mode 100644 vsockserver-socketbinder.c > copy vsockserver.c => vsockserverd.c (64%) [snip] > diff --git a/vsockserver-socketbinder.c b/vsockserver-socketbinder.c > new file mode 100644 > index 0000000..598c01c > --- /dev/null > +++ b/vsockserver-socketbinder.c > @@ -0,0 +1,86 @@ [snip] > +int main(int argc, char *argv[]) > +{ > + int opt, fd; > + uint32_t cid, port; > + > + // A skeleton of an option parser to reject any options that > + // are given, so we can add options in the future without > + // worrying about breaking backwards compatibility because > + // they were previously interpreted as a first argument. > + while ((opt = getopt(argc, argv, "+")) != -1) { > + switch (opt) { > + default: > + ex_usage(); > + } > + } > + > + // Check there are enough positional arguments (two for the > + // address and at least one to exec into). > + if (optind > argc - 3) > + ex_usage(); > + > + // Parse the `cid' argument. > + if (!strcmp(argv[optind], "-1")) > + cid = VMADDR_CID_ANY; > + else if (getu32(argv[optind], 0, UINT32_MAX, &cid)) > + ex_usage(); > + optind++; > + > + // Parse the `port' argument. > + if (!strcmp(argv[optind], "-1")) > + port = VMADDR_PORT_ANY; > + else if (getu32(argv[optind], 0, UINT32_MAX, &port)) > + ex_usage(); > + optind++; > + > + // Set up the socket. > + fd = socket(AF_VSOCK, SOCK_STREAM, 0); > + if (fd == -1) > + diee(EX_OSERR, "socket"); > + if (fcntl(fd, F_SETFL, O_NONBLOCK) == -1) > + diee(EX_OSERR, "fcntl"); > + if (vsock_bind(fd, cid, port) == -1) > + diee(EX_OSERR, "bind"); > + if (listen(fd ,40) == -1) Minor formatting nit (comma, then space); but also, what is `40` representative of? Should this be `#define`d, or otherwise assigned to some descriptive name? > + diee(EX_OSERR, "listen"); > + > + // Place the socket at stdout. > + if (dup2(fd, STDIN_FILENO) == -1) > + diee(EX_OSERR, "dup2"); > + if (fd != STDIN_FILENO) > + close(fd); > + > + // Finally, exec into `prog'. > + execvp(argv[optind], &argv[optind]); > + diee(EX_OSERR, "execvp"); > +} Cole ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-19 3:39 ` Cole Helbling @ 2021-03-20 20:24 ` Alyssa Ross 2021-03-21 2:52 ` Cole Helbling 0 siblings, 1 reply; 15+ messages in thread From: Alyssa Ross @ 2021-03-20 20:24 UTC (permalink / raw) To: Cole Helbling; +Cc: devel [-- Attachment #1: Type: text/plain, Size: 1526 bytes --] On Thu, Mar 18, 2021 at 08:39:31PM -0700, Cole Helbling wrote: > > + if (listen(fd ,40) == -1) > > Minor formatting nit (comma, then space); but also, what is `40` > representative of? Should this be `#define`d, or otherwise assigned to > some descriptive name? Looks like the next thing I should do after this is set up clang-tidy or something. ;) It's the backlog parameter, i.e. the minimum[1] number of connections the kernel should allow to queue up before it starts rejecting them. Would the following additional diff make you happy? :) (I won't bother resending the whole patch with it applied; for the purposes of reviewing just pretend this is part of the patch I sent.) [1]: https://utcc.utoronto.ca/~cks/space/blog/unix/ListenBacklogMeaning PS: cool thing I just discovered -- you can pipe this message to git apply, and it'll find and apply the diff (without trying to make a commit like git am would). diff --git i/vsockserver-socketbinder.c w/vsockserver-socketbinder.c index 598c01c..fdcdfa8 100644 --- i/vsockserver-socketbinder.c +++ w/vsockserver-socketbinder.c @@ -20,6 +20,8 @@ #include "num.h" #include "vsock.h" +static const int LISTEN_BACKLOG = 40; + noreturn static void ex_usage(void) { if (verbosity) @@ -71,7 +73,7 @@ int main(int argc, char *argv[]) diee(EX_OSERR, "fcntl"); if (vsock_bind(fd, cid, port) == -1) diee(EX_OSERR, "bind"); - if (listen(fd ,40) == -1) + if (listen(fd, LISTEN_BACKLOG) == -1) diee(EX_OSERR, "listen"); // Place the socket at stdout. [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-20 20:24 ` Alyssa Ross @ 2021-03-21 2:52 ` Cole Helbling 2021-03-21 14:33 ` Alyssa Ross 0 siblings, 1 reply; 15+ messages in thread From: Cole Helbling @ 2021-03-21 2:52 UTC (permalink / raw) To: Alyssa Ross; +Cc: devel On Sat Mar 20, 2021 at 1:24 PM PDT, Alyssa Ross wrote: > On Thu, Mar 18, 2021 at 08:39:31PM -0700, Cole Helbling wrote: > > > + if (listen(fd ,40) == -1) > > > > Minor formatting nit (comma, then space); but also, what is `40` > > representative of? Should this be `#define`d, or otherwise assigned to > > some descriptive name? > > Looks like the next thing I should do after this is set up clang-tidy or > something. ;) > > It's the backlog parameter, i.e. the minimum[1] number of connections > the kernel should allow to queue up before it starts rejecting them. > > Would the following additional diff make you happy? :) > (I won't bother resending the whole patch with it applied; for the > purposes of reviewing just pretend this is part of the patch I sent.) Yep, looks good to me! Though if I'm being greedy, maybe add the above explanation as a comment above the new variable? ("It's the backlog [...]") Entire series looks good as well: Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com> ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-21 2:52 ` Cole Helbling @ 2021-03-21 14:33 ` Alyssa Ross 2021-03-21 14:38 ` Alyssa Ross 0 siblings, 1 reply; 15+ messages in thread From: Alyssa Ross @ 2021-03-21 14:33 UTC (permalink / raw) To: Cole Helbling; +Cc: devel [-- Attachment #1: Type: text/plain, Size: 1367 bytes --] On Sat, Mar 20, 2021 at 07:52:00PM -0700, Cole Helbling wrote: > On Sat Mar 20, 2021 at 1:24 PM PDT, Alyssa Ross wrote: > > On Thu, Mar 18, 2021 at 08:39:31PM -0700, Cole Helbling wrote: > > > > + if (listen(fd ,40) == -1) > > > > > > Minor formatting nit (comma, then space); but also, what is `40` > > > representative of? Should this be `#define`d, or otherwise assigned to > > > some descriptive name? > > > > Looks like the next thing I should do after this is set up clang-tidy or > > something. ;) > > > > It's the backlog parameter, i.e. the minimum[1] number of connections > > the kernel should allow to queue up before it starts rejecting them. > > > > Would the following additional diff make you happy? :) > > (I won't bother resending the whole patch with it applied; for the > > purposes of reviewing just pretend this is part of the patch I sent.) > > Yep, looks good to me! Though if I'm being greedy, maybe add the above > explanation as a comment above the new variable? ("It's the backlog > [...]") I think I don't want to get too much into explaining things listen(2) will explain better than me (but you were right about giving this number a name)! > Entire series looks good as well: > > Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com> Thanks! To ssh://atuin/home/spectrum/git/ucspi-vsock.git e718a97..a452f16 master -> master [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd 2021-03-21 14:33 ` Alyssa Ross @ 2021-03-21 14:38 ` Alyssa Ross 0 siblings, 0 replies; 15+ messages in thread From: Alyssa Ross @ 2021-03-21 14:38 UTC (permalink / raw) To: Cole Helbling; +Cc: devel [-- Attachment #1: Type: text/plain, Size: 1632 bytes --] On Sun, Mar 21, 2021 at 02:33:14PM +0000, Alyssa Ross wrote: > On Sat, Mar 20, 2021 at 07:52:00PM -0700, Cole Helbling wrote: > > On Sat Mar 20, 2021 at 1:24 PM PDT, Alyssa Ross wrote: > > > On Thu, Mar 18, 2021 at 08:39:31PM -0700, Cole Helbling wrote: > > > > > + if (listen(fd ,40) == -1) > > > > > > > > Minor formatting nit (comma, then space); but also, what is `40` > > > > representative of? Should this be `#define`d, or otherwise assigned to > > > > some descriptive name? > > > > > > Looks like the next thing I should do after this is set up clang-tidy or > > > something. ;) > > > > > > It's the backlog parameter, i.e. the minimum[1] number of connections > > > the kernel should allow to queue up before it starts rejecting them. > > > > > > Would the following additional diff make you happy? :) > > > (I won't bother resending the whole patch with it applied; for the > > > purposes of reviewing just pretend this is part of the patch I sent.) > > > > Yep, looks good to me! Though if I'm being greedy, maybe add the above > > explanation as a comment above the new variable? ("It's the backlog > > [...]") > > I think I don't want to get too much into explaining things listen(2) > will explain better than me (but you were right about giving this > number a name)! > > > Entire series looks good as well: > > > > Reviewed-by: Cole Helbling <cole.e.helbling@outlook.com> > > Thanks! > > To ssh://atuin/home/spectrum/git/ucspi-vsock.git > e718a97..a452f16 master -> master Actually it's e718a97..20a27f1. Realised I hadn't actually applied that diff, and decided to just force push before anybody noticed. ;) [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 833 bytes --] ^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2021-03-21 14:39 UTC | newest] Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2021-03-19 3:23 Broken threading fix Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 1/7] vsock: get cid and port instead of just cid Alyssa Ross 2021-03-19 2:53 ` [PATCH ucspi-vsock 0/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross 2021-03-19 3:19 ` Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 2/7] vsock: check socket family before reading sockaddr Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 3/7] vsockserver: switch to a non-blocking socket Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 4/7] configure: create, to generate config.h Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 5/7] log: add die function Alyssa Ross 2021-03-19 2:56 ` [PATCH ucspi-vsock 6/7] exec: add execzp() Alyssa Ross 2021-03-19 3:17 ` [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd Alyssa Ross 2021-03-19 3:39 ` Cole Helbling 2021-03-20 20:24 ` Alyssa Ross 2021-03-21 2:52 ` Cole Helbling 2021-03-21 14:33 ` Alyssa Ross 2021-03-21 14:38 ` Alyssa Ross
Code repositories for project(s) associated with this public inbox https://spectrum-os.org/git/crosvm https://spectrum-os.org/git/doc https://spectrum-os.org/git/mktuntap https://spectrum-os.org/git/nixpkgs https://spectrum-os.org/git/spectrum https://spectrum-os.org/git/ucspi-vsock https://spectrum-os.org/git/www This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).