[Devel] Re: [PATCH 1/3] Checkpoint/restart epoll sets

[Matt Helsley]

On Tue, Oct 20, 2009 at 07:31:28PM -0500, Serge E. Hallyn wrote:
> Quoting Matt Helsley (matthltc at us.ibm.com):

<snip>

> > +struct file* ep_file_restore(struct ckpt_ctx *ctx,
> > +			     struct ckpt_hdr_file *h)
> > +{
> > +	struct file *epfile;
> > +	int epfd, ret;
> > +
> > +	if (h->h.type != CKPT_HDR_FILE ||
> > +	    h->h.len  != sizeof(*h) ||
> > +	    h->f_type != CKPT_FILE_EPOLL)
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	epfd = sys_epoll_create1(h->f_flags & EPOLL_CLOEXEC);
> > +	if (epfd < 0)
> > +		return ERR_PTR(epfd);
> > +	epfile = fget(epfd);
> > +	sys_close(epfd); /* harmless even if an error occured */
> > +	BUG_ON(!epfile);
> 
> Would perhaps return ERR_PTR(-ENOENT) be nicer?  (And maybe safer - I'm
> not quite clear on under which arches BUG_ON does nothing).

OK, good idea. Mind if I post it as a separate patch to be merged with
this series?

> 
> > +
> > +	/*
> > +	 * Needed before we can properly restore the watches and enforce the
> > +	 * limit on watch numbers.
> > +	 */
> > +	ret = restore_file_common(ctx, epfile, h);
> > +	if (ret < 0)
> > +		goto fput_out;
> > +
> > +	/*
> > +	 * Defer restoring the epoll items until the file table is
> > +	 * fully restored. Ensures that valid file objrefs will resolve.
> > +	 */
> > +	ret = deferqueue_add_ptr(ctx->files_deferq, ctx, ep_items_restore, NULL);
> > +	if (ret < 0) {
> > +fput_out:
> > +		fput(epfile);
> > +		epfile = ERR_PTR(ret);
> > +	}
> > +	return epfile;
> > +}
> > +
> > +#endif /* CONFIG_CHECKPOINT */
> > +
> >  static int __init eventpoll_init(void)
> >  {
> >  	struct sysinfo si;
> > diff --git a/include/linux/checkpoint_hdr.h b/include/linux/checkpoint_hdr.h
> > index ca2500d..1a3edab 100644
> > --- a/include/linux/checkpoint_hdr.h
> > +++ b/include/linux/checkpoint_hdr.h
> > @@ -119,6 +119,8 @@ enum {
> >  #define CKPT_HDR_TTY CKPT_HDR_TTY
> >  	CKPT_HDR_TTY_LDISC,
> >  #define CKPT_HDR_TTY_LDISC CKPT_HDR_TTY_LDISC
> > +	CKPT_HDR_EPOLL_ITEMS = 391, /* Follows file-table */
> 
> What is the comment supposed to mean (other than that such
> comments inevitably become stale :)?

It means that these items must be present some point after the
file-table in order for the checkpoint image to restart. That's
why I chose a number near 400  -- it looked like [300-400) were set 
aside for file objects.

Cheers,
	-Matt Helsley
_______________________________________________
Containers mailing list
Containers at lists.linux-foundation.org
https://lists.linux-foundation.org/mailman/listinfo/containers