On Sun, Oct 19, 2014 at 2:48 PM, Rob Landley <[email protected]> wrote: > 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.
It's in util-linux. I bet you're using Ubuntu or Debian :) Except for very new Debians (IIRC), they're both quite a few years behind on util-linux updates. > >> 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 <[email protected]> >> + >> +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. I think this is the behavior I want. I haven't checked exactly for longopts, but, for the short form, the behavior seems to match util-linux's. > >> +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. Whoops. Lowercase is correct; the "-T" is wrong. > >> + -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.) That should have been const, in which case I think it makes sense to have it outside GLOBALS. But I'll defer to your proposed tinier version. > > (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.) At least it reduces the number of them. :-/ > >> +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. I wrote it this way because I might want to split the loop if I ever implement the setuid stuff. But, on second thought, that might not be necessary. > >> + 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.) I think that happened when I did toybox nsenter -t PID -U command. This should be interpreted as -U with no argument followed by a command, and filename ended up being the empty string. --Andy _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
