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 <[email protected]> > 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 <[email protected]> > --- > 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 [email protected] https://lists.linux-foundation.org/mailman/listinfo/containers _______________________________________________ Devel mailing list [email protected] https://openvz.org/mailman/listinfo/devel
