Re: [PATCH 2/2] unshare: allow persisting namespaces

[Mike Frysinger <vapier@gentoo.org>]

[-- Attachment #1: Type: text/plain, Size: 3657 bytes --]

On 28 Dec 2014 21:23, Lubomir Rintel wrote:
> Bind mount the namespace file to a given location after creating it if
> requested (analogously to what "ip netns" and other tools do).

since i found out about `ip netns`, i've wanted this in unshare ;).  although 
the two implementations seem to differ: iproute uses a common location 
(/run/netns/$NAME) while this implementation requires specifying the full path 
all the time.  would it be possible to rectify this ?

maybe if you give it a plain name, it defaults to a common location ?  so 
something like this would "just work":
  $ ip netns add foo
  $ unshare --net=foo ...
(yes, i'm aware of `ip netns exec ...`)

using /run/${type}ns/ for all paths seems a bit ugly ... maybe claim 
/run/ns/${type}/ instead ?

> The ugly bit about this patch is the clone(2) call, arguably not our
> fault. The stack size glibc requires for its clone(2) wrapper is not
> documented anywhere and its semantics (stack growth direction) is arch
> dependent. We could figure it out by comparing a return value of a helper
> function that would return an address of its local variable with caller's
> local variable address, but I guess that would be even more messed-up.

are you sure this is strictly a glibc requirement ?  seems like it's mostly 
hardware/ABI related (certainly direction is).  i'd also point out that ia64 
doesn't implement clone either ... it has __clone2().

> +static struct namespace_file {

const

> +	int nstype;
> +	const char *proc_name;
> +	const char *target_name;
> +} namespace_files[] = {
> +	{ .nstype = CLONE_NEWUSER, .proc_name = "ns/user", .target_name = NULL },
> +	{ .nstype = CLONE_NEWIPC,  .proc_name = "ns/ipc",  .target_name = NULL },
> +	{ .nstype = CLONE_NEWUTS,  .proc_name = "ns/uts",  .target_name = NULL },
> +	{ .nstype = CLONE_NEWNET,  .proc_name = "ns/net",  .target_name = NULL },
> +	{ .nstype = CLONE_NEWPID,  .proc_name = "ns/pid",  .target_name = NULL },
> +	{ .nstype = CLONE_NEWNS,   .proc_name = "ns/mnt",  .target_name = NULL },
> +	{ .nstype = 0, .proc_name = NULL, .target_name = NULL }

use ARRAY_SIZE instead and you don't need the sentinel entry

> +int c, forkit = 0, maproot = 0;
> +const char *procmnt = NULL;

static

> +	fputs(_(" -m, --mount[=<file>]      unshare mounts namespace\n"), out);
> +	fputs(_(" -u, --uts[=<file>]        unshare UTS namespace (hostname etc)\n"), out);
> +	fputs(_(" -i, --ipc[=<file>]        unshare System V IPC namespace\n"), out);
> +	fputs(_(" -n, --net[=<file>]        unshare network namespace\n"), out);
> +	fputs(_(" -p, --pid[=<file>]        unshare pid namespace\n"), out);
> +	fputs(_(" -U, --user[=<file>]       unshare user namespace\n"), out);

probably want <path> instead of <file> since it can be either

> +static void persist_ns(pid_t pid)
> +{
> +	struct namespace_file *nsfile;
> +
> +	for (nsfile = namespace_files; nsfile->nstype; nsfile++) {
> +		char pathbuf[PATH_MAX];
> +
> +		if (!nsfile->target_name)
> +			continue;
> +
> +		snprintf(pathbuf, sizeof(pathbuf), "/proc/%u/%s", pid,
> +			nsfile->proc_name);

use xasprintf to avoid the PATH_MAX constant

> +		if (-1 == mknod(nsfile->target_name, 0666, 0)) {
> +			warn(_("failed to create %s"), nsfile->target_name);
> +			continue;
> +		}
> +
> +		if (-1 == mount(pathbuf, nsfile->target_name, NULL, MS_BIND, NULL)) {
> +			warn(_("mount %s failed"), nsfile->target_name);
> +			unlink(nsfile->target_name);

generally the codebase uses the other style -- constants go on the right

> +static int in_child (void *arg)

no space before the (
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]