patches and low-level development discussion
 help / color / mirror / code / Atom feed
* [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

* [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 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 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

* 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

* 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).