[PATCH 0/5] nsenter,unshare: small usability improvements

[PATCH 0/5] nsenter,unshare: small usability improvements

[Zbigniew Jędrzejewski-Szmek]

Hi,

those patches are ON TOP OF SAMI KREOLA'S SERIES
(nsenter: trivial adjustments).

Zbigniew Jędrzejewski-Szmek (5):
  nsenter: allow arguments to be specified in any order
  nsenter: add --all meaning all namespaces and cwd and root
  nsenter: respect --exec no matter where it appears
  nsenter: rename --exec/-e to --no-fork/-F
  unshare: add --all meaning all namespaces

 sys-utils/nsenter.1 |   7 +++-
 sys-utils/nsenter.c | 104 ++++++++++++++++++++++++++++++++++++++++------------
 sys-utils/unshare.1 |   3 ++
 sys-utils/unshare.c |   9 ++++-
 4 files changed, 98 insertions(+), 25 deletions(-)

-- 
1.7.11.7

[PATCH 1/5] nsenter: allow arguments to be specified in any order

[Zbigniew Jędrzejewski-Szmek]

Allows 'nsenter -mt $PID', which would fail previously.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 sys-utils/nsenter.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index 04ac314..1b57cc5 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -25,6 +25,7 @@
 #include <sched.h>
 #include <stdio.h>
 #include <stdlib.h>
+#include <stdbool.h>
 #include <unistd.h>
 #include <assert.h>
 
@@ -174,7 +175,8 @@ int main(int argc, char *argv[])
 
 	struct namespace_file *nsfile;
 	int do_fork = 0;
-	int c;
+	int c, namespaces = 0;
+	bool do_rd = false, do_wd = false;
 
 	setlocale(LC_MESSAGES, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
@@ -195,32 +197,56 @@ int main(int argc, char *argv[])
 			    strtoul_or_err(optarg, _("failed to parse pid"));
 			break;
 		case 'm':
-			open_namespace_fd(CLONE_NEWNS, optarg);
+			if (optarg)
+				open_namespace_fd(CLONE_NEWNS, optarg);
+			else
+				namespaces |= CLONE_NEWNS;
 			break;
 		case 'u':
-			open_namespace_fd(CLONE_NEWUTS, optarg);
+			if (optarg)
+				open_namespace_fd(CLONE_NEWUTS, optarg);
+			else
+				namespaces |= CLONE_NEWUTS;
 			break;
 		case 'i':
-			open_namespace_fd(CLONE_NEWIPC, optarg);
+			if (optarg)
+				open_namespace_fd(CLONE_NEWIPC, optarg);
+			else
+				namespaces |= CLONE_NEWIPC;
 			break;
 		case 'n':
-			open_namespace_fd(CLONE_NEWNET, optarg);
+			if (optarg)
+				open_namespace_fd(CLONE_NEWNET, optarg);
+			else
+				namespaces |= CLONE_NEWNET;
 			break;
 		case 'p':
 			do_fork = 1;
-			open_namespace_fd(CLONE_NEWPID, optarg);
+			if (optarg)
+				open_namespace_fd(CLONE_NEWPID, optarg);
+			else
+				namespaces |= CLONE_NEWPID;
 			break;
 		case 'U':
-			open_namespace_fd(CLONE_NEWUSER, optarg);
+			if (optarg)
+				open_namespace_fd(CLONE_NEWUSER, optarg);
+			else
+				namespaces |= CLONE_NEWUSER;
 			break;
 		case 'e':
 			do_fork = 0;
 			break;
 		case 'r':
-			open_target_fd(&root_fd, "root", optarg);
+			if (optarg)
+				open_target_fd(&root_fd, "root", optarg);
+			else
+				do_rd = true;
 			break;
 		case 'w':
-			open_target_fd(&wd_fd, "cwd", optarg);
+			if (optarg)
+				open_target_fd(&wd_fd, "cwd", optarg);
+			else
+				do_wd = true;
 			break;
 		default:
 			usage(EXIT_FAILURE);
@@ -231,6 +257,17 @@ int main(int argc, char *argv[])
 		usage(EXIT_FAILURE);
 
 	/*
+	 * Open remaining namespace and directory descriptors.
+	 */
+	for (nsfile = namespace_files; nsfile->nstype; nsfile++)
+		if (nsfile->nstype & namespaces)
+			open_namespace_fd(nsfile->nstype, NULL);
+	if (do_rd)
+		open_target_fd(&root_fd, "root", optarg);
+	if (do_wd)
+		open_target_fd(&wd_fd, "cwd", optarg);
+
+	/*
 	 * Now that we know which namespaces we want to enter, enter them.
 	 */
 	for (nsfile = namespace_files; nsfile->nstype; nsfile++) {
-- 
1.7.11.7

Re: [PATCH 1/5] nsenter: allow arguments to be specified in any order

[Karel Zak]

On Mon, Jan 21, 2013 at 01:38:01AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> Allows 'nsenter -mt $PID', which would fail previously.
> 
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
>  sys-utils/nsenter.c | 55 ++++++++++++++++++++++++++++++++++++++++++++---------
>  1 file changed, 46 insertions(+), 9 deletions(-)

 Applied, thanks.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Zbigniew Jędrzejewski-Szmek]

I guess that most of the time one will want to enter all
namespaces, and then it is easier not to have to remember
all the option names.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 sys-utils/nsenter.1 |  5 +++++
 sys-utils/nsenter.c | 42 ++++++++++++++++++++++++++++++------------
 2 files changed, 35 insertions(+), 12 deletions(-)

diff --git a/sys-utils/nsenter.1 b/sys-utils/nsenter.1
index d133e1f..ab1a6f1 100644
--- a/sys-utils/nsenter.1
+++ b/sys-utils/nsenter.1
@@ -120,6 +120,11 @@ Enter the user namespace.  If no file is specified enter the user namespace of
 the target process.  If file is specified enter the user namespace specified by
 file.
 .TP
+\fB\-a\fR, \fB\-\-all\fR
+Set the root directory and enter the user, pid, net, IPC, uts, and mount namespaces
+of the target process (all namespaces listed above). In case of the user namespace,
+ignore failure to enter own namespace.
+.TP
 \fB\-r\fR, \fB\-\-root\fR [\fIdirectory\fR]
 Set the root directory.  If no directory is specified set the root directory to
 the root directory of the target process.  If directory is specified set the
diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index 1b57cc5..4ea3c09 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -71,9 +71,10 @@ static void usage(int status)
 		" -n, --net   [=<file>]  enter network namespace\n"
 		" -p, --pid   [=<file>]  enter pid namespace\n"
 		" -U, --user  [=<file>]  enter user namespace\n"
-		" -e, --exec             don't fork before exec'ing <program>\n"
 		" -r, --root  [=<dir>]   set the root directory\n"
-		" -w, --wd    [=<dir>]   set the working directory\n"), out);
+		" -w, --wd    [=<dir>]   set the working directory\n"
+		" -a, --all              set root and cwd and enter namespaces listed above\n"
+		" -e, --exec             don't fork before exec'ing <program>\n"), out);
 	fputs(USAGE_SEPARATOR, out);
 	fputs(USAGE_HELP, out);
 	fputs(USAGE_VERSION, out);
@@ -167,16 +168,17 @@ int main(int argc, char *argv[])
 		{ "net", optional_argument, NULL, 'n' },
 		{ "pid", optional_argument, NULL, 'p' },
 		{ "user", optional_argument, NULL, 'U' },
-		{ "exec", no_argument, NULL, 'e' },
 		{ "root", optional_argument, NULL, 'r' },
 		{ "wd", optional_argument, NULL, 'w' },
+		{ "all", no_argument, NULL, 'a' },
+		{ "exec", no_argument, NULL, 'e' },
 		{ NULL, 0, NULL, 0 }
 	};
 
 	struct namespace_file *nsfile;
 	int do_fork = 0;
 	int c, namespaces = 0;
-	bool do_rd = false, do_wd = false;
+	bool do_rd = false, do_wd = false, all_used = false;
 
 	setlocale(LC_MESSAGES, "");
 	bindtextdomain(PACKAGE, LOCALEDIR);
@@ -184,7 +186,7 @@ int main(int argc, char *argv[])
 	atexit(close_stdout);
 
 	while ((c =
-		getopt_long(argc, argv, "hVt:m::u::i::n::p::U::er::w::",
+		getopt_long(argc, argv, "hVt:m::u::i::n::p::U::r::w::ae",
 			    longopts, NULL)) != -1) {
 		switch (c) {
 		case 'h':
@@ -233,9 +235,6 @@ int main(int argc, char *argv[])
 			else
 				namespaces |= CLONE_NEWUSER;
 			break;
-		case 'e':
-			do_fork = 0;
-			break;
 		case 'r':
 			if (optarg)
 				open_target_fd(&root_fd, "root", optarg);
@@ -248,6 +247,15 @@ int main(int argc, char *argv[])
 			else
 				do_wd = true;
 			break;
+		case 'a':
+			namespaces = ~0;
+			do_rd = do_wd = true;
+			do_fork = 1;
+			all_used = true;
+			break;
+		case 'e':
+			do_fork = 0;
+			break;
 		default:
 			usage(EXIT_FAILURE);
 		}
@@ -271,12 +279,22 @@ int main(int argc, char *argv[])
 	 * Now that we know which namespaces we want to enter, enter them.
 	 */
 	for (nsfile = namespace_files; nsfile->nstype; nsfile++) {
+		int r;
+
 		if (nsfile->fd < 0)
 			continue;
-		if (setns(nsfile->fd, nsfile->nstype))
-			err(EXIT_FAILURE,
-			    _("reassociate to namespace '%s' failed"),
-			    nsfile->name);
+		r = setns(nsfile->fd, nsfile->nstype);
+		if (r) {
+			/*
+			 * Skip EINVAL when trying to enter user namespace
+			 * if it was requested through --all.
+			 */
+			if (!(nsfile->nstype == CLONE_NEWUSER &&
+			      errno == EINVAL && all_used))
+				err(EXIT_FAILURE,
+				    _("reassociate to namespace '%s' failed"),
+				    nsfile->name);
+		}
 		close(nsfile->fd);
 		nsfile->fd = -1;
 	}
-- 
1.7.11.7

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Karel Zak]

On Mon, Jan 21, 2013 at 01:38:02AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> I guess that most of the time one will want to enter all
> namespaces, and then it is easier not to have to remember
> all the option names.

 Not sure if this is the right argument. From my point of view it's
 better to be explicit for such things, something like --all sounds
 like a magical blackbox where semantic depends on features implemented
 by kernel and nsenter(1). 

 Not applied.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Zbigniew Jędrzejewski-Szmek]

On Fri, Jan 25, 2013 at 04:02:10PM +0100, Karel Zak wrote:
> On Mon, Jan 21, 2013 at 01:38:02AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> > I guess that most of the time one will want to enter all
> > namespaces, and then it is easier not to have to remember
> > all the option names.
> 
>  Not sure if this is the right argument. From my point of view it's
>  better to be explicit for such things, something like --all sounds
>  like a magical blackbox where semantic depends on features implemented
>  by kernel and nsenter(1). 
Hi,

I'm was trying to document how a user should enter a namespace
container created by systemd-nspawn. I would prefer not to have the
user type 'nsenter -t $PID -muipn', but something simpler.

What about an alternative patch, which implements --all which means:
"all namespaces supported by the kernel" (i.e. iterate over /proc/$PID/ns/*'
and enter all of them. This way the behaviour would depend only on the
kernel options, not on util-linux version.

Zbyszek

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Eric W. Biederman]

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> On Fri, Jan 25, 2013 at 04:02:10PM +0100, Karel Zak wrote:
>> On Mon, Jan 21, 2013 at 01:38:02AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
>> > I guess that most of the time one will want to enter all
>> > namespaces, and then it is easier not to have to remember
>> > all the option names.
>> 
>>  Not sure if this is the right argument. From my point of view it's
>>  better to be explicit for such things, something like --all sounds
>>  like a magical blackbox where semantic depends on features implemented
>>  by kernel and nsenter(1). 

Which is the reason I did not implement --all in the first place,
although it is attractive.

> Hi,
>
> I'm was trying to document how a user should enter a namespace
> container created by systemd-nspawn. I would prefer not to have the
> user type 'nsenter -t $PID -muipn', but something simpler.

As I see it nsenter is the raw tool for when you need to get your
hands dirty.  lxc already has a more integrated version.  And
it isn't hard to define a simple wrapper such as:

cat > systemd-nsenter <<EOF
#!/bin/sh
PID=$1
shift
exec nsenter -t $PID --mount --ipc --pid --net --uts "$@"
EOF

If you need things to be slightly simpler and it isn't worth deriving
your own c wrapper.

I assume you didn't include -U because systemd-nspawn doesn't create
a user namespace?

Of course at the point you wrap nsenter you probably want to have
something that takes a name and looks at a pid file I expect.

> What about an alternative patch, which implements --all which means:
> "all namespaces supported by the kernel" (i.e. iterate over /proc/$PID/ns/*'
> and enter all of them. This way the behaviour would depend only on the
> kernel options, not on util-linux version.

If we add another namespace will it have oddball semantics to worry
about?  So far the mount namespace, the user namespace, and the pid
namespace do.  So judging from history you have a 50/50 chance of
needing special code in nsenter.  I don't expect blinding iterarting
over /proc/[pid]/ns/* will remove the need for future changes to
nsenter if and when we add another namespace.

What will keep from breaking peoples scripts is to not have an option
that is ambiguous.

Eric

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Zbigniew Jędrzejewski-Szmek]

On Fri, Jan 25, 2013 at 09:44:50AM -0800, Eric W. Biederman wrote:
> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> 
> > On Fri, Jan 25, 2013 at 04:02:10PM +0100, Karel Zak wrote:
> >> On Mon, Jan 21, 2013 at 01:38:02AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> >> > I guess that most of the time one will want to enter all
> >> > namespaces, and then it is easier not to have to remember
> >> > all the option names.
> >> 
> >>  Not sure if this is the right argument. From my point of view it's
> >>  better to be explicit for such things, something like --all sounds
> >>  like a magical blackbox where semantic depends on features implemented
> >>  by kernel and nsenter(1). 
> 
> Which is the reason I did not implement --all in the first place,
> although it is attractive.
> 
> > Hi,
> >
> > I'm was trying to document how a user should enter a namespace
> > container created by systemd-nspawn. I would prefer not to have the
> > user type 'nsenter -t $PID -muipn', but something simpler.
> 
> As I see it nsenter is the raw tool for when you need to get your
> hands dirty.  lxc already has a more integrated version.  And
> it isn't hard to define a simple wrapper such as:
> 
> cat > systemd-nsenter <<EOF
> #!/bin/sh
> PID=$1
> shift
> exec nsenter -t $PID --mount --ipc --pid --net --uts "$@"
> EOF
> 
> If you need things to be slightly simpler and it isn't worth deriving
> your own c wrapper.
> 
> I assume you didn't include -U because systemd-nspawn doesn't create
> a user namespace?
Yes, systemd-nspawn so far doesn't.

> Of course at the point you wrap nsenter you probably want to have
> something that takes a name and looks at a pid file I expect.
> 
> > What about an alternative patch, which implements --all which means:
> > "all namespaces supported by the kernel" (i.e. iterate over /proc/$PID/ns/*'
> > and enter all of them. This way the behaviour would depend only on the
> > kernel options, not on util-linux version.
> 
> If we add another namespace will it have oddball semantics to worry
> about?  So far the mount namespace, the user namespace, and the pid
> namespace do.  So judging from history you have a 50/50 chance of
> needing special code in nsenter.  I don't expect blinding iterarting
> over /proc/[pid]/ns/* will remove the need for future changes to
> nsenter if and when we add another namespace.
OK, so that's not an option.

> What will keep from breaking peoples scripts is to not have an option
> that is ambiguous.
Well, in the two patches I was careful to write "enter all of the
above namespaces", to underline the fact that the kernel might know
about some additional ones.

OK, bear with me. What about --all meaning "enter all of the
namespaces that nsenter has support for, and fail if there's a
namespace diffent between target and requesting process that
nsenter doesn't know about"?

In scripts one could say 'nsenter -<wanted namespaces>', but for quick'n'dirty
commandline use, nsenter --all would work.

Zbyszek

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Ángel González]

Eric W. Biederman writes:
> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
>>>  Not sure if this is the right argument. From my point of view it's
>>>  better to be explicit for such things, something like --all sounds
>>>  like a magical blackbox where semantic depends on features implemented
>>>  by kernel and nsenter(1). 
> 
> Which is the reason I did not implement --all in the first place,
> although it is attractive.
> 
>> Hi,
>>
>> I'm was trying to document how a user should enter a namespace
>> container created by systemd-nspawn. I would prefer not to have the
>> user type 'nsenter -t $PID -muipn', but something simpler.
> 
> As I see it nsenter is the raw tool for when you need to get your
> hands dirty.  lxc already has a more integrated version.  And
> it isn't hard to define a simple wrapper such as:
> 
> cat > systemd-nsenter <<EOF
> #!/bin/sh
> PID=$1
> shift
> exec nsenter -t $PID --mount --ipc --pid --net --uts "$@"
> EOF
> 
> If you need things to be slightly simpler and it isn't worth deriving
> your own c wrapper.

Except that when you are distributing such script (eg. an init-like
script), your shell script will need to add code detecting which
namespaces the kernel supports (and adding appropiate flags to nsenter)
and checking if your nsenter version supports them or not.

It's better to have --all to enter all namespaces that nsenter supports.
If you want to, it could print a warning when using --all and nsenter
knows about more namespaces than the kernel or if it detects that the
kernel knows about more namespaces than itself.
But having a --all to enter “as namespaces as possible” would be the
right thing IMHO.

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Eric W. Biederman]

Ángel González <ingenit@zoho.com> writes:

> Eric W. Biederman writes:
>> Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
>>>>  Not sure if this is the right argument. From my point of view it's
>>>>  better to be explicit for such things, something like --all sounds
>>>>  like a magical blackbox where semantic depends on features implemented
>>>>  by kernel and nsenter(1). 
>> 
>> Which is the reason I did not implement --all in the first place,
>> although it is attractive.
>> 
>>> Hi,
>>>
>>> I'm was trying to document how a user should enter a namespace
>>> container created by systemd-nspawn. I would prefer not to have the
>>> user type 'nsenter -t $PID -muipn', but something simpler.
>> 
>> As I see it nsenter is the raw tool for when you need to get your
>> hands dirty.  lxc already has a more integrated version.  And
>> it isn't hard to define a simple wrapper such as:
>> 
>> cat > systemd-nsenter <<EOF
>> #!/bin/sh
>> PID=$1
>> shift
>> exec nsenter -t $PID --mount --ipc --pid --net --uts "$@"
>> EOF
>> 
>> If you need things to be slightly simpler and it isn't worth deriving
>> your own c wrapper.
>
> Except that when you are distributing such script (eg. an init-like
> script), your shell script will need to add code detecting which
> namespaces the kernel supports (and adding appropiate flags to nsenter)
> and checking if your nsenter version supports them or not.

Then what you want is --ignore-namespaces-if-not-supported-in-kernel.
That is a somewhat different problem than --all.

The --all patch attempts to do something like that and I suspect in the
corner case where the we have namespace files for that namespace but
no setns support in the kernel the logic in the --all patch will work.
However usually the failure will be you can't open /proc/$pid/ns/$file,
which was not handled.

> It's better to have --all to enter all namespaces that nsenter supports.
> If you want to, it could print a warning when using --all and nsenter
> knows about more namespaces than the kernel or if it detects that the
> kernel knows about more namespaces than itself.
> But having a --all to enter “as namespaces as possible” would be the
> right thing IMHO.

The argument for --all and this is an argument that only applies to
nsenter and not unshare is that usually things are more likely to do
what you expect if you share more namespaces with them.

The counter argument is that sharing fewer namespaces than expected
can easily invalidate your testing and introduce subtle bugs.

"nsenter -t $pid --all" is what I want most days, but I can't
convince myself that "nsenter -t $pid -muinpUrw" is worse (just a few
more characters) and it has the advantage that what has worked before
should work again.

Right now --all looks like a subtle trap waiting for users.  Even
--ignore-namespaces-if-not-supported-in-kernel looks like that to
a degree.

With nsenter if we don't ignore failures when something we need is
missing the failure is up there and in your face.  If we do ignore
failures we can easily run into cases where commands that we run
continue to work but on the wrong namespaces, causing all kinds of
things to fail.

Think about using: "nsneter -t $pid -p /bin/kill -KILL -1" to ask a
container to shutdown.  If we were to ignore the fact you can't enter
the pid namespace than the command would kill everything and the box
would go down.

That seems very scary.   So I don't like removing namespaces silently.

Having an --all that could mean everything nsenter supports this week
is better but you start getting shell scripts that work fine on new
distros but fail in horrible ways on old distro's with old versions of
nsenter.  That certainly is better but still a bit of a scary prospect.

Eric

Re: [PATCH 2/5] nsenter: add --all meaning all namespaces and cwd and root

[Ángel González]

On 28/01/13 03:38, Eric W. Biederman wrote:
> Ángel González <ingenit@zoho.com> writes:
>> Except that when you are distributing such script (eg. an init-like
>> script), your shell script will need to add code detecting which
>> namespaces the kernel supports (and adding appropiate flags to nsenter)
>> and checking if your nsenter version supports them or not.
> 
> Then what you want is --ignore-namespaces-if-not-supported-in-kernel.
> That is a somewhat different problem than --all.

No, I also want it to do
--include-namespaces-i-did-not-know-when-writing-this-shell-script :)



>> It's better to have --all to enter all namespaces that nsenter supports.
>> If you want to, it could print a warning when using --all and nsenter
>> knows about more namespaces than the kernel or if it detects that the
>> kernel knows about more namespaces than itself.
>> But having a --all to enter “as namespaces as possible” would be the
>> right thing IMHO.
> 
> The argument for --all and this is an argument that only applies to
> nsenter and not unshare is that usually things are more likely to do
> what you expect if you share more namespaces with them.
> 
> The counter argument is that sharing fewer namespaces than expected
> can easily invalidate your testing and introduce subtle bugs.
> 
> "nsenter -t $pid --all" is what I want most days, but I can't
> convince myself that "nsenter -t $pid -muinpUrw" is worse (just a few
> more characters) and it has the advantage that what has worked before
> should work again.
> 
> Right now --all looks like a subtle trap waiting for users.  Even
> --ignore-namespaces-if-not-supported-in-kernel looks like that to
> a degree.
> 
> With nsenter if we don't ignore failures when something we need is
> missing the failure is up there and in your face.  If we do ignore
> failures we can easily run into cases where commands that we run
> continue to work but on the wrong namespaces, causing all kinds of
> things to fail.
> 
> Think about using: "nsneter -t $pid -p /bin/kill -KILL -1" to ask a
> container to shutdown.  If we were to ignore the fact you can't enter
> the pid namespace than the command would kill everything and the box
> would go down.
> 
> That seems very scary.   So I don't like removing namespaces silently.

It could show a "Warning: Your kernel doesn't support pid namespaces"
in stderr.

No texactly ideal in your case, but slightly better.
Another option is to add --all-available, making a hard error not having
the namespaces specified by the explicit options but
--all-available to ignore missing ones.

> Having an --all that could mean everything nsenter supports this week
> is better but you start getting shell scripts that work fine on new
> distros but fail in horrible ways on old distro's with old versions of
> nsenter.  That certainly is better but still a bit of a scary prospect.
> 
> Eric

The namespaces-of-the-week could be a configure option for people
running on old kernels, but I don't see much way for improvement.

[PATCH 3/5] nsenter: respect --exec no matter where it appears

[Zbigniew Jędrzejewski-Szmek]

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 sys-utils/nsenter.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index 4ea3c09..1f773e1 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -176,7 +176,7 @@ int main(int argc, char *argv[])
 	};
 
 	struct namespace_file *nsfile;
-	int do_fork = 0;
+	int do_fork = -1; /* unknown yet */
 	int c, namespaces = 0;
 	bool do_rd = false, do_wd = false, all_used = false;
 
@@ -223,7 +223,6 @@ int main(int argc, char *argv[])
 				namespaces |= CLONE_NEWNET;
 			break;
 		case 'p':
-			do_fork = 1;
 			if (optarg)
 				open_namespace_fd(CLONE_NEWPID, optarg);
 			else
@@ -250,7 +249,6 @@ int main(int argc, char *argv[])
 		case 'a':
 			namespaces = ~0;
 			do_rd = do_wd = true;
-			do_fork = 1;
 			all_used = true;
 			break;
 		case 'e':
@@ -283,6 +281,10 @@ int main(int argc, char *argv[])
 
 		if (nsfile->fd < 0)
 			continue;
+
+		if (nsfile->nstype == CLONE_NEWPID && do_fork == -1)
+			do_fork = 1;
+
 		r = setns(nsfile->fd, nsfile->nstype);
 		if (r) {
 			/*
@@ -295,6 +297,7 @@ int main(int argc, char *argv[])
 				    _("reassociate to namespace '%s' failed"),
 				    nsfile->name);
 		}
+
 		close(nsfile->fd);
 		nsfile->fd = -1;
 	}
@@ -330,7 +333,7 @@ int main(int argc, char *argv[])
 		wd_fd = -1;
 	}
 
-	if (do_fork)
+	if (do_fork == 1)
 		continue_as_child();
 
 	execvp(argv[optind], argv + optind);
-- 
1.7.11.7

Re: [PATCH 3/5] nsenter: respect --exec no matter where it appears

[Karel Zak]

On Mon, Jan 21, 2013 at 01:38:03AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
>  sys-utils/nsenter.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)

 It seems necessary for --all only, right?

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

Re: [PATCH 3/5] nsenter: respect --exec no matter where it appears

[Zbigniew Jędrzejewski-Szmek]

On Fri, Jan 25, 2013 at 04:02:55PM +0100, Karel Zak wrote:
> On Mon, Jan 21, 2013 at 01:38:03AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> >  sys-utils/nsenter.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> 
>  It seems necessary for --all only, right?
Hm, no. If I understand the (original) code correctly, doing something
like 'nsenter --exec --pid' would ignore the '--exec' argument.

Zbyszek

Re: [PATCH 3/5] nsenter: respect --exec no matter where it appears

[Karel Zak]

On Fri, Jan 25, 2013 at 04:07:28PM +0100, Zbigniew Jędrzejewski-Szmek wrote:
> On Fri, Jan 25, 2013 at 04:02:55PM +0100, Karel Zak wrote:
> > On Mon, Jan 21, 2013 at 01:38:03AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> > >  sys-utils/nsenter.c | 11 +++++++----
> > >  1 file changed, 7 insertions(+), 4 deletions(-)
> > 
> >  It seems necessary for --all only, right?
> Hm, no. If I understand the (original) code correctly, doing something
> like 'nsenter --exec --pid' would ignore the '--exec' argument.

 Ah, I see, You're right. I'll apply the patch. Thanks.

    Karel.

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[PATCH 4/5] nsenter: rename --exec/-e to --no-fork/-F

[Zbigniew Jędrzejewski-Szmek]

The naming of this option was really confusing.
Just rename it for clarity.

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 sys-utils/nsenter.1 | 2 +-
 sys-utils/nsenter.c | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/sys-utils/nsenter.1 b/sys-utils/nsenter.1
index ab1a6f1..7ca0f0e 100644
--- a/sys-utils/nsenter.1
+++ b/sys-utils/nsenter.1
@@ -135,7 +135,7 @@ Set the working directory.  If no directory is specified set the working
 directory to the working directory of the target process.  If directory is
 specified set the working directory to the specified directory.
 .TP
-\fB\-e\fR, \fB\-\-exec\fR
+\fB\-e\fR, \fB\-\-no\-fork\fR
 Do not fork before exec'ing the specified program.  By default when entering a
 pid namespace enter calls fork before calling exec so that the children will be
 in the newly entered pid namespace.
diff --git a/sys-utils/nsenter.c b/sys-utils/nsenter.c
index 1f773e1..0a4985c 100644
--- a/sys-utils/nsenter.c
+++ b/sys-utils/nsenter.c
@@ -74,7 +74,7 @@ static void usage(int status)
 		" -r, --root  [=<dir>]   set the root directory\n"
 		" -w, --wd    [=<dir>]   set the working directory\n"
 		" -a, --all              set root and cwd and enter namespaces listed above\n"
-		" -e, --exec             don't fork before exec'ing <program>\n"), out);
+		" -F, --no-fork          don't fork before exec'ing <program>\n"), out);
 	fputs(USAGE_SEPARATOR, out);
 	fputs(USAGE_HELP, out);
 	fputs(USAGE_VERSION, out);
@@ -171,7 +171,7 @@ int main(int argc, char *argv[])
 		{ "root", optional_argument, NULL, 'r' },
 		{ "wd", optional_argument, NULL, 'w' },
 		{ "all", no_argument, NULL, 'a' },
-		{ "exec", no_argument, NULL, 'e' },
+		{ "no-fork", no_argument, NULL, 'F' },
 		{ NULL, 0, NULL, 0 }
 	};
 
@@ -186,7 +186,7 @@ int main(int argc, char *argv[])
 	atexit(close_stdout);
 
 	while ((c =
-		getopt_long(argc, argv, "hVt:m::u::i::n::p::U::r::w::ae",
+		getopt_long(argc, argv, "hVt:m::u::i::n::p::U::r::w::aF",
 			    longopts, NULL)) != -1) {
 		switch (c) {
 		case 'h':
@@ -251,7 +251,7 @@ int main(int argc, char *argv[])
 			do_rd = do_wd = true;
 			all_used = true;
 			break;
-		case 'e':
+		case 'F':
 			do_fork = 0;
 			break;
 		default:
-- 
1.7.11.7

Re: [PATCH 4/5] nsenter: rename --exec/-e to --no-fork/-F

[Karel Zak]

On Mon, Jan 21, 2013 at 01:38:05AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> The naming of this option was really confusing.

 I agree.

> Just rename it for clarity.
> 
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
>  sys-utils/nsenter.1 | 2 +-
>  sys-utils/nsenter.c | 8 ++++----
>  2 files changed, 5 insertions(+), 5 deletions(-)

 Applied, thanks.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com

[PATCH 5/5] unshare: add --all meaning all namespaces

[Zbigniew Jędrzejewski-Szmek]

Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
---
 sys-utils/unshare.1 | 3 +++
 sys-utils/unshare.c | 9 ++++++++-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/sys-utils/unshare.1 b/sys-utils/unshare.1
index 8cdc6e5..d91c86e 100644
--- a/sys-utils/unshare.1
+++ b/sys-utils/unshare.1
@@ -61,6 +61,9 @@ Unshare the pid namespace.
 .TP
 .BR \-U , " \-\-user"
 Unshare the user namespace.
+.TP
+.BR \-U , " \-\-user"
+Unshare all the namespaces listed above.
 .SH NOTES
 .SH SEE ALSO
 .BR unshare (2),
diff --git a/sys-utils/unshare.c b/sys-utils/unshare.c
index 9b849ee..c891cb5 100644
--- a/sys-utils/unshare.c
+++ b/sys-utils/unshare.c
@@ -54,6 +54,10 @@ static void usage(int status)
 	exit(status);
 }
 
+#define ALL_CLONE_FLAGS (CLONE_NEWNS|CLONE_NEWUTS|CLONE_NEWIPC|CLONE_NEWNET\
+			 |CLONE_NEWPID|CLONE_NEWUSER)
+
+
 int main(int argc, char *argv[])
 {
 	static const struct option longopts[] = {
@@ -65,6 +69,7 @@ int main(int argc, char *argv[])
 		{ "net", no_argument, 0, 'n' },
 		{ "pid", no_argument, 0, 'p' },
 		{ "user", no_argument, 0, 'U' },
+		{ "all", no_argument, 0, 'a' },
 		{ NULL, 0, 0, 0 }
 	};
 
@@ -77,7 +82,7 @@ int main(int argc, char *argv[])
 	textdomain(PACKAGE);
 	atexit(close_stdout);
 
-	while((c = getopt_long(argc, argv, "hVmuinpU", longopts, NULL)) != -1) {
+	while((c = getopt_long(argc, argv, "hVmuinpUa", longopts, NULL)) != -1) {
 		switch(c) {
 		case 'h':
 			usage(EXIT_SUCCESS);
@@ -102,6 +107,8 @@ int main(int argc, char *argv[])
 		case 'U':
 			unshare_flags |= CLONE_NEWUSER;
 			break;
+		case 'a':
+			unshare_flags |= ALL_CLONE_FLAGS;
 		default:
 			usage(EXIT_FAILURE);
 		}
-- 
1.7.11.7

Re: [PATCH 5/5] unshare: add --all meaning all namespaces

[Karel Zak]

On Mon, Jan 21, 2013 at 01:38:07AM -0500, Zbigniew Jędrzejewski-Szmek wrote:
> Signed-off-by: Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl>
> ---
>  sys-utils/unshare.1 | 3 +++
>  sys-utils/unshare.c | 9 ++++++++-
>  2 files changed, 11 insertions(+), 1 deletion(-)

 .. same like nsenter. Not applied.

    Karel

-- 
 Karel Zak  <kzak@redhat.com>
 http://karelzak.blogspot.com