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 <[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. > +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 _______________________________________________ Toybox mailing list [email protected] http://lists.landley.net/listinfo.cgi/toybox-landley.net
