Re: [PATCH 5/9] cr: capabilities: define checkpoint and restore fns

[Oren Laadan <orenl@cs.columbia.edu>]

Serge E. Hallyn wrote:
> Quoting Andrew G. Morgan (morgan@kernel.org):
>> On Mon, Jun 1, 2009 at 3:18 PM, Serge E. Hallyn <serue@us.ibm.com> wrote:
>>> Quoting Andrew G. Morgan (morgan@kernel.org):
>>>> On Mon, Jun 1, 2009 at 6:35 AM, Serge E. Hallyn <serue@us.ibm.com> wrote:
>>>>>>> I'll put in a commented BUILD_BUG_ON like Alexey suggests - does that
>>>>>>> suffice?
>>>> I can't speak for other subsystems, but it seems to me as if for the
>>>> capabilities, I'd want to create something like this in
>>>> include/linux/capabilities.h
>>>>
>>>> typedef struct checkpoint_caps_s {
>>>>    /* what goes in here is the capability code's business */
>>>> } checkpoint_caps_t;
>>> Sigh - Did a patch this way, but the problem is userspace needs to be
>>> able to parse the checkpoint image, so it needs to know what this struct
>>> looks like.  So if I put it the struct definition
>>> include/linux/capability.h, I run into a whole new set of problems
>>> trying to compile a userspace program to do a sys_restart().
>> Does the user space app need to be able to modify the data in some
>> way? It seems like embedding a length with the structure or something
>> might simplify such a user space dependency.

Userspace needs to be able not only to parse the checkpoint image
but also to understand the contents:

1) For analysis and debugging tool(s) that will give information
about a checkpoint image

2) For conversion tool(s) that will convert a checkpoint image from
an older to a newer kernel. The idea is to avoid accumulating endless
compatibility code in the kernel.

So yes, these userspace apps need to be able to look and perhaps
modify the data in some way.

However, this raises an interesting question of _how_ to do this even
in userspace. Suppose we want to convert from version X to version Y,
where struct ckpt_blah changed between the version.

What would be an effective way to allow userspace to include both the
old and the new ckpt_blah and have them named differently ?

Oren.

> 
> Hmm, I suppose I could do something like define struct ckpt_capabilities
> in capabilities.h, then in checkpoint_hdr.h do
> 
> struct ckpt_capabilities;
> struct ckpt_cap_dummy {
> 	__u64 dummies[9];
> };
> 
> struct ckpt_hdr_cred {
> 	...
> 	union {
> 		struct ckpt_capabilities r;
> 		struct ckpt_cap_dummy d;
> 	} caps;
> };
> 
> with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d).  Ugly, but
> should suit everyone?
> 
>>> So I went part-way to what you suggested in the patchset I'm about to
>>> send out (please see patch 6/8).  I think the caps code does look
>>> nicer in this new version.
>> Better, but I remain concerned that the code looks hard to maintain
>> when structured this way.
> 
> Why exactly?  Just having the struct defined in checkpoint_hdr.h?  Or
> is there something else I'm unwittingly doing?
> 
> thanks,
> -serge
>