Hi,
I applied this changes (a modified version) as part of the
cleanup I mentioned to user-cr a few minutes ago ...
Thanks,
Oren.
On 04/09/2010 11:25 PM, Sukadev Bhattiprolu wrote:
> From: Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx>
> Date: Fri, 9 Apr 2010 18:36:46 -0700
> Subject: [RFC][PATCH] Remove exit() calls from app_restart().
>
> app_restart() will eventually become an API for USERCR. When it encounters
> an error, app_restart() should return an error code rather than exit. So
> replace (most) exit() calls in app_restart() with a return code. Of course
> there maybe situations where app_restart() may fail and leave some child
> processes, but hopefully this patch does not worsen those cases :-)
>
> NOTE: There are few exit() calls remaining in restart.c. These fall into
> two categories. First category of exit() calls are those made in
> child processes (i.e processes created by app_restart()). Those exits
> are fine (and required).
>
> The second category of the exit() calls are those in signal handlers.
> We should remove these exits when we define better API to address
> signal-handling in app_restart().
>
> Signed-off-by: Sukadev Bhattiprolu <sukadev@xxxxxxxxxxxxxxxxxx>
> ---
> restart.c | 102 +++++++++++++++++++++++++++++++++++-------------------------
> 1 files changed, 59 insertions(+), 43 deletions(-)
>
> diff --git a/restart.c b/restart.c
> index c5c65a2..3224335 100644
> --- a/restart.c
> +++ b/restart.c
> @@ -335,7 +335,7 @@ static void sigint_handler(int sig)
> static int freezer_prepare(struct ckpt_ctx *ctx)
> {
> char *freezer;
> - int fd, ret;
> + int fd, n, ret;
>
> #define FREEZER_THAWED "THAWED"
>
> @@ -345,25 +345,31 @@ static int freezer_prepare(struct ckpt_ctx *ctx)
> return -1;
> }
>
> + ret = -1;
> sprintf(freezer, "%s/freezer.state", ctx->args->freezer);
>
> fd = open(freezer, O_WRONLY, 0);
> if (fd < 0) {
> ckpt_perror("freezer path");
> - free(freezer);
> - exit(1);
> + goto out_free;
> }
> - ret = write(fd, FREEZER_THAWED, sizeof(FREEZER_THAWED));
> - if (ret != sizeof(FREEZER_THAWED)) {
> +
> + n = write(fd, FREEZER_THAWED, sizeof(FREEZER_THAWED));
> + if (n != sizeof(FREEZER_THAWED)) {
> ckpt_perror("thawing freezer");
> - free(freezer);
> - exit(1);
> + goto out_close;
> }
>
> sprintf(freezer, "%s/tasks", ctx->args->freezer);
> ctx->freezer = freezer;
> + ret = 0;
> +
> +out_close:
> close(fd);
> - return 0;
> +
> +out_free:
> + free(freezer);
> + return ret;
> }
>
> static int freezer_register(struct ckpt_ctx *ctx, pid_t pid)
> @@ -371,7 +377,6 @@ static int freezer_register(struct ckpt_ctx *ctx, pid_t pid)
> char pidstr[16];
> int fd, n, ret;
>
> -
> fd = open(ctx->freezer, O_WRONLY, 0);
> if (fd < 0) {
> ckpt_perror("freezer path");
> @@ -406,7 +411,7 @@ int process_args(struct app_restart_args *args)
> if (args->infd >= 0) {
> if (dup2(args->infd, STDIN_FILENO) < 0) {
> ckpt_perror("dup2 input file");
> - exit(1);
> + return -1;
> }
> if (args->infd != STDIN_FILENO)
> close(args->infd);
> @@ -423,7 +428,7 @@ int process_args(struct app_restart_args *args)
> if (args->pidns) {
> ckpt_err("This version of restart was compiled without "
> "support for --pidns.\n");
> - exit(1);
> + return -1;
> }
> #endif
>
> @@ -431,7 +436,7 @@ int process_args(struct app_restart_args *args)
> if (global_debug) {
> ckpt_err("This version of restart was compiled without "
> "support for --debug.\n");
> - exit(1);
> + return -1;
> }
> #endif
>
> @@ -443,7 +448,7 @@ int process_args(struct app_restart_args *args)
> if (args->pids) {
> ckpt_err("This version of restart was compiled without "
> "support for --pids.\n");
> - exit(1);
> + return -1;
> }
> #endif
> #endif
> @@ -452,7 +457,7 @@ int process_args(struct app_restart_args *args)
> (args->pids || args->pidns || args->show_status ||
> args->copy_status || args->freezer)) {
> ckpt_err("Invalid mix of --self with multiprocess options\n");
> - exit(1);
> + return -1;
> }
>
> return 0;
> @@ -472,29 +477,30 @@ int app_restart(struct app_restart_args *args)
>
> /* freezer preparation */
> if (args->freezer && freezer_prepare(&ctx) < 0)
> - exit(1);
> + return -1;
>
> + ret = -1;
> /* self-restart ends here: */
> if (args->self) {
> /* private mounts namespace ? */
> if (args->mntns && unshare(CLONE_NEWNS | CLONE_FS) < 0) {
> ckpt_perror("unshare");
> - exit(1);
> + goto cleanup_freezer;
> }
> /* chroot ? */
> if (args->root && chroot(args->root) < 0) {
> ckpt_perror("chroot");
> - exit(1);
> + goto cleanup_freezer;
> }
> /* remount /dev/pts ? */
> if (args->mnt_pty && ckpt_remount_devpts(&ctx) < 0)
> - exit(1);
> + goto cleanup_freezer;
>
> restart(getpid(), STDIN_FILENO, RESTART_TASKSELF, args->klogfd);
>
> /* reach here if restart(2) failed ! */
> ckpt_perror("restart");
> - exit(1);
> + goto cleanup_freezer;
> }
>
> setpgrp();
> @@ -502,48 +508,50 @@ int app_restart(struct app_restart_args *args)
> ret = ckpt_read_header(&ctx);
> if (ret < 0) {
> ckpt_perror("read c/r header");
> - exit(1);
> + goto cleanup_freezer;
> }
>
> ret = ckpt_read_header_arch(&ctx);
> if (ret < 0) {
> ckpt_perror("read c/r header arch");
> - exit(1);
> + goto cleanup_freezer;
> }
>
> ret = ckpt_read_container(&ctx);
> if (ret < 0) {
> ckpt_perror("read c/r container section");
> - exit(1);
> + goto cleanup_freezer;
> }
>
> ret = ckpt_read_tree(&ctx);
> if (ret < 0) {
> ckpt_perror("read c/r tree");
> - exit(1);
> + goto cleanup_freezer;
> }
>
> ret = ckpt_read_vpids(&ctx);
> if (ret < 0) {
> ckpt_perror("read c/r tree");
> - exit(1);
> + goto cleanup_freezer;
> }
>
> /* build creator-child-relationship tree */
> - if (hash_init(&ctx) < 0)
> - exit(1);
> + ret = hash_init(&ctx);
> + if (ret < 0)
> + goto cleanup_freezer;
> +
> ret = ckpt_build_tree(&ctx);
> hash_exit(&ctx);
> if (ret < 0)
> - exit(1);
> + goto cleanup_freezer;
>
> ret = assign_vpids(&ctx);
> if (ret < 0)
> - exit(1);
> + goto cleanup_freezer;
>
> ret = ckpt_fork_feeder(&ctx);
> if (ret < 0)
> - exit(1);
> + goto cleanup_freezer;
>
> /*
> * Have the first child in the restarted process tree
> @@ -587,6 +595,8 @@ int app_restart(struct app_restart_args *args)
> if (ret >= 0)
> ret = global_child_pid;
>
> +cleanup_freezer:
> + free(ctx.freezer);
> return ret;
> }
>
> @@ -648,7 +658,7 @@ static int ckpt_collect_child(struct ckpt_ctx *ctx)
> status = global_child_status;
> } else if (pid < 0) {
> ckpt_perror("WEIRD: collect child task");
> - exit(1);
> + return -1;
> }
>
> return ckpt_parse_status(status, mimic, verbose);
> @@ -742,14 +752,14 @@ static int ckpt_probe_child(pid_t pid, char *str)
> ret = waitpid(pid, &status, WNOHANG);
> if (ret == pid) {
> report_exit_status(status, str, 0);
> - exit(1);
> + return -1;
> } else if (ret < 0 && errno == ECHILD) {
> ckpt_err("WEIRD: %s exited without trace (%s)\n",
> str, strerror(errno));
> - exit(1);
> + return -1;
> } else if (ret != 0) {
> ckpt_err("waitpid for %s (%s)", str, strerror(errno));
> - exit(1);
> + return -1;
> }
> return 0;
> }
> @@ -841,10 +851,11 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
> return -1;
> }
>
> + ret = -1;
> stk = genstack_alloc(PTHREAD_STACK_MIN);
> if (!stk) {
> ckpt_perror("coordinator genstack_alloc");
> - return -1;
> + goto out_close;
> }
> sp = genstack_sp(stk);
>
> @@ -858,7 +869,7 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
> genstack_release(stk);
> if (coord_pid < 0) {
> ckpt_perror("clone coordinator");
> - return coord_pid;
> + goto out_close;
> }
> global_child_pid = coord_pid;
>
> @@ -872,7 +883,7 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
> * signal handler was plugged; verify that it's still there.
> */
> if (ckpt_probe_child(coord_pid, "coordinator") < 0)
> - return -1;
> + goto out_close;
>
> ctx->args->copy_status = copy;
>
> @@ -881,13 +892,18 @@ static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
> if (ret == 0 && ctx->args->wait)
> ret = ckpt_collect_child(ctx);
>
> +out_close:
> + if (ret < 0) {
> + close(ctx->pipe_coord[0]);
> + close(ctx->pipe_coord[1]);
> + }
> return ret;
> }
> #else /* CLONE_NEWPID */
> static int ckpt_coordinator_pidns(struct ckpt_ctx *ctx)
> {
> ckpt_err("logical error: ckpt_coordinator_pidns unexpected\n");
> - exit(1);
> + return -1;
> }
> #endif /* CLONE_NEWPID */
>
> @@ -899,7 +915,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
>
> root_pid = ckpt_fork_child(ctx, &ctx->tasks_arr[0]);
> if (root_pid < 0)
> - exit(1);
> + return -1;
> global_child_pid = root_pid;
>
> /* catch SIGCHLD to detect errors during hierarchy creation */
> @@ -912,7 +928,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
> * signal handler was plugged; verify that it's still there.
> */
> if (ckpt_probe_child(root_pid, "root task") < 0)
> - exit(1);
> + return -1;
>
> if (ctx->args->keep_frozen)
> flags |= RESTART_FROZEN;
> @@ -925,7 +941,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
> ckpt_perror("restart failed");
> ckpt_verbose("Failed\n");
> ckpt_dbg("restart failed ?\n");
> - exit(1);
> + return -1;
> }
>
> ckpt_verbose("Success\n");
> @@ -937,7 +953,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
> /* Report success/failure to the parent */
> if (write(ctx->pipe_coord[1], &ret, sizeof(ret)) < 0) {
> ckpt_perror("failed to report status");
> - exit(1);
> + return -1;
> }
>
> /*
> @@ -1835,12 +1851,12 @@ static int ckpt_fork_feeder(struct ckpt_ctx *ctx)
>
> if (pipe(ctx->pipe_feed)) {
> ckpt_perror("pipe");
> - exit(1);
> + return -1;
> }
>
> if (pipe(ctx->pipe_child) < 0) {
> ckpt_perror("pipe");
> - exit(1);
> + return -1;
> }
>
> /*
>
>
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
