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

["Cole Helbling" <cole.e.helbling@outlook.com>]

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