[I'm sorry if I'm flogging a dead horse. I'm actually really excited by this functionality! :-) ] Comments inline... On Tue, Jun 2, 2009 at 7:23 AM, Serge E. Hallyn <serue@xxxxxxxxxx> wrote: > Quoting Andrew G. Morgan (morgan@xxxxxxxxxx): >> On Mon, Jun 1, 2009 at 3:18 PM, Serge E. Hallyn <serue@xxxxxxxxxx> wrote: >> > Quoting Andrew G. Morgan (morgan@xxxxxxxxxx): >> >> On Mon, Jun 1, 2009 at 6:35 AM, Serge E. Hallyn <serue@xxxxxxxxxx> 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. > > 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; > }; Yes, something like this, but perhaps: struct ckpt_caps_part_s { int length; /* = sizeof(struct ckpt_capabilities) */ cap_ckpt_t data; } caps; and then the generic checkpoint code would do: #include <linux/capabilities.h> caps.ckpt_capabilities_length = cap_checkpoint_save(&caps.data); [...] cap_checkpoint_restore(caps.length, &caps.caps.data); and the capability code could opaquely deal with the details. The reason I think this is more maintainable is that its clear (to the capability code) what is being check-pointed and, conversely, for the checkpoint code it is abstracted with the responsibility for detailed state decisions elsewhere in the kernel. I suspect I don't understand the user space code issue sufficiently. But if, for some reason, the user space source code is unable to include the definition of cap_ckpt_t, it should be clear that parsing this type of data structure, given that offsets are embedded in it, should be straightforward. > with a BUILD_BUG_ON to ensure that sizeof(r)==sizeof(d). Ugly, but > should suit everyone? could the checkpointing code check the return value for cap_checkpoint_restore() and fail the restore if it returned an error? Cheers Andrew > >> > 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 > -- > To unsubscribe from this list: send the line "unsubscribe linux-security-module" in > the body of a message to majordomo@xxxxxxxxxxxxxxx > More majordomo info at http://vger.kernel.org/majordomo-info.html > _______________________________________________ Containers mailing list Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx https://lists.linux-foundation.org/mailman/listinfo/containers