Re: [PATCH 04/14] nstool: Split some command line parsing and socket setup to subcommands

On Wed, Apr 05, 2023 at 01:58:00PM +0200, Stefano Brivio wrote:

On Tue, 4 Apr 2023 11:46:28 +1000 David Gibson wrote:

...

This will make it easier to differentiate the options to those commands further in future.

Signed-off-by: David Gibson --- test/nstool.c | 102 +++++++++++++++++++++++++++++++++----------------- 1 file changed, 68 insertions(+), 34 deletions(-)

diff --git a/test/nstool.c b/test/nstool.c index 7e069b6..9ea7eeb 100644 --- a/test/nstool.c +++ b/test/nstool.c @@ -11,6 +11,7 @@ #include #include #include +#include #include #include #include @@ -37,19 +38,55 @@ static void usage(void) " terminate.\n"); }

-static void hold(int fd, const struct sockaddr_un *addr) +static int connect_ctl(const char * sockpath, bool wait) { + int fd = socket(AF_UNIX, SOCK_STREAM, PF_UNIX); + struct sockaddr_un addr = { + .sun_family = AF_UNIX, + }; int rc;

- rc = bind(fd, (struct sockaddr *)addr, sizeof(*addr)); + if (fd < 0) + die("socket(): %s\n", strerror(errno));

Unrelated: it would be nice if die() added newlines eventually.

Sure, but as you say unrelated.

...

+ + strncpy(addr.sun_path, sockpath, UNIX_PATH_MAX); + + do { + rc = connect(fd, (struct sockaddr *)&addr, sizeof(addr)); + if (rc < 0 && + (!wait || (errno != ENOENT && errno != ECONNREFUSED))) + die("connect() to %s: %s\n", sockpath, strerror(errno));

A (1ms?) delay would be nice to have here -- it's almost a busyloop, connect() fails fast.

Yeah, I guess so. That's not new, it was already like that in "nsholder pid", so I think something to fix separately.

...

+ } while (rc < 0); + + return fd; +} + +static void cmd_hold(int argc, char *argv[]) +{ + int fd = socket(AF_UNIX, SOCK_STREAM, PF_UNIX); + struct sockaddr_un addr = { + .sun_family = AF_UNIX, + }; + const char *sockpath = argv[1]; + int rc; + + if (argc != 2) + usage(); + + if (fd < 0) + die("socket(): %s\n", strerror(errno)); + + strncpy(addr.sun_path, sockpath, UNIX_PATH_MAX); + + rc = bind(fd, (struct sockaddr *)&addr, sizeof(addr)); if (rc < 0) - die("bind(): %s\n", strerror(errno)); + die("bind() to %s: %s\n", sockpath, strerror(errno));

rc = listen(fd, 0); if (rc < 0) - die("listen(): %s\n", strerror(errno)); + die("listen() on %s: %s\n", sockpath, strerror(errno));

- printf("nstool: local PID=%d local UID=%u local GID=%u\n", + printf("nstool hold: local PID=%d local UID=%u local GID=%u\n", getpid(), getuid(), getgid()); do { int afd = accept(fd, NULL, NULL); @@ -63,71 +100,68 @@ static void hold(int fd, const struct sockaddr_un *addr) die("read(): %s\n", strerror(errno)); } while (rc == 0);

- unlink(addr->sun_path); + unlink(sockpath); }

-static void pid(int fd, const struct sockaddr_un *addr) +static void cmd_pid(int argc, char *argv[]) { - int rc; + const char *sockpath = argv[1]; struct ucred peercred; socklen_t optlen = sizeof(peercred); + int fd, rc;

- do { - rc = connect(fd, (struct sockaddr *)addr, sizeof(*addr)); - if (rc < 0 && errno != ENOENT && errno != ECONNREFUSED) - die("connect(): %s\n", strerror(errno)); - } while (rc < 0); + if (argc != 2) + usage(); + + fd = connect_ctl(sockpath, true);

I didn't spot this earlier, but... does it really make sense to wait in cmd_pid(), also on ENOENT, rather than making 'hold' return only once the socket is ready?

So, this is a consequence of the fact that the holder doesn't move into the background itself - it just sits in the foreground until terminated. That means that the typical usecase puts it into the background from the shell with &, which in turn means that when we reach the next shell command the socket may not be ready - or not even created. One of the things I had in mind for a hypothetical "nstool unshare" would be to avoid this and have it background itself once the socket is ready.

I don't think it would be outrageous to have 'nstool pid' failing if the holding process doesn't exist.

Admittely, I'm biased by the few hundreds of times I needed to 'killall -9 nsholder' in the past months. :)

So... I agree that's irritating, I've done it a similar number of times. However, I don't think that's really related to the question above - in my experience it's always been the holder process that's hung around, not something waiting on a holder.

...

rc = getsockopt(fd, SOL_SOCKET, SO_PEERCRED, &peercred, &optlen); if (rc < 0) - die("getsockopet(SO_PEERCRED): %s\n", strerror(errno)); + die("getsockopet(SO_PEERCRED) %s: %s\n", + sockpath, strerror(errno));

close(fd);

printf("%d\n", peercred.pid); }

-static void stop(int fd, const struct sockaddr_un *addr) +static void cmd_stop(int argc, char *argv[]) { - int rc; + const char *sockpath = argv[1]; + int fd, rc; char buf = 'Q';

- rc = connect(fd, (struct sockaddr *)addr, sizeof(*addr)); - if (rc < 0) - die("connect(): %s\n", strerror(errno)); + if (argc != 2) + usage(); + + fd = connect_ctl(sockpath, false);

rc = write(fd, &buf, sizeof(buf));

Unrelated: a compound literal would make this more readable.

Uh.. I don't see where a compound literal would even go here. -- David Gibson | I'll have my music baroque, and my code david AT gibson.dropbear.id.au | minimalist, thank you. NOT _the_ _other_ | _way_ _around_! http://www.ozlabs.org/~dgibson