[lxc-devel] [PATCH] lxcapi_restore shouldn't steal the calling process

St├ęphane Graber stgraber at ubuntu.com
Mon Apr 6 16:03:55 UTC 2015


On Fri, Apr 03, 2015 at 11:02:11AM -0600, Tycho Andersen wrote:
> On Fri, Apr 03, 2015 at 04:41:01PM +0000, Serge Hallyn wrote:
> > Quoting Tycho Andersen (tycho.andersen at canonical.com):
> > > Previously, lxcapi_restore used the calling process as the lxc monitor process
> > > (and just never returned), requiring users to fork before calling it. This, of
> > > course, would cause problems for things like LXD, which can't fork.
> > > 
> > > Now, restore() forks the monitor as a child of the process that calls it. Users
> > > who want to daemonize the restore process need to fork themselves.
> > > lxc-checkpoint has been updated to reflect this behavior change.
> > > 
> > > Signed-off-by: Tycho Andersen <tycho.andersen at canonical.com>
> > 
> > Thanks, Tycho.  I'm ok with it as is,
> > 
> > Acked-by: Serge E. Hallyn <serge.hallyn at ubuntu.com>
> > 
> > but there's one thing that could stand improvement (with a later
> > patch).  The lxcapi_restore() will hang on many errors in the
> > restoring task bc you don't always send a failure status back over the
> > socket.  Yo ucoudl either alway ssend a failure status, or you could
> > probably just select() before the read, though you'd have to guess at a
> > decent timeout.
> 
> Good point. I think always sending a failure status sounds good; I'll
> send a patch for that soon.
> 
> Thanks,
> 
> Tycho

This patch is failing to apply to current git master.

Please send a rebase version.

Thanks

> 
> > > ---
> > >  src/lxc/lxc_checkpoint.c |  48 +++++++++++++------
> > >  src/lxc/lxccontainer.c   | 121 +++++++++++++++++++++++++++++++++++------------
> > >  2 files changed, 125 insertions(+), 44 deletions(-)
> > > 
> > > diff --git a/src/lxc/lxc_checkpoint.c b/src/lxc/lxc_checkpoint.c
> > > index cfa08fc..2e76c2e 100644
> > > --- a/src/lxc/lxc_checkpoint.c
> > > +++ b/src/lxc/lxc_checkpoint.c
> > > @@ -20,6 +20,8 @@
> > >  #include <stdio.h>
> > >  #include <errno.h>
> > >  #include <unistd.h>
> > > +#include <sys/types.h>
> > > +#include <sys/wait.h>
> > >  
> > >  #include <lxc/lxccontainer.h>
> > >  
> > > @@ -27,6 +29,7 @@
> > >  #include "config.h"
> > >  #include "lxc.h"
> > >  #include "arguments.h"
> > > +#include "utils.h"
> > >  
> > >  static char *checkpoint_dir = NULL;
> > >  static bool stop = false;
> > > @@ -139,36 +142,53 @@ bool checkpoint(struct lxc_container *c)
> > >  	return true;
> > >  }
> > >  
> > > -bool restore(struct lxc_container *c)
> > > +bool restore_finalize(struct lxc_container *c)
> > >  {
> > > -	pid_t pid = 0;
> > > -	bool ret = true;
> > > +	bool ret = c->restore(c, checkpoint_dir, verbose);
> > > +	if (!ret) {
> > > +		fprintf(stderr, "Restoring %s failed.\n", my_args.name);
> > > +	}
> > >  
> > > +	lxc_container_put(c);
> > > +	return ret;
> > > +}
> > > +
> > > +bool restore(struct lxc_container *c)
> > > +{
> > >  	if (c->is_running(c)) {
> > >  		fprintf(stderr, "%s is running, not restoring.\n", my_args.name);
> > >  		lxc_container_put(c);
> > >  		return false;
> > >  	}
> > >  
> > > -	if (my_args.daemonize)
> > > +	if (my_args.daemonize) {
> > > +		pid_t pid;
> > > +
> > >  		pid = fork();
> > > +		if (pid < 0) {
> > > +			perror("fork");
> > > +			return false;
> > > +		}
> > >  
> > > -	if (pid == 0) {
> > > -		if (my_args.daemonize) {
> > > +		if (pid == 0) {
> > >  			close(0);
> > >  			close(1);
> > > -		}
> > >  
> > > -		ret = c->restore(c, checkpoint_dir, verbose);
> > > -
> > > -		if (!ret) {
> > > -			fprintf(stderr, "Restoring %s failed.\n", my_args.name);
> > > +			exit(!restore_finalize(c));
> > > +		} else {
> > > +			return wait_for_pid(pid) == 0;
> > >  		}
> > > -	}
> > > +	} else {
> > > +		int status;
> > >  
> > > -	lxc_container_put(c);
> > > +		if (!restore_finalize(c))
> > > +			return false;
> > >  
> > > -	return ret;
> > > +		if (waitpid(-1, &status, 0) < 0)
> > > +			return false;
> > > +
> > > +		return WIFEXITED(status) && WEXITSTATUS(status) == 0;
> > > +	}
> > >  }
> > >  
> > >  int main(int argc, char *argv[])
> > > diff --git a/src/lxc/lxccontainer.c b/src/lxc/lxccontainer.c
> > > index 4422f4a..c3369cd 100644
> > > --- a/src/lxc/lxccontainer.c
> > > +++ b/src/lxc/lxccontainer.c
> > > @@ -3853,28 +3853,20 @@ out_unlock:
> > >  	return !has_error;
> > >  }
> > >  
> > > -static bool lxcapi_restore(struct lxc_container *c, char *directory, bool verbose)
> > > +// do_restore never returns, the calling process is used as the
> > > +// monitor process. do_restore calls exit() if it fails.
> > > +static void do_restore(struct lxc_container *c, int pipe, char *directory, bool verbose)
> > >  {
> > >  	pid_t pid;
> > > -	struct lxc_rootfs *rootfs;
> > >  	char pidfile[L_tmpnam];
> > >  	struct lxc_handler *handler;
> > > -	bool has_error = true;
> > > -
> > > -	if (!criu_ok(c))
> > > -		return false;
> > > -
> > > -	if (geteuid()) {
> > > -		ERROR("Must be root to restore\n");
> > > -		return false;
> > > -	}
> > >  
> > >  	if (!tmpnam(pidfile))
> > > -		return false;
> > > +		exit(1);
> > >  
> > >  	handler = lxc_init(c->name, c->lxc_conf, c->config_path);
> > >  	if (!handler)
> > > -		return false;
> > > +		exit(1);
> > >  
> > >  	if (!cgroup_init(handler)) {
> > >  		ERROR("failed initing cgroups");
> > > @@ -3897,9 +3889,10 @@ static bool lxcapi_restore(struct lxc_container *c, char *directory, bool verbos
> > >  
> > >  	if (pid == 0) {
> > >  		struct criu_opts os;
> > > +		struct lxc_rootfs *rootfs;
> > >  
> > >  		if (unshare(CLONE_NEWNS))
> > > -			exit(1);
> > > +			goto out_fini_handler;
> > >  
> > >  		/* CRIU needs the lxc root bind mounted so that it is the root of some
> > >  		 * mount. */
> > > @@ -3907,15 +3900,14 @@ static bool lxcapi_restore(struct lxc_container *c, char *directory, bool verbos
> > >  
> > >  		if (rootfs_is_blockdev(c->lxc_conf)) {
> > >  			if (do_rootfs_setup(c->lxc_conf, c->name, c->config_path) < 0)
> > > -				exit(1);
> > > -		}
> > > -		else {
> > > +				goto out_fini_handler;
> > > +		} else {
> > >  			if (mkdir(rootfs->mount, 0755) < 0 && errno != EEXIST)
> > > -				exit(1);
> > > +				goto out_fini_handler;
> > >  
> > >  			if (mount(rootfs->path, rootfs->mount, NULL, MS_BIND, NULL) < 0) {
> > >  				rmdir(rootfs->mount);
> > > -				exit(1);
> > > +				goto out_fini_handler;
> > >  			}
> > >  		}
> > >  
> > > @@ -3930,22 +3922,30 @@ static bool lxcapi_restore(struct lxc_container *c, char *directory, bool verbos
> > >  		exec_criu(&os);
> > >  		umount(rootfs->mount);
> > >  		rmdir(rootfs->mount);
> > > -		exit(1);
> > > +		goto out_fini_handler;
> > >  	} else {
> > > -		int status;
> > > +		int status, ret;
> > > +		char title[2048];
> > >  
> > >  		pid_t w = waitpid(pid, &status, 0);
> > > -
> > >  		if (w == -1) {
> > >  			perror("waitpid");
> > >  			goto out_fini_handler;
> > >  		}
> > >  
> > > +		ret = write(pipe, &status, sizeof(status));
> > > +		close(pipe);
> > > +
> > > +		if (sizeof(status) != ret) {
> > > +			perror("write");
> > > +			ERROR("failed to write all of status");
> > > +			goto out_fini_handler;
> > > +		}
> > > +
> > >  		if (WIFEXITED(status)) {
> > >  			if (WEXITSTATUS(status)) {
> > >  				goto out_fini_handler;
> > > -			}
> > > -			else {
> > > +			} else {
> > >  				int ret;
> > >  				FILE *f = fopen(pidfile, "r");
> > >  				if (!f) {
> > > @@ -3969,17 +3969,78 @@ static bool lxcapi_restore(struct lxc_container *c, char *directory, bool verbos
> > >  			goto out_fini_handler;
> > >  		}
> > >  
> > > -		if (lxc_poll(c->name, handler)) {
> > > +		/*
> > > +		 * See comment in lxcapi_start; we don't care if these
> > > +		 * fail because it's just a beauty thing. We just
> > > +		 * assign the return here to silence potential.
> > > +		 */
> > > +		ret = snprintf(title, sizeof(title), "[lxc monitor] %s %s", c->config_path, c->name);
> > > +		ret = setproctitle(title);
> > > +
> > > +		ret = lxc_poll(c->name, handler);
> > > +		if (ret)
> > >  			lxc_abort(c->name, handler);
> > > -			goto out_fini_handler;
> > > -		}
> > > +		lxc_fini(c->name, handler);
> > > +		exit(ret);
> > >  	}
> > >  
> > > -	has_error = false;
> > > -
> > >  out_fini_handler:
> > >  	lxc_fini(c->name, handler);
> > > -	return !has_error;
> > > +	exit(1);
> > > +}
> > > +
> > > +static bool lxcapi_restore(struct lxc_container *c, char *directory, bool verbose)
> > > +{
> > > +	pid_t pid;
> > > +	int status, nread;
> > > +	int pipefd[2];
> > > +
> > > +	if (!criu_ok(c))
> > > +		return false;
> > > +
> > > +	if (geteuid()) {
> > > +		ERROR("Must be root to restore\n");
> > > +		return false;
> > > +	}
> > > +
> > > +	if (pipe(pipefd)) {
> > > +		ERROR("failed to create pipe");
> > > +		return false;
> > > +	}
> > > +
> > > +	pid = fork();
> > > +	if (pid < 0) {
> > > +		close(pipefd[0]);
> > > +		close(pipefd[1]);
> > > +		return false;
> > > +	}
> > > +
> > > +	if (pid == 0) {
> > > +		close(pipefd[0]);
> > > +		// this never returns
> > > +		do_restore(c, pipefd[1], directory, verbose);
> > > +	}
> > > +
> > > +	close(pipefd[1]);
> > > +
> > > +	nread = read(pipefd[0], &status, sizeof(status));
> > > +	close(pipefd[0]);
> > > +	if (sizeof(status) != nread) {
> > > +		ERROR("reading status from pipe failed");
> > > +		goto err_wait;
> > > +	}
> > > +
> > > +	// If the criu process was killed or exited nonzero, wait() for the
> > > +	// handler, since the restore process died. Otherwise, we don't need to
> > > +	// wait, since the child becomes the monitor process.
> > > +	if (!WIFEXITED(status) || WEXITSTATUS(status))
> > > +		goto err_wait;
> > > +	return true;
> > > +
> > > +err_wait:
> > > +	if (wait_for_pid(pid))
> > > +		ERROR("restore process died");
> > > +	return false;
> > >  }
> > >  
> > >  static int lxcapi_attach_run_waitl(struct lxc_container *c, lxc_attach_options_t *options, const char *program, const char *arg, ...)
> > > -- 
> > > 2.1.0
> > > 
> > > _______________________________________________
> > > lxc-devel mailing list
> > > lxc-devel at lists.linuxcontainers.org
> > > http://lists.linuxcontainers.org/listinfo/lxc-devel
> > _______________________________________________
> > lxc-devel mailing list
> > lxc-devel at lists.linuxcontainers.org
> > http://lists.linuxcontainers.org/listinfo/lxc-devel
> _______________________________________________
> lxc-devel mailing list
> lxc-devel at lists.linuxcontainers.org
> http://lists.linuxcontainers.org/listinfo/lxc-devel

-- 
St├ęphane Graber
Ubuntu developer
http://www.ubuntu.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://lists.linuxcontainers.org/pipermail/lxc-devel/attachments/20150406/bfc5dcc5/attachment.sig>


More information about the lxc-devel mailing list