[Toybox] [PATCH] nsenter: A tool to use setns(2)

[Rob Landley]

I imported the first one, but applied the second as a patch because new
commands go in the "pending" directory so I don't lose track of what
I've fully reviewed yet.

On 10/17/14 22:01, Andy Lutomirski wrote:
> nsenter: A tool to use setns(2)

I don't have this command on my host system, and it's not even in the
python "install this package if you want this command" thing.

> This implements all of the namespace parts of nsenter, but UID and GID
> switching are missing, as are -r and -w (both because they're not strictly
> necessary and because the nsenter manpage has an insufficient
> description of how they work).
> 
> diff -r 5330d3f88fa3 -r 5520248c82b2 toys/other/nsenter.c
> --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
> +++ b/toys/other/nsenter.c	Fri Oct 17 20:00:58 2014 -0700
> @@ -0,0 +1,104 @@
> +/* nsenter.c - Enter existing namespaces
> + *
> + * Copyright 2014 andy Lutomirski <luto at amacapital.net>
> +
> +USE_NSENTER(NEWTOY(nsenter, "<1F(no-fork)t#(target)i:(ipc);m:(mount);n:(net);p:(pid);u:(uts);U:(user);", TOYFLAG_USR|TOYFLAG_BIN))

All the ";" characters mean you can only supply the option to a longopt
with =, as in "--ipc=abc" but "--ipc abc" are considered two separate
arguments (as in "--ipc= abc").

(Because ls --color works that way. Because gnu, apparently.)

I'm guessing this is _not_ the behavior you want? I don't even remember
what I made the short options do in that case, I designed it to apply to
a bare longopt that needed it.

> +config NSENTER
> +  bool "nsenter"
> +  default n
> +  help
> +    usage: nsenter [-t pid] [-F] [-i] [-m] [-n] [-p] [-u] [-U] COMMAND...
> +
> +    Run COMMAND in a different set of namespaces.
> +
> +    -T  PID to take namespaces from

Lower case above, capital here.

> +    -F  don't fork, even if -p is set
> +
> +    The namespaces to switch are:
> +
> +    -i	SysV IPC (message queues, semaphores, shared memory)
> +    -m	Mount/unmount tree
> +    -n	Network address, sockets, routing, iptables
> +    -p	Process IDs and init (will fork unless -F is used)
> +    -u	Host and domain names
> +    -U	UIDs, GIDs, capabilities
> +
> +    Each of those options takes an optional argument giving the path of
> +    the namespace file (usually in /proc).  This optional argument is
> +    mandatory unless -t is used.
> +*/

Lower case again. and I note that the lib/args.c option parsing
infrastructure isn't really designed to work that way. (I can probably
extend it, just... What command are you copying the behavior of?)

> +#define FOR_nsenter
> +#define _GNU_SOURCE

I never do that. This is not a GNU project, it's a Linux project. (If
anything it stands in opposition to gnu.)

The headers have #define _ALL_SOURCE for musl, for the rest I do fixups
in portabilty.h as necessary (and include linux/*.h headers from the
local command, since that doesn't belong in the global headers either).

> +#include "toys.h"
> +#include <errno.h>
> +#include <sched.h>
> +#include <linux/sched.h>

I note toys.h already includes errno.h and sched.h in the standard
headers, and I already did the sched.h namespace dance for this stuff in
unshare.c. I'll probably put this command in that file so they can share
the infrastructure.

> +#define NUM_NSTYPES 6

We have an ARRAY_LEN() macro.

> +struct nstype {
> +  int type;
> +  const char *name;
> +};
> +
> +struct nstype nstypes[NUM_NSTYPES] = {
> +  {CLONE_NEWUSER, "user"}, /* must be first to allow non-root operation */
> +  {CLONE_NEWUTS,  "uts"},
> +  {CLONE_NEWPID,  "pid"},
> +  {CLONE_NEWNET,  "net"},
> +  {CLONE_NEWNS,   "mnt"},
> +  {CLONE_NEWIPC,  "ipc"},
> +};

If you leave the array's [] empty it'll automatically allocate space for
the number of entries you provide, and then you can ARRAY_LEN to see the
length.

That said, if unhare already has an array of these symbols we can reuse
that and then ahve the names just be a "user\0uts\0pid\0net\0mnt\ipc"
string.

Also, I try not to define global data outside the GLOBALS block. If it
needs to be initialized, I do that in the function's main(). (There are
times when having static global data is sufficiently cheaper to be worth
it, but it's not my first choice. Finding a way to avoid needing it is
my first choice.)

(P.S. I can do this cleanup, just explaining conceptually how I'd
simplify it.)

> +GLOBALS(
> +  char *nsnames[6];
> +  long targetpid;
> +)

You have a macro above, and a hardwired 6 here. (I know the macro
doesn't apply to globals, but if you're willing to hardwire 6 here
having the macro at all is not a net win.)

> +static void enter_by_name(int idx)
> +{

This function has exactly one caller, and can be inlined at that
callsite. Doing so often reveals opportunities for further simplification.

> +  int fd, rc;
> +  char buf[64];

We have "toybuf", a static 4k global char array, available to all
commands. (And there's another one "libbuf" for library code.)

> +  char *filename = TT.nsnames[idx];
> +
> +  if (!(toys.optflags & (1<<idx))) return;
> +
> +  if (!filename || !*filename) {

Ok, I'll bite: where did TT.nsnames[] get set to anything other than
NULL, so that filename would not be NULL here? (The GLOBALS block starts
zeroed.)

Sorry to trail off halfway through, I have to go run an errand before a
place closes...

Rob

 1413755336.0