Re: [PATCH ucspi-vsock 7/7] Extract vsockserver-socketbinder and vsockserverd

Cole Helbling
2021-03-19T03:48:11Z

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