[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