[PATCH] make cr depend on all namespaces

[PATCH] make cr depend on all namespaces

[Serge E. Hallyn]

This should let us get rid of some ifdefed code and reduce
chances for bad config combinations.  There's really no reason
to support it.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/Kconfig     |    5 +++++
 checkpoint/namespace.c |   10 ----------
 2 files changed, 5 insertions(+), 10 deletions(-)

diff --git a/checkpoint/Kconfig b/checkpoint/Kconfig
index 4a2c845..549668e 100644
--- a/checkpoint/Kconfig
+++ b/checkpoint/Kconfig
@@ -9,6 +9,11 @@ config DEFERQUEUE
 config CHECKPOINT
 	bool "Checkpoint/restart (EXPERIMENTAL)"
 	depends on CHECKPOINT_SUPPORT && EXPERIMENTAL
+	depends on UTS_NS
+	depends on USER_NS
+	depends on PID_NS
+	depends on NET_NS
+	depends on DEVPTS_MULTIPLE_INSTANCES
 	depends on CGROUP_FREEZER
 	select DEFERQUEUE
 	help
diff --git a/checkpoint/namespace.c b/checkpoint/namespace.c
index 4b3ac5a..6389dbd 100644
--- a/checkpoint/namespace.c
+++ b/checkpoint/namespace.c
@@ -63,7 +63,6 @@ static struct uts_namespace *do_restore_uts_ns(struct ckpt_ctx *ctx)
 	if (IS_ERR(h))
 		return (struct uts_namespace *) h;
 
-#ifdef CONFIG_UTS_NS
 	uts_ns = create_uts_ns();
 	if (!uts_ns) {
 		uts_ns = ERR_PTR(-ENOMEM);
@@ -78,15 +77,6 @@ static struct uts_namespace *do_restore_uts_ns(struct ckpt_ctx *ctx)
 	memcpy(name->machine, h->machine, sizeof(name->machine));
 	memcpy(name->domainname, h->domainname, sizeof(name->domainname));
 	up_read(&uts_sem);
-#else
-	/* complain if image contains multiple namespaces */
-	if (ctx->stats.uts_ns) {
-		uts_ns = ERR_PTR(-EEXIST);
-		goto out;
-	}
-	uts_ns = current->nsproxy->uts_ns;
-	get_uts_ns(uts_ns);
-#endif
 
 	ctx->stats.uts_ns++;
  out:
-- 
1.6.1

Re: [PATCH] make cr depend on all namespaces

[Nathan Lynch]

On Mon, 2010-03-15 at 15:05 -0500, Serge E. Hallyn wrote:
> This should let us get rid of some ifdefed code and reduce
> chances for bad config combinations.  There's really no reason
> to support it.

I agree!

Acked-by: Nathan Lynch <ntl-e+AXbWqSrlAAvxtiuMwx3w@public.gmane.org>

Re: [PATCH] make cr depend on all namespaces

[Oren Laadan]

Serge E. Hallyn wrote:
> This should let us get rid of some ifdefed code and reduce
> chances for bad config combinations.  There's really no reason
> to support it.

I disagree.

You are right that this will reduce the changes of bad config
combinations.

However, it will also introduce some restrictions on the kernel
config which are unnecessary. Some people may not want to have
all namespaces configured.

Note that the namespaces are independent in the sense that we
don't need to test all combination of all namespaces - instead,
I consider turning on/off one at a time to be safe enough.

(FWIW, is it because you only wanted to show a point that you
only remove UTS_NS ifdefs ?)

Oren.

> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/Kconfig     |    5 +++++
>  checkpoint/namespace.c |   10 ----------
>  2 files changed, 5 insertions(+), 10 deletions(-)
> 
> diff --git a/checkpoint/Kconfig b/checkpoint/Kconfig
> index 4a2c845..549668e 100644
> --- a/checkpoint/Kconfig
> +++ b/checkpoint/Kconfig
> @@ -9,6 +9,11 @@ config DEFERQUEUE
>  config CHECKPOINT
>  	bool "Checkpoint/restart (EXPERIMENTAL)"
>  	depends on CHECKPOINT_SUPPORT && EXPERIMENTAL
> +	depends on UTS_NS
> +	depends on USER_NS
> +	depends on PID_NS
> +	depends on NET_NS
> +	depends on DEVPTS_MULTIPLE_INSTANCES
>  	depends on CGROUP_FREEZER
>  	select DEFERQUEUE
>  	help
> diff --git a/checkpoint/namespace.c b/checkpoint/namespace.c
> index 4b3ac5a..6389dbd 100644
> --- a/checkpoint/namespace.c
> +++ b/checkpoint/namespace.c
> @@ -63,7 +63,6 @@ static struct uts_namespace *do_restore_uts_ns(struct ckpt_ctx *ctx)
>  	if (IS_ERR(h))
>  		return (struct uts_namespace *) h;
>  
> -#ifdef CONFIG_UTS_NS
>  	uts_ns = create_uts_ns();
>  	if (!uts_ns) {
>  		uts_ns = ERR_PTR(-ENOMEM);
> @@ -78,15 +77,6 @@ static struct uts_namespace *do_restore_uts_ns(struct ckpt_ctx *ctx)
>  	memcpy(name->machine, h->machine, sizeof(name->machine));
>  	memcpy(name->domainname, h->domainname, sizeof(name->domainname));
>  	up_read(&uts_sem);
> -#else
> -	/* complain if image contains multiple namespaces */
> -	if (ctx->stats.uts_ns) {
> -		uts_ns = ERR_PTR(-EEXIST);
> -		goto out;
> -	}
> -	uts_ns = current->nsproxy->uts_ns;
> -	get_uts_ns(uts_ns);
> -#endif
>  
>  	ctx->stats.uts_ns++;
>   out:

Re: [PATCH] make cr depend on all namespaces

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> 
> Serge E. Hallyn wrote:
> > This should let us get rid of some ifdefed code and reduce
> > chances for bad config combinations.  There's really no reason
> > to support it.
> 
> I disagree.
> 
> You are right that this will reduce the changes of bad config
> combinations.
> 
> However, it will also introduce some restrictions on the kernel
> config which are unnecessary. Some people may not want to have
> all namespaces configured.

Why?  The only reason right now to disable namespaces is for
kernel size.

> Note that the namespaces are independent in the sense that we
> don't need to test all combination of all namespaces - instead,
> I consider turning on/off one at a time to be safe enough.

And do you do that?  :)  It still gets more complicated bc
you have things like sysvipc and posix mq which both can allow
ipc_ns.

> (FWIW, is it because you only wanted to show a point that you
> only remove UTS_NS ifdefs ?)

It was just right there in my face...

Anyway if you don't take this patch then the UTS_NS code I
removed should have 'name' put under ifdef to avoid a build
warning.

-serge

Re: [PATCH] make cr depend on all namespaces

[Oren Laadan]

Serge E. Hallyn wrote:
> Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
>>
>> Serge E. Hallyn wrote:
>>> This should let us get rid of some ifdefed code and reduce
>>> chances for bad config combinations.  There's really no reason
>>> to support it.
>> I disagree.
>>
>> You are right that this will reduce the changes of bad config
>> combinations.
>>
>> However, it will also introduce some restrictions on the kernel
>> config which are unnecessary. Some people may not want to have
>> all namespaces configured.
> 
> Why?  The only reason right now to disable namespaces is for
> kernel size.

Yup.

So we know it works well when all ns are enabled. If there are
so few users that disable some ns, then we will rarely hear about
breakage anyway.

On the other hand - what about current distributions - do they
enable all namespaces by default ?  If not (mine didn't), then
potential tester will haev yet another barrier to testing since,
for instance, net-ns is disabled.

> 
>> Note that the namespaces are independent in the sense that we
>> don't need to test all combination of all namespaces - instead,
>> I consider turning on/off one at a time to be safe enough.
> 
> And do you do that?  :)  It still gets more complicated bc
> you have things like sysvipc and posix mq which both can allow
> ipc_ns.

You are 100% correct - we don't, and we should automate it.

I thought you intentionally leave out IPC in that patch...  ?

Anyway, ipc is the exception, because it can be disabled as a
whole. Can you do that on others ? (e.g. uts, user, etc)

> 
>> (FWIW, is it because you only wanted to show a point that you
>> only remove UTS_NS ifdefs ?)
> 
> It was just right there in my face...
> 
> Anyway if you don't take this patch then the UTS_NS code I
> removed should have 'name' put under ifdef to avoid a build
> warning.

Ok, will patch away and push to v20-rc2.

Oren.

Re: [PATCH] make cr depend on all namespaces

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> 
> 
> Serge E. Hallyn wrote:
> >Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> >>
> >>Serge E. Hallyn wrote:
> >>>This should let us get rid of some ifdefed code and reduce
> >>>chances for bad config combinations.  There's really no reason
> >>>to support it.
> >>I disagree.
> >>
> >>You are right that this will reduce the changes of bad config
> >>combinations.
> >>
> >>However, it will also introduce some restrictions on the kernel
> >>config which are unnecessary. Some people may not want to have
> >>all namespaces configured.
> >
> >Why?  The only reason right now to disable namespaces is for
> >kernel size.
> 
> Yup.
> 
> So we know it works well when all ns are enabled. If there are
> so few users that disable some ns, then we will rarely hear about
> breakage anyway.
> 
> On the other hand - what about current distributions - do they
> enable all namespaces by default ?  If not (mine didn't), then
> potential tester will haev yet another barrier to testing since,
> for instance, net-ns is disabled.
> 
> >
> >>Note that the namespaces are independent in the sense that we
> >>don't need to test all combination of all namespaces - instead,
> >>I consider turning on/off one at a time to be safe enough.
> >
> >And do you do that?  :)  It still gets more complicated bc
> >you have things like sysvipc and posix mq which both can allow
> >ipc_ns.
> 
> You are 100% correct - we don't, and we should automate it.
> 
> I thought you intentionally leave out IPC in that patch...  ?
> 
> Anyway, ipc is the exception, because it can be disabled as a
> whole. Can you do that on others ? (e.g. uts, user, etc)
> 
> >
> >>(FWIW, is it because you only wanted to show a point that you
> >>only remove UTS_NS ifdefs ?)
> >
> >It was just right there in my face...
> >
> >Anyway if you don't take this patch then the UTS_NS code I
> >removed should have 'name' put under ifdef to avoid a build
> >warning.
> 
> Ok, will patch away and push to v20-rc2.

Thanks, I will fetch in a bit and re-test with your (presumably
rebased) tree and report over irc.