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

6 Apr 2023


      On Thu, Apr 06, 2023 at 08:47:48AM +0200, Stefano Brivio wrote:
...
On Thu, 6 Apr 2023 12:31:55 +1000
David Gibson  wrote:
...
On Wed, Apr 05, 2023 at 01:58:00PM +0200, Stefano Brivio wrote:
[...]
...
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.
Ah, sure, it makes sense now.
...
...
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.
Yes, same here, but it's something I file under the same category (I
don't remember why nsholder would hang, you probably explained at some
point...).
I believe the main reason is because of the holders which are PID 1
within their pid namespaces.  That means that if you interrupt the
tests, the SIGINT or SIGHUP they'll get from tmux etc. shutting down
won't be sufficient to kill them.
...
...
...
...
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.
I meant:
rc = write(fd, &(char){ 'Q' }, 1);
...so that one doesn't need to look at 'buf'. nstool is C99 anyway.
Oh, ok.  On the other hand it means not using sizeof() to get the
length, which isn't ideal.

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