> -----Original Message----- > From: Eric W. Biederman [mailto:ebiederm@xxxxxxxxxxxx] > Sent: Thursday, October 09, 2014 1:56 AM > > Serge Hallyn <serge.hallyn@xxxxxxxxxx> writes: > > > Quoting Chen Hanxiao (chenhanxiao@xxxxxxxxxxxxxx): > >> We could use setns to join the current ns, > >> which did a lot of unnecessary work. > >> This patch will check this senario and > >> return 0 directly. > >> > >> Signed-off-by: Chen Hanxiao <chenhanxiao@xxxxxxxxxxxxxx> > > > > Plus it's just asking for trouble. > > > > I would ack this, except you need to fclose(file) on the > > return paths. So just set err = 0 and goto out. > > I completely disagree. > > Nacked-by: "Eric W. Biederman" <ebiederm@xxxxxxxxxxxx> > > This patch adds a new code path to test, and gets that new code path > wrong. So unless there is a performance advantage for some real world > case I don't see the point. Is there real software that is rejoining > the a current namespace. > > This patch changes the behavior of CLONE_NEWNS (which always does a > chdir and chroot) when you change into the current namespace. > > This patch changes the behavior of CLONE_NEWUSER which current errors > out. > As reentering the same namespace looks meaningless, and handling reentering same ns we behaved differently, How about just *reject* the behaviour of setns to current namespace? + switch (ops->type) { + case CLONE_NEWIPC: + if (ei->ns == tsk->nsproxy->ipc_ns) { + err = -EINVAL; + goto out; + } ... And things became easy, 6 simply cases could cover the whole scenario and will not bring troubles to users. Thanks, - Chen > This code adds a big switch statement to code that is otherwise table > driven. With the result that two pieces of code must be looked at > and modified whenever we want to tweak the behavior of setns for a > namespace. > > So in general I think this piece of code is a maintenance disaster, > with no apparent redeem virtues. > > Eric _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linuxfoundation.org/mailman/listinfo/containers
