build breaks when checkpoint unimplemented by arch

build breaks when checkpoint unimplemented by arch

[Nathan Lynch]

Hi Oren,

With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:

In file included from include/linux/checkpoint.h:28,
                 from kernel/exit.c:53:
include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
In file included from include/linux/checkpoint.h:28,
                 from kernel/exit.c:53:
include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
make[1]: *** [kernel/exit.o] Error 1


It appears that any architecture which does not supply
asm/checkpoint_hdr.h is broken in the same way.

Either all architectures need to supply asm/checkpoint_hdr.h (and define
CKPT_ARCH_NSIG), or there needs to be some other fix which allows
as-yet-unsupported arches to build..

Re: build breaks when checkpoint unimplemented by arch

[Oren Laadan]

On Mon, 6 Jul 2009, Nathan Lynch wrote:

> Hi Oren,
> 
> With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:
> 
> In file included from include/linux/checkpoint.h:28,
>                  from kernel/exit.c:53:
> include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
> In file included from include/linux/checkpoint.h:28,
>                  from kernel/exit.c:53:
> include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
> make[1]: *** [kernel/exit.o] Error 1
> 
> 
> It appears that any architecture which does not supply
> asm/checkpoint_hdr.h is broken in the same way.
> 
> Either all architectures need to supply asm/checkpoint_hdr.h (and define
> CKPT_ARCH_NSIG), or there needs to be some other fix which allows
> as-yet-unsupported arches to build..
> 

I see... well - maybe it's time to resend the powerpc port :p

Until then, this patch worked for me to compile without c/r

Oren.


diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index c47e796..b8f99be 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -19,6 +19,7 @@
 #define RESTART_TASKSELF	0x1
 
 #ifdef __KERNEL__
+#ifdef CONFIG_CHECKPOINT
 
 #include <linux/sched.h>
 #include <linux/nsproxy.h>
@@ -299,6 +300,7 @@ extern unsigned long ckpt_debug_level;
 
 #endif /* CONFIG_CHECKPOINT_DEBUG */
 
+#endif /* CONFIG_CHECKPOINT */
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_CHECKPOINT_H_ */

Re: build breaks when checkpoint unimplemented by arch

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> On Mon, 6 Jul 2009, Nathan Lynch wrote:
> 
> > Hi Oren,
> > 
> > With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:
> > 
> > In file included from include/linux/checkpoint.h:28,
> >                  from kernel/exit.c:53:
> > include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
> > In file included from include/linux/checkpoint.h:28,
> >                  from kernel/exit.c:53:
> > include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
> > make[1]: *** [kernel/exit.o] Error 1
> > 
> > 
> > It appears that any architecture which does not supply
> > asm/checkpoint_hdr.h is broken in the same way.
> > 
> > Either all architectures need to supply asm/checkpoint_hdr.h (and define
> > CKPT_ARCH_NSIG), or there needs to be some other fix which allows
> > as-yet-unsupported arches to build..
> > 
> 
> I see... well - maybe it's time to resend the powerpc port :p

arm, sh, etc.

> Until then, this patch worked for me to compile without c/r

That isn't enough bc a lot of files are including <linux/checkpoint_hdr.h> and
_types.h.

The following patch enables compilation on powerpc, and it still works on
s390.

-serge

From 14eb1922d4bb3b01aa5f729022edf843b354ec97 Mon Sep 17 00:00:00 2001
From: root <root-JJi412eHaPLkOmf+N4b0O9FgqiXiwxn+0E9HWUfgJXw@public.gmane.org>
Date: Mon, 6 Jul 2009 16:22:29 -0700
Subject: [PATCH] cr: enable compile on architectures without asm/checkpoint_hdr.h

ifdef out the contents of linux/checkpoint.h if !CONFIG_CHECKPOINT.
For all files which were including one of the other checkpoint*.h
files, include linux/checkpoint.h instead, which will #include
those.

Signed-off-by: Serge Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 fs/pipe.c                  |    1 -
 include/linux/checkpoint.h |    2 ++
 ipc/shm.c                  |    1 -
 ipc/util.h                 |    2 +-
 kernel/capability.c        |    4 +++-
 mm/filemap.c               |    2 --
 mm/mmap.c                  |    2 --
 mm/shmem.c                 |    2 --
 8 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/fs/pipe.c b/fs/pipe.c
index 5d4c1c8..18c22eb 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -24,7 +24,6 @@
 #include <asm/ioctls.h>
 
 #include <linux/checkpoint.h>
-#include <linux/checkpoint_hdr.h>
 
 /*
  * We use a start+len construction, which provides full use of the 
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index c47e796..9d82082 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -10,6 +10,7 @@
  *  distribution for more details.
  */
 
+#ifdef CONFIG_CHECKPOINT
 #define CHECKPOINT_VERSION  1
 
 /* checkpoint user flags */
@@ -301,4 +302,5 @@ extern unsigned long ckpt_debug_level;
 
 #endif /* __KERNEL__ */
 
+#endif
 #endif /* _LINUX_CHECKPOINT_H_ */
diff --git a/ipc/shm.c b/ipc/shm.c
index 0991134..516b179 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -40,7 +40,6 @@
 #include <linux/mount.h>
 #include <linux/ipc_namespace.h>
 #include <linux/ima.h>
-#include <linux/checkpoint_hdr.h>
 #include <linux/checkpoint.h>
 
 #include <asm/uaccess.h>
diff --git a/ipc/util.h b/ipc/util.h
index 48de0a9..1ddfd90 100644
--- a/ipc/util.h
+++ b/ipc/util.h
@@ -12,7 +12,7 @@
 
 #include <linux/unistd.h>
 #include <linux/err.h>
-#include <linux/checkpoint_hdr.h>
+#include <linux/checkpoint.h>
 
 #define SEQ_MULTIPLIER	(IPCMNI)
 
diff --git a/kernel/capability.c b/kernel/capability.c
index 90cc7b4..94328c9 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -15,7 +15,7 @@
 #include <linux/syscalls.h>
 #include <linux/pid_namespace.h>
 #include <linux/securebits.h>
-#include <linux/checkpoint_hdr.h>
+#include <linux/checkpoint.h>
 #include <asm/uaccess.h>
 #include "cred-internals.h"
 
@@ -375,6 +375,7 @@ static inline int restore_cap_bset(kernel_cap_t bset, struct cred *cred)
 }
 #endif /* CONFIG_SECURITY_FILE_CAPABILITIES */
 
+#ifdef CONFIG_CHECKPOINT
 static int do_restore_caps(struct ckpt_capabilities *h, struct cred *cred)
 {
 	kernel_cap_t effective, inheritable, permitted, bset;
@@ -428,6 +429,7 @@ int restore_capabilities(struct ckpt_capabilities *h, struct cred *new)
 
 	return ret;
 }
+#endif
 
 /**
  * capable - Determine if the current task has a superior capability in effect
diff --git a/mm/filemap.c b/mm/filemap.c
index 782664d..cd8556d 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -36,8 +36,6 @@
 #include <linux/mm_inline.h> /* for page_is_file_cache() */
 #include "internal.h"
 
-#include <linux/checkpoint_types.h>
-#include <linux/checkpoint_hdr.h>
 #include <linux/checkpoint.h>
 
 /*
diff --git a/mm/mmap.c b/mm/mmap.c
index e60424b..ba0b039 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -35,8 +35,6 @@
 #include <asm/tlb.h>
 #include <asm/mmu_context.h>
 
-#include <linux/checkpoint_types.h>
-#include <linux/checkpoint_hdr.h>
 #include <linux/checkpoint.h>
 
 #include "internal.h"
diff --git a/mm/shmem.c b/mm/shmem.c
index e123ec2..7d80ef8 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -31,8 +31,6 @@
 #include <linux/swap.h>
 #include <linux/ima.h>
 
-#include <linux/checkpoint_types.h>
-#include <linux/checkpoint_hdr.h>
 #include <linux/checkpoint.h>
 
 static struct vfsmount *shm_mnt;
-- 
1.6.2.3

Re: build breaks when checkpoint unimplemented by arch

[Nathan Lynch]

Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:

> On Mon, 6 Jul 2009, Nathan Lynch wrote:
>
>> Hi Oren,
>> 
>> With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:
>> 
>> In file included from include/linux/checkpoint.h:28,
>>                  from kernel/exit.c:53:
>> include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
>> In file included from include/linux/checkpoint.h:28,
>>                  from kernel/exit.c:53:
>> include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
>> make[1]: *** [kernel/exit.o] Error 1
>> 
>> 
>> It appears that any architecture which does not supply
>> asm/checkpoint_hdr.h is broken in the same way.
>> 
>> Either all architectures need to supply asm/checkpoint_hdr.h (and define
>> CKPT_ARCH_NSIG), or there needs to be some other fix which allows
>> as-yet-unsupported arches to build..
>> 
>
> I see... well - maybe it's time to resend the powerpc port :p

I'm working on that, but the powerpc port won't be of any help to the
twenty-odd other architectures that are broken.


> Until then, this patch worked for me to compile without c/r
>
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index c47e796..b8f99be 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -19,6 +19,7 @@
>  #define RESTART_TASKSELF	0x1
>  
>  #ifdef __KERNEL__
> +#ifdef CONFIG_CHECKPOINT
>  
>  #include <linux/sched.h>
>  #include <linux/nsproxy.h>
> @@ -299,6 +300,7 @@ extern unsigned long ckpt_debug_level;
>  
>  #endif /* CONFIG_CHECKPOINT_DEBUG */
>  
> +#endif /* CONFIG_CHECKPOINT */
>  #endif /* __KERNEL__ */

I think something like the following could be a better approach.

It needs to be split into separate patches with good commit messages,
but the gist is that asm/checkpoint_hdr.h should be included only by
files built when CONFIG_CHECKPOINT=y.

And linux/checkpoint_hdr.h doesn't really need it: all architectures
provide a sigset_t definition.

 arch/x86/mm/checkpoint.c       |    1 +
 checkpoint/checkpoint.c        |    1 +
 checkpoint/restart.c           |    1 +
 include/linux/checkpoint_hdr.h |    6 ++----
 4 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/arch/x86/mm/checkpoint.c b/arch/x86/mm/checkpoint.c
index 68432c8..3d21989 100644
--- a/arch/x86/mm/checkpoint.c
+++ b/arch/x86/mm/checkpoint.c
@@ -17,6 +17,7 @@
 
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
+#include <asm/checkpoint_hdr.h>
 
 /*
  * helpers to encode/decode/validate registers/segments/eflags
diff --git a/checkpoint/checkpoint.c b/checkpoint/checkpoint.c
index 2d4923c..6e90755 100644
--- a/checkpoint/checkpoint.c
+++ b/checkpoint/checkpoint.c
@@ -26,6 +26,7 @@
 #include <linux/hrtimer.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
+#include <asm/checkpoint_hdr.h>
 
 /* unique checkpoint identifier (FIXME: should be per-container ?) */
 static atomic_t ctx_count = ATOMIC_INIT(0);
diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 7982a03..afe2e1c 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -22,6 +22,7 @@
 #include <linux/elf.h>
 #include <linux/checkpoint.h>
 #include <linux/checkpoint_hdr.h>
+#include <asm/checkpoint_hdr.h>
 
 /**
  * _ckpt_read_objref - dispatch handling of a shared object
diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
index b5243e1..a2872a4 100644
--- a/include/linux/checkpoint_hdr.h
+++ b/include/linux/checkpoint_hdr.h
@@ -12,6 +12,7 @@
 
 #include <linux/types.h>
 #include <linux/utsname.h>
+#include <asm/signal.h>
 
 /*
  * To maintain compatibility between 32-bit and 64-bit architecture flavors,
@@ -39,9 +40,6 @@ struct ckpt_hdr {
 } __attribute__((aligned(8)));
 
 
-#include <asm/checkpoint_hdr.h>
-
-
 /* header types */
 enum {
 	CKPT_HDR_HEADER = 1,
@@ -407,7 +405,7 @@ struct ckpt_hdr_pgarr {
 
 /* signals */
 struct ckpt_hdr_sigset {
-	__u8 sigset[CKPT_ARCH_NSIG / 8];
+	sigset_t sigset;
 } __attribute__((aligned(8)));
 
 struct ckpt_hdr_sigaction {

Re: build breaks when checkpoint unimplemented by arch

[Oren Laadan]

On Mon, 6 Jul 2009, Nathan Lynch wrote:

> Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
> 
> > On Mon, 6 Jul 2009, Nathan Lynch wrote:
> >
> >> Hi Oren,
> >> 
> >> With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:
> >> 
> >> In file included from include/linux/checkpoint.h:28,
> >>                  from kernel/exit.c:53:
> >> include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
> >> In file included from include/linux/checkpoint.h:28,
> >>                  from kernel/exit.c:53:
> >> include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
> >> make[1]: *** [kernel/exit.o] Error 1
> >> 
> >> 
> >> It appears that any architecture which does not supply
> >> asm/checkpoint_hdr.h is broken in the same way.
> >> 
> >> Either all architectures need to supply asm/checkpoint_hdr.h (and define
> >> CKPT_ARCH_NSIG), or there needs to be some other fix which allows
> >> as-yet-unsupported arches to build..
> >> 
> >
> > I see... well - maybe it's time to resend the powerpc port :p
> 
> I'm working on that, but the powerpc port won't be of any help to the
> twenty-odd other architectures that are broken.
> 

[...]

That's what I tried initially, but the problem is that sigset_t may
be defined differently for userspace - see /usr/include/asm/sigset_t.h.
In fact, for x86_32, it it is different, defined as 'unsigned long' 
(and NSIG defined as 32, so only 32 bits).

Moreover, if you include <asm/sigset.h> in checkpoint_hdr.h, which is
also included by userspace, you get lots of compilations warnings,
because of other stuff included from the kernel's asm/sigset.h that
isn't supposed to be included by userspace.

So the introduction of CKPT_ARCH_NSIG is a workaround that.

And back to the other problem - I agree, kernel should only include
<linux/checkpoint.h>, which in turn will include <checkpoint_hdr.h>
if defined CONFIG_CHECKPOINT.

Can you try this patch:

(Hallyn: note that there is some checkpoint-related code within
kerbel/capability.c that should be inside #ifdef CONFIG_CHECKPOINT).

Oren.


diff --git a/fs/pipe.c b/fs/pipe.c
index 5d4c1c8..68fdab4 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -19,12 +19,11 @@
 #include <linux/pagemap.h>
 #include <linux/audit.h>
 #include <linux/syscalls.h>
+#include <linux/checkpoint.h>
 
 #include <asm/uaccess.h>
 #include <asm/ioctls.h>
 
-#include <linux/checkpoint.h>
-#include <linux/checkpoint_hdr.h>
 
 /*
  * We use a start+len construction, which provides full use of the 
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index c47e796..b8f99be 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -19,6 +19,7 @@
 #define RESTART_TASKSELF	0x1
 
 #ifdef __KERNEL__
+#ifdef CONFIG_CHECKPOINT
 
 #include <linux/sched.h>
 #include <linux/nsproxy.h>
@@ -299,6 +300,7 @@ extern unsigned long ckpt_debug_level;
 
 #endif /* CONFIG_CHECKPOINT_DEBUG */
 
+#endif /* CONFIG_CHECKPOINT */
 #endif /* __KERNEL__ */
 
 #endif /* _LINUX_CHECKPOINT_H_ */
diff --git a/ipc/shm.c b/ipc/shm.c
index 0991134..516b179 100644
--- a/ipc/shm.c
+++ b/ipc/shm.c
@@ -40,7 +40,6 @@
 #include <linux/mount.h>
 #include <linux/ipc_namespace.h>
 #include <linux/ima.h>
-#include <linux/checkpoint_hdr.h>
 #include <linux/checkpoint.h>
 
 #include <asm/uaccess.h>
diff --git a/kernel/capability.c b/kernel/capability.c
index 90cc7b4..4f58454 100644
--- a/kernel/capability.c
+++ b/kernel/capability.c
@@ -15,7 +15,7 @@
 #include <linux/syscalls.h>
 #include <linux/pid_namespace.h>
 #include <linux/securebits.h>
-#include <linux/checkpoint_hdr.h>
+#include <linux/checkpoint.h>
 #include <asm/uaccess.h>
 #include "cred-internals.h"
 
@@ -375,6 +375,7 @@ static inline int restore_cap_bset(kernel_cap_t bset, struct cred *cred)
 }
 #endif /* CONFIG_SECURITY_FILE_CAPABILITIES */
 
+#ifdef CONFIG_CHECKPOINT
 static int do_restore_caps(struct ckpt_capabilities *h, struct cred *cred)
 {
 	kernel_cap_t effective, inheritable, permitted, bset;
@@ -428,6 +429,7 @@ int restore_capabilities(struct ckpt_capabilities *h, struct cred *new)
 
 	return ret;
 }
+#endif /* CONFIG_CHECKPOINT */
 
 /**
  * capable - Determine if the current task has a superior capability in effect
diff --git a/mm/filemap.c b/mm/filemap.c
index 782664d..202bd74 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -34,11 +34,9 @@
 #include <linux/hardirq.h> /* for BUG_ON(!in_atomic()) only */
 #include <linux/memcontrol.h>
 #include <linux/mm_inline.h> /* for page_is_file_cache() */
+#include <linux/checkpoint.h>
 #include "internal.h"
 
-#include <linux/checkpoint_types.h>
-#include <linux/checkpoint_hdr.h>
-#include <linux/checkpoint.h>
 
 /*
  * FIXME: remove all knowledge of the buffer layer from the core VM
diff --git a/mm/mmap.c b/mm/mmap.c
index e60424b..4c01a90 100644
--- a/mm/mmap.c
+++ b/mm/mmap.c
@@ -29,16 +29,13 @@
 #include <linux/rmap.h>
 #include <linux/mmu_notifier.h>
 #include <linux/perf_counter.h>
+#include <linux/checkpoint.h>
 
 #include <asm/uaccess.h>
 #include <asm/cacheflush.h>
 #include <asm/tlb.h>
 #include <asm/mmu_context.h>
 
-#include <linux/checkpoint_types.h>
-#include <linux/checkpoint_hdr.h>
-#include <linux/checkpoint.h>
-
 #include "internal.h"
 
 #ifndef arch_mmap_check
diff --git a/mm/shmem.c b/mm/shmem.c
index e123ec2..9334810 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -30,9 +30,6 @@
 #include <linux/module.h>
 #include <linux/swap.h>
 #include <linux/ima.h>
-
-#include <linux/checkpoint_types.h>
-#include <linux/checkpoint_hdr.h>
 #include <linux/checkpoint.h>
 
 static struct vfsmount *shm_mnt;

Re: build breaks when checkpoint unimplemented by arch

[Nathan Lynch]

Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
> That's what I tried initially, but the problem is that sigset_t may
> be defined differently for userspace - see /usr/include/asm/sigset_t.h.
> In fact, for x86_32, it it is different, defined as 'unsigned long' 
> (and NSIG defined as 32, so only 32 bits).

I noticed this, but I figured only the kernel definition was salient.
Apart from debugging checkpoint/restart, why would userspace need the
definition of struct ckpt_hdr_sigset?

For that matter, why would userspace need the definitions of most of the
structures in checkpoint_hdr.h?  (Again, debugging purposes don't count:
ckptinfo or similar developer utilities can be included with the
kernel.)


> Moreover, if you include <asm/sigset.h> in checkpoint_hdr.h, which is
> also included by userspace, you get lots of compilations warnings,
> because of other stuff included from the kernel's asm/sigset.h that
> isn't supposed to be included by userspace.

Can you provide examples of such warnings?  Are you building against
"sanitized" kernel headers (make headers_install) or using them in
place?

Re: build breaks when checkpoint unimplemented by arch

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> On Mon, 6 Jul 2009, Nathan Lynch wrote:
> 
> > Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
> > 
> > > On Mon, 6 Jul 2009, Nathan Lynch wrote:
> > >
> > >> Hi Oren,
> > >> 
> > >> With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:
> > >> 
> > >> In file included from include/linux/checkpoint.h:28,
> > >>                  from kernel/exit.c:53:
> > >> include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
> > >> In file included from include/linux/checkpoint.h:28,
> > >>                  from kernel/exit.c:53:
> > >> include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
> > >> make[1]: *** [kernel/exit.o] Error 1
> > >> 
> > >> 
> > >> It appears that any architecture which does not supply
> > >> asm/checkpoint_hdr.h is broken in the same way.
> > >> 
> > >> Either all architectures need to supply asm/checkpoint_hdr.h (and define
> > >> CKPT_ARCH_NSIG), or there needs to be some other fix which allows
> > >> as-yet-unsupported arches to build..
> > >> 
> > >
> > > I see... well - maybe it's time to resend the powerpc port :p
> > 
> > I'm working on that, but the powerpc port won't be of any help to the
> > twenty-odd other architectures that are broken.
> > 
> 
> [...]
> 
> That's what I tried initially, but the problem is that sigset_t may
> be defined differently for userspace - see /usr/include/asm/sigset_t.h.
> In fact, for x86_32, it it is different, defined as 'unsigned long' 
> (and NSIG defined as 32, so only 32 bits).
> 
> Moreover, if you include <asm/sigset.h> in checkpoint_hdr.h, which is
> also included by userspace, you get lots of compilations warnings,
> because of other stuff included from the kernel's asm/sigset.h that
> isn't supposed to be included by userspace.
> 
> So the introduction of CKPT_ARCH_NSIG is a workaround that.
> 
> And back to the other problem - I agree, kernel should only include
> <linux/checkpoint.h>, which in turn will include <checkpoint_hdr.h>
> if defined CONFIG_CHECKPOINT.
> 
> Can you try this patch:

(Which looks exactly like my patch from yesterday.)

> (Hallyn: note that there is some checkpoint-related code within
> kerbel/capability.c that should be inside #ifdef CONFIG_CHECKPOINT).

Yup, the patch i sent yesterday took care of that.

So if that's the route we want to take, then again I can
confirm it did compile on ppc, and compile and boot on
s390.

> Oren.
> 
> 
> diff --git a/fs/pipe.c b/fs/pipe.c
> index 5d4c1c8..68fdab4 100644
> --- a/fs/pipe.c
> +++ b/fs/pipe.c
> @@ -19,12 +19,11 @@
>  #include <linux/pagemap.h>
>  #include <linux/audit.h>
>  #include <linux/syscalls.h>
> +#include <linux/checkpoint.h>
> 
>  #include <asm/uaccess.h>
>  #include <asm/ioctls.h>
> 
> -#include <linux/checkpoint.h>
> -#include <linux/checkpoint_hdr.h>
> 
>  /*
>   * We use a start+len construction, which provides full use of the 
> diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
> index c47e796..b8f99be 100644
> --- a/include/linux/checkpoint.h
> +++ b/include/linux/checkpoint.h
> @@ -19,6 +19,7 @@
>  #define RESTART_TASKSELF	0x1
> 
>  #ifdef __KERNEL__
> +#ifdef CONFIG_CHECKPOINT
> 
>  #include <linux/sched.h>
>  #include <linux/nsproxy.h>
> @@ -299,6 +300,7 @@ extern unsigned long ckpt_debug_level;
> 
>  #endif /* CONFIG_CHECKPOINT_DEBUG */
> 
> +#endif /* CONFIG_CHECKPOINT */
>  #endif /* __KERNEL__ */
> 
>  #endif /* _LINUX_CHECKPOINT_H_ */
> diff --git a/ipc/shm.c b/ipc/shm.c
> index 0991134..516b179 100644
> --- a/ipc/shm.c
> +++ b/ipc/shm.c
> @@ -40,7 +40,6 @@
>  #include <linux/mount.h>
>  #include <linux/ipc_namespace.h>
>  #include <linux/ima.h>
> -#include <linux/checkpoint_hdr.h>
>  #include <linux/checkpoint.h>
> 
>  #include <asm/uaccess.h>
> diff --git a/kernel/capability.c b/kernel/capability.c
> index 90cc7b4..4f58454 100644
> --- a/kernel/capability.c
> +++ b/kernel/capability.c
> @@ -15,7 +15,7 @@
>  #include <linux/syscalls.h>
>  #include <linux/pid_namespace.h>
>  #include <linux/securebits.h>
> -#include <linux/checkpoint_hdr.h>
> +#include <linux/checkpoint.h>
>  #include <asm/uaccess.h>
>  #include "cred-internals.h"
> 
> @@ -375,6 +375,7 @@ static inline int restore_cap_bset(kernel_cap_t bset, struct cred *cred)
>  }
>  #endif /* CONFIG_SECURITY_FILE_CAPABILITIES */
> 
> +#ifdef CONFIG_CHECKPOINT
>  static int do_restore_caps(struct ckpt_capabilities *h, struct cred *cred)
>  {
>  	kernel_cap_t effective, inheritable, permitted, bset;
> @@ -428,6 +429,7 @@ int restore_capabilities(struct ckpt_capabilities *h, struct cred *new)
> 
>  	return ret;
>  }
> +#endif /* CONFIG_CHECKPOINT */
> 
>  /**
>   * capable - Determine if the current task has a superior capability in effect
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 782664d..202bd74 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -34,11 +34,9 @@
>  #include <linux/hardirq.h> /* for BUG_ON(!in_atomic()) only */
>  #include <linux/memcontrol.h>
>  #include <linux/mm_inline.h> /* for page_is_file_cache() */
> +#include <linux/checkpoint.h>
>  #include "internal.h"
> 
> -#include <linux/checkpoint_types.h>
> -#include <linux/checkpoint_hdr.h>
> -#include <linux/checkpoint.h>
> 
>  /*
>   * FIXME: remove all knowledge of the buffer layer from the core VM
> diff --git a/mm/mmap.c b/mm/mmap.c
> index e60424b..4c01a90 100644
> --- a/mm/mmap.c
> +++ b/mm/mmap.c
> @@ -29,16 +29,13 @@
>  #include <linux/rmap.h>
>  #include <linux/mmu_notifier.h>
>  #include <linux/perf_counter.h>
> +#include <linux/checkpoint.h>
> 
>  #include <asm/uaccess.h>
>  #include <asm/cacheflush.h>
>  #include <asm/tlb.h>
>  #include <asm/mmu_context.h>
> 
> -#include <linux/checkpoint_types.h>
> -#include <linux/checkpoint_hdr.h>
> -#include <linux/checkpoint.h>
> -
>  #include "internal.h"
> 
>  #ifndef arch_mmap_check
> diff --git a/mm/shmem.c b/mm/shmem.c
> index e123ec2..9334810 100644
> --- a/mm/shmem.c
> +++ b/mm/shmem.c
> @@ -30,9 +30,6 @@
>  #include <linux/module.h>
>  #include <linux/swap.h>
>  #include <linux/ima.h>
> -
> -#include <linux/checkpoint_types.h>
> -#include <linux/checkpoint_hdr.h>
>  #include <linux/checkpoint.h>
> 
>  static struct vfsmount *shm_mnt;
> _______________________________________________
> Containers mailing list
> Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
> https://lists.linux-foundation.org/mailman/listinfo/containers

Re: build breaks when checkpoint unimplemented by arch

[Oren Laadan]

On Tue, 7 Jul 2009, Nathan Lynch wrote:

> Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
> > That's what I tried initially, but the problem is that sigset_t may
> > be defined differently for userspace - see /usr/include/asm/sigset_t.h.
> > In fact, for x86_32, it it is different, defined as 'unsigned long' 
> > (and NSIG defined as 32, so only 32 bits).
> 
> I noticed this, but I figured only the kernel definition was salient.
> Apart from debugging checkpoint/restart, why would userspace need the
> definition of struct ckpt_hdr_sigset?

I expect user space tools to at least:

- Assist in debugging c/r

- Assist users in reporting problems with c/r (especially since they
  themselves do not debug or hack)

- Convert checkpoint images from one kernel version to another

- Provide information about a checkpoint image, and even allow its
  manipulation.  This can assist developers in debugging their programs
  (e.g. to debug a crash you need to run a program for 30 minutes so it 
  ets up its state; instead of repeatedly running it, you run it once, 
  checkpoint, and then debug from a restarted version. A tool could 
  allow you to peek/poke inside the checkpoint and even modify data in 
  it).

- Or a tool that converts a checkpoint image to a core dump so it
  can be inspected with gdb.

I'm pretty sure others will find other uses to it...

> 
> For that matter, why would userspace need the definitions of most of the
> structures in checkpoint_hdr.h?  (Again, debugging purposes don't count:
> ckptinfo or similar developer utilities can be included with the
> kernel.)

Keeping the checkpoint header format understandable by user space (and 
immune to 32-64 variations) has been a requirement since day 1.

> 
> > Moreover, if you include <asm/sigset.h> in checkpoint_hdr.h, which is
> > also included by userspace, you get lots of compilations warnings,
> > because of other stuff included from the kernel's asm/sigset.h that
						      ^^^^^^^^^^^^^
					I meant:      asm/siginfo.h

> > isn't supposed to be included by userspace.
> 
> Can you provide examples of such warnings?  Are you building against
> "sanitized" kernel headers (make headers_install) or using them in
> place?
> 

Both.

Oren.

Re: build breaks when checkpoint unimplemented by arch

[Oren Laadan]

On Tue, 7 Jul 2009, Serge E. Hallyn wrote:

> Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> > On Mon, 6 Jul 2009, Nathan Lynch wrote:
> > 
> > > Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
> > > 
> > > > On Mon, 6 Jul 2009, Nathan Lynch wrote:
> > > >
> > > >> Hi Oren,
> > > >> 
> > > >> With ckpt-v17-rc1 (as well as ckpt-v16-dev) the powerpc build is broken:
> > > >> 
> > > >> In file included from include/linux/checkpoint.h:28,
> > > >>                  from kernel/exit.c:53:
> > > >> include/linux/checkpoint_hdr.h:42:32: error: asm/checkpoint_hdr.h: No such file or directory
> > > >> In file included from include/linux/checkpoint.h:28,
> > > >>                  from kernel/exit.c:53:
> > > >> include/linux/checkpoint_hdr.h:410: error: 'CKPT_ARCH_NSIG' undeclared here (not in a function)
> > > >> make[1]: *** [kernel/exit.o] Error 1
> > > >> 
> > > >> 
> > > >> It appears that any architecture which does not supply
> > > >> asm/checkpoint_hdr.h is broken in the same way.
> > > >> 
> > > >> Either all architectures need to supply asm/checkpoint_hdr.h (and define
> > > >> CKPT_ARCH_NSIG), or there needs to be some other fix which allows
> > > >> as-yet-unsupported arches to build..
> > > >> 
> > > >
> > > > I see... well - maybe it's time to resend the powerpc port :p
> > > 
> > > I'm working on that, but the powerpc port won't be of any help to the
> > > twenty-odd other architectures that are broken.
> > > 
> > 
> > [...]
> > 
> > That's what I tried initially, but the problem is that sigset_t may
> > be defined differently for userspace - see /usr/include/asm/sigset_t.h.
> > In fact, for x86_32, it it is different, defined as 'unsigned long' 
> > (and NSIG defined as 32, so only 32 bits).
> > 
> > Moreover, if you include <asm/sigset.h> in checkpoint_hdr.h, which is
> > also included by userspace, you get lots of compilations warnings,
> > because of other stuff included from the kernel's asm/sigset.h that
> > isn't supposed to be included by userspace.
> > 
> > So the introduction of CKPT_ARCH_NSIG is a workaround that.
> > 
> > And back to the other problem - I agree, kernel should only include
> > <linux/checkpoint.h>, which in turn will include <checkpoint_hdr.h>
> > if defined CONFIG_CHECKPOINT.
> > 
> > Can you try this patch:
> 
> (Which looks exactly like my patch from yesterday.)

... great minds, eh ?  (and one was really tired ;)

> 
> > (Hallyn: note that there is some checkpoint-related code within
> > kerbel/capability.c that should be inside #ifdef CONFIG_CHECKPOINT).
> 
> Yup, the patch i sent yesterday took care of that.
> 
> So if that's the route we want to take, then again I can
> confirm it did compile on ppc, and compile and boot on
> s390.
> 

Sound good. 

Oren.

[...]

Re: build breaks when checkpoint unimplemented by arch

[Serge E. Hallyn]

Quoting Oren Laadan (orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org):
> > So if that's the route we want to take, then again I can
> > confirm it did compile on ppc, and compile and boot on
> > s390.
> > 
> 
> Sound good. 

Cool, so feel free to add my Signed-off-by to your patch.

Nathan, this doesn't mean that I don't think it's worthwhile to
try and get rid of the #include where it's not needed, so if you
want to keep looking into any which can be replaced by a function
or struct prototype, all the better.

thanks,
-serge

Re: build breaks when checkpoint unimplemented by arch

[Nathan Lynch]

Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
> On Tue, 7 Jul 2009, Nathan Lynch wrote:
>
>> Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
>> > That's what I tried initially, but the problem is that sigset_t may
>> > be defined differently for userspace - see /usr/include/asm/sigset_t.h.
>> > In fact, for x86_32, it it is different, defined as 'unsigned long' 
>> > (and NSIG defined as 32, so only 32 bits).
>> 
>> I noticed this, but I figured only the kernel definition was salient.
>> Apart from debugging checkpoint/restart, why would userspace need the
>> definition of struct ckpt_hdr_sigset?
>
> I expect user space tools to at least:
>
> - Assist in debugging c/r
>
> - Assist users in reporting problems with c/r (especially since they
>   themselves do not debug or hack)
>
> - Convert checkpoint images from one kernel version to another
>
> - Provide information about a checkpoint image, and even allow its
>   manipulation.  This can assist developers in debugging their programs
>   (e.g. to debug a crash you need to run a program for 30 minutes so it 
>   ets up its state; instead of repeatedly running it, you run it once, 
>   checkpoint, and then debug from a restarted version. A tool could 
>   allow you to peek/poke inside the checkpoint and even modify data in 
>   it).
>
> - Or a tool that converts a checkpoint image to a core dump so it
>   can be inspected with gdb.
>
> I'm pretty sure others will find other uses to it...

But I asked specifically about ckpt_hdr_sigset.


>> For that matter, why would userspace need the definitions of most of the
>> structures in checkpoint_hdr.h?  (Again, debugging purposes don't count:
>> ckptinfo or similar developer utilities can be included with the
>> kernel.)
>
> Keeping the checkpoint header format understandable by user space (and 
> immune to 32-64 variations) has been a requirement since day 1.

I guess I wasn't around that day.  It seems backwards to expose the
format of every checkpoint record in the ABI regardless of whether
plausible use cases exist.  Linux has a well-established pattern of
introducing interfaces without sufficient testing or documentation[1],
and I expect C/R will adhere to tradition.  Making the ABI obese in the
hope of anticipating every conceivable use will just provide more
opportunities to screw up.

[1] http://userweb.kernel.org/~mtk/papers/lce2007/What_we_lose_without_words.pdf

Re: build breaks when checkpoint unimplemented by arch

[Oren Laadan]

Nathan Lynch wrote:
> Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
>> On Tue, 7 Jul 2009, Nathan Lynch wrote:
>>
>>> Oren Laadan <orenl-eQaUEPhvms7ENvBUuze7eA@public.gmane.org> writes:
>>>> That's what I tried initially, but the problem is that sigset_t may
>>>> be defined differently for userspace - see /usr/include/asm/sigset_t.h.
>>>> In fact, for x86_32, it it is different, defined as 'unsigned long' 
>>>> (and NSIG defined as 32, so only 32 bits).
>>> I noticed this, but I figured only the kernel definition was salient.
>>> Apart from debugging checkpoint/restart, why would userspace need the
>>> definition of struct ckpt_hdr_sigset?
>> I expect user space tools to at least:
>>
>> - Assist in debugging c/r
>>
>> - Assist users in reporting problems with c/r (especially since they
>>   themselves do not debug or hack)
>>
>> - Convert checkpoint images from one kernel version to another
>>
>> - Provide information about a checkpoint image, and even allow its
>>   manipulation.  This can assist developers in debugging their programs
>>   (e.g. to debug a crash you need to run a program for 30 minutes so it 
>>   ets up its state; instead of repeatedly running it, you run it once, 
>>   checkpoint, and then debug from a restarted version. A tool could 
>>   allow you to peek/poke inside the checkpoint and even modify data in 
>>   it).
>>
>> - Or a tool that converts a checkpoint image to a core dump so it
>>   can be inspected with gdb.
>>
>> I'm pretty sure others will find other uses to it...
> 
> But I asked specifically about ckpt_hdr_sigset.
> 
> 
>>> For that matter, why would userspace need the definitions of most of the
>>> structures in checkpoint_hdr.h?  (Again, debugging purposes don't count:
>>> ckptinfo or similar developer utilities can be included with the
>>> kernel.)
>> Keeping the checkpoint header format understandable by user space (and 
>> immune to 32-64 variations) has been a requirement since day 1.
> 
> I guess I wasn't around that day.  It seems backwards to expose the
> format of every checkpoint record in the ABI regardless of whether
> plausible use cases exist.  Linux has a well-established pattern of
> introducing interfaces without sufficient testing or documentation[1],
> and I expect C/R will adhere to tradition.  Making the ABI obese in the
> hope of anticipating every conceivable use will just provide more
> opportunities to screw up.
> 
> [1] http://userweb.kernel.org/~mtk/papers/lce2007/What_we_lose_without_words.pdf

I could not agree more !

The intent of exposure to userspace is not to establish an ABI, but
solely to allow *specialized* c/r-related user tools to understand
such data, per kernel version.

On the contrary: it is expected to change between kernel versions
and break compatibility with older version, on a regular basis.
That is why we plan to do conversion of checkpoint images between
kernel version in userspace.

I view it as a "window" for userspace to glance at how checkpoint
image for a specific kernel version is defined. And it comes as is,
no-strings-attached, with nothing but a promise to likely break it
on the next release.

This begs the question: how to make sure that this message is clear
and is not misinterpreted ?   Or (and I'm no API expert) - perhaps
there is a better way...

Oren.