Hi Suka,
Indeed the use of STDIN_FILENO wasn't too friendly to callers
of the library. However, I think the correct fix is different.
So I fixed that, and added a few other cleanups in an attempt
to ensure that the restart code cleans up properly after being
called (succesfully or unsuccesfully) - including memory leaks,
file decsriptors, and even drop a call to setpgrp(). I pushed
those patches to user-cr.
Thanks,
Oren.
On 12/14/2010 12:50 AM, Sukadev Bhattiprolu wrote:
> Oren
>
> Is there a reason we hard code STDIN_FILENO during restart in usercr ?
>
> Or can we simply use the fd passed by the caller of cr_restart() -
> something like in this patch ?
>
> Whether it is STDIN_FILENO or another fd, do_sys_restart() holds a reference
> to the 'struct file' and so we should still be able to read the image even
> if close_all_fds() closes the fd. Or is there a subtle reason, we stick to
> STDIN_FILENO ?
>
> Sukadev
> ---
> src/lxc/user-cr/restart.c | 35 ++++++++++++++++++++++-------------
> 1 files changed, 22 insertions(+), 13 deletions(-)
>
> diff --git a/src/lxc/user-cr/restart.c b/src/lxc/user-cr/restart.c
> index 5b22948..944dbf1 100644
> --- a/src/lxc/user-cr/restart.c
> +++ b/src/lxc/user-cr/restart.c
> @@ -169,6 +169,7 @@ static int global_child_status;
> static int global_child_collected;
> static int global_sent_sigint;
> static struct signal_array signal_array[] = INIT_SIGNAL_ARRAY;
> +static int global_input_fd = -1;
>
> /*
> * TODO: Implement an API to let callers choose if/how an interrupt be sent
> @@ -403,15 +404,23 @@ int process_args(struct cr_restart_args *args)
> global_ulogfd = args->ulogfd;
> global_uerrfd = args->uerrfd;
>
> + if (args->infd < 0) {
> + ckpt_err("Invalid input fd %d\n", args->infd);
> + exit(1);
> + }
> + global_input_fd = args->infd;
> +
> +#if 0
> /* input file descriptor (default: stdin) */
> if (args->infd >= 0) {
> - if (dup2(args->infd, STDIN_FILENO) < 0) {
> + if (dup2(args->infd, global_input_fd) < 0) {
> ckpt_perror("dup2 input file");
> exit(1);
> }
> - if (args->infd != STDIN_FILENO)
> + if (args->infd != global_input_fd)
> close(args->infd);
> }
> +#endif
>
> /* output file descriptor (default: none) */
> if (args->klogfd < 0)
> @@ -492,7 +501,7 @@ int cr_restart(struct cr_restart_args *args)
> if (args->mnt_pty && ckpt_remount_devpts(&ctx) < 0)
> exit(1);
>
> - restart(getpid(), STDIN_FILENO, RESTART_TASKSELF, args->klogfd);
> + restart(getpid(), global_input_fd, RESTART_TASKSELF, args->klogfd);
>
> /* reach here if restart(2) failed ! */
> ckpt_perror("restart");
> @@ -926,7 +935,7 @@ static int ckpt_coordinator(struct ckpt_ctx *ctx)
> if (ctx->args->keep_lsm)
> flags |= RESTART_KEEP_LSM;
>
> - ret = restart(root_pid, STDIN_FILENO, flags, ctx->args->klogfd);
> + ret = restart(root_pid, global_input_fd, flags, ctx->args->klogfd);
>
> if (ret < 0) {
> ckpt_perror("restart failed");
> @@ -1658,7 +1667,7 @@ static int ckpt_make_tree(struct ckpt_ctx *ctx, struct task *task)
>
> /* on success this doesn't return */
> ckpt_dbg("about to call sys_restart(), flags %#lx\n", flags);
> - ret = restart(0, STDIN_FILENO, flags, CHECKPOINT_FD_NONE);
> + ret = restart(0, global_input_fd, flags, CHECKPOINT_FD_NONE);
> if (ret < 0)
> ckpt_perror("task restore failed");
> return ret;
> @@ -1857,8 +1866,8 @@ static int ckpt_fork_feeder(struct ckpt_ctx *ctx)
> ctx->pipe_out = ctx->pipe_child[1];
> /* feeder pipe */
> close(ctx->pipe_feed[1]);
> - if (ctx->pipe_feed[0] != STDIN_FILENO) {
> - dup2(ctx->pipe_feed[0], STDIN_FILENO);
> + if (ctx->pipe_feed[0] != global_input_fd) {
> + dup2(ctx->pipe_feed[0], global_input_fd);
> close(ctx->pipe_feed[0]);
> }
>
> @@ -1878,7 +1887,7 @@ static void ckpt_read_write_blind(struct ckpt_ctx *ctx)
> int ret;
>
> while (1) {
> - ret = read(STDIN_FILENO, ctx->buf, BUFSIZE);
> + ret = read(global_input_fd, ctx->buf, BUFSIZE);
> ckpt_dbg("c/r read input %d\n", ret);
> if (ret == 0)
> break;
> @@ -1897,7 +1906,7 @@ static void ckpt_read_write_inspect(struct ckpt_ctx *ctx)
> int len, ret;
>
> while (1) {
> - ret = _ckpt_read(STDIN_FILENO, &h, sizeof(h));
> + ret = _ckpt_read(global_input_fd, &h, sizeof(h));
> ckpt_dbg("ret %d len %d type %d\n", ret, h.len, h.type);
> if (ret == 0)
> break;
> @@ -1915,7 +1924,7 @@ ckpt_dbg("ret %d len %d type %d\n", ret, h.len, h.type);
> h.len -= sizeof(h);
> if (h.type == CKPT_HDR_ERROR) {
> len = (h.len > BUFSIZE ? BUFSIZE : h.len);
> - ret = read(STDIN_FILENO, ctx->buf, len);
> + ret = read(global_input_fd, ctx->buf, len);
> if (ret < 0)
> ckpt_abort(ctx, "error record");
> errno = EIO;
> @@ -1926,7 +1935,7 @@ ckpt_dbg("ret %d len %d type %d\n", ret, h.len, h.type);
>
> while (h.len) {
> len = (h.len > BUFSIZE ? BUFSIZE : h.len);
> - ret = read(STDIN_FILENO, ctx->buf, len);
> + ret = read(global_input_fd, ctx->buf, len);
> if (ret == 0)
> ckpt_abort(ctx, "short record");
> if (ret < 0)
> @@ -2163,7 +2172,7 @@ static int ckpt_read_obj(struct ckpt_ctx *ctx,
> {
> int ret;
>
> - ret = ckpt_read(STDIN_FILENO, h, sizeof(*h));
> + ret = ckpt_read(global_input_fd, h, sizeof(*h));
> if (ret < 0)
> return ret;
> if (h->len < sizeof(*h) || h->len > n) {
> @@ -2172,7 +2181,7 @@ static int ckpt_read_obj(struct ckpt_ctx *ctx,
> }
> if (h->len == sizeof(*h))
> return 0;
> - return ckpt_read(STDIN_FILENO, buf, h->len - sizeof(*h));
> + return ckpt_read(global_input_fd, buf, h->len - sizeof(*h));
> }
>
> static int ckpt_read_obj_type(struct ckpt_ctx *ctx, void *buf, int n, int type)
_______________________________________________
Containers mailing list
Containers@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/containers
