Quoting Matt Helsley (matthltc@xxxxxxxxxx):
> On Fri, Apr 24, 2009 at 04:06:08PM -0500, Serge E. Hallyn wrote:
Thanks for taking a look, Matt. Oren has done some nice work to it
in his ckpt-v14, please take a look there.
> > + cnt = ref->users + 1;
>
> Perhaps this switch is another candidate for an fops-style function pointer.
Yup, Oren did that :)
> > +static int cr_check_leaks(struct cr_ctx *ctx)
>
> What are "leaks" ? ;)
>
> How about cr_find_ref_leaks ? I suggest "find" because "check" confuses with
> "checkpoint". "ref" because we're not looking for memory leaks.
Oren renamed it to ckpt_obj_contained()
...
> > mm = get_task_mm(t);
> >
>
> Might add a check for exe_file == NULL here too (see below).
Hmm, I think it's ok to store a NULL pointer in the objhash...
...
> > + if (!exe_file) {
> > + /* really it can't be the case that it NOT be NULL... */
>
> This comment isn't correct. Yes, *most* of the time exe_file should be
> non-NULL.
You misread the comment, though you're right about the code.
The comment says it can't be the case that it *not* be NULL here.
That's because the object can't be in the objhash yet, because it always
comes after the main mm entry in the layout.
> Some tools avoid pinning the filesystem with a reference to its executable file
> by copying the executable into private, anonymous, executable pages,
> and then unmaping the originals. This drops the last VMA reference to the
> file which causes the exe_file reference to be dropped as well.
>
> It may provide an interesting testcase for checkpoint/restart since it
> would mean the executable couldn't be mapped from a checkpointed file --
> we'd have to rely solely on VMA reconstruction for these.
Yeah, taking another look I don't handle the restart case where exe_file
is NULL. (It won't harm the kernel, just return an error from
checkpoint I believe)
How does a userspace tool do that though? Once the file has been
exec()d, userspace doesn't have an open fd to the executable anymore...
Can you explain how it's done, and/or send me a testcase?
> > /* cr_ctx: flags */
> > -#define CR_CTX_CKPT 0x1
> > -#define CR_CTX_RSTR 0x2
> > +#define CR_CTX_CKPT 0x1
> > +#define CR_CTX_RSTR 0x2
> > +#define CHECKPOINT_SUBTREE 0x4
>
> The whitespace change somewhat obscures the introduction of the flag here.
True. Guess that could've been a separate tiny patch.
thanks,
-serge
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
