[PATCH 1/6] add image offset to ckpt_err output

[PATCH 1/6] add image offset to ckpt_err output

[serue-r/Jw6+rmf7HQT0dZR+AlfA]

From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Changelog:
	Nov 17: check for len overrun

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/sys.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index c1c4e99..d64f5a7 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -471,6 +471,8 @@ static void _ckpt_msg_appendv(struct ckpt_ctx *ctx, int err, char *fmt,
 			goto full;
 	}
 
+	len += snprintf(&ctx->msg[len], CKPT_MSG_LEN-len, "[offset %lld]",
+			ctx->total);
 	len += vsnprintf(&ctx->msg[len], CKPT_MSG_LEN-len, fmt, ap);
 	if (len > CKPT_MSG_LEN) {
 full:
-- 
1.6.1

[PATCH 2/6] use ckpt_err in ckpt_obj_fetch()

[serue-r/Jw6+rmf7HQT0dZR+AlfA]

From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Changelog:
	Nov 17: print requested obj type

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/objhash.c |   11 +++++++++--
 1 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 9b1b2f4..2e50025 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -1004,8 +1004,15 @@ void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref, enum obj_type type)
 	struct ckpt_obj *obj;
 
 	obj = obj_find_by_objref(ctx, objref);
-	if (!obj)
+	if (!obj) {
+		ckpt_err(ctx, -EINVAL, "%(O)No such object (type %d)\n",
+			 objref, type);
 		return ERR_PTR(-EINVAL);
+	}
 	ckpt_debug("%s ref %d\n", obj->ops->obj_name, obj->objref);
-	return (obj->ops->obj_type == type ? obj->ptr : ERR_PTR(-ENOMSG));
+	if (obj->ops->obj_type == type)
+		return obj->ptr;
+	ckpt_err(ctx, -ENOMSG, "%(O)Hashed objed was type %d, not %d\n",
+		 objref, obj->ops->obj_type, type);
+	return ERR_PTR(-ENOMSG);
 }
-- 
1.6.1

[PATCH 3/6] use ckpt_err in ckpt_read_obj_type()

[serue-r/Jw6+rmf7HQT0dZR+AlfA]

From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/restart.c |    7 ++++++-
 1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 4af4647..b684d7a 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -416,11 +416,16 @@ void *ckpt_read_obj_type(struct ckpt_ctx *ctx, int len, int type)
 	BUG_ON(!len);
 
 	h = ckpt_read_obj(ctx, len, len);
-	if (IS_ERR(h))
+	if (IS_ERR(h)) {
+		ckpt_err(ctx, PTR_ERR(h), "Looking for type %d in ckptfile\n",
+			 type);
 		return h;
+	}
 
 	if (h->type != type) {
 		ckpt_hdr_put(ctx, h);
+		ckpt_err(ctx, -EINVAL, "Next object was type %d, not %d\n",
+			h->type, type);
 		h = ERR_PTR(-EINVAL);
 	}
 
-- 
1.6.1

[PATCH 4/6] Move init_completion(&ctx->complete) to ctx_alloc

[serue-r/Jw6+rmf7HQT0dZR+AlfA]

From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

That way we don't risk indirectly calling complete(&ctx->complete)
(through ckpt_err()) before ctx->complete has been initialized.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/restart.c |    2 --
 checkpoint/sys.c     |    4 ++++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index b684d7a..6cdefc9 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -1092,8 +1092,6 @@ static int wait_all_tasks_finish(struct ckpt_ctx *ctx)
 {
 	int ret;
 
-	init_completion(&ctx->complete);
-
 	BUG_ON(ctx->active_pid != -1);
 	ret = restore_activate_next(ctx);
 	if (ret < 0)
diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index d64f5a7..bf66418 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -245,6 +245,8 @@ static struct ckpt_ctx *ckpt_ctx_alloc(int fd, unsigned long uflags,
 	INIT_LIST_HEAD(&ctx->pgarr_pool);
 	init_waitqueue_head(&ctx->waitq);
 	init_waitqueue_head(&ctx->ghostq);
+	if (kflags == CKPT_CTX_RESTART)
+		init_completion(&ctx->complete);
 
 #ifdef CONFIG_CHECKPOINT_DEBUG
 	INIT_LIST_HEAD(&ctx->task_status);
@@ -473,6 +475,8 @@ static void _ckpt_msg_appendv(struct ckpt_ctx *ctx, int err, char *fmt,
 
 	len += snprintf(&ctx->msg[len], CKPT_MSG_LEN-len, "[offset %lld]",
 			ctx->total);
+	if (len > CKPT_MSG_LEN)
+		goto full;
 	len += vsnprintf(&ctx->msg[len], CKPT_MSG_LEN-len, fmt, ap);
 	if (len > CKPT_MSG_LEN) {
 full:
-- 
1.6.1

[PATCH 5/6] define ckpt_obj_try_fetch

[serue-r/Jw6+rmf7HQT0dZR+AlfA]

From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Which doesn't ckpt_err() on failure.  Also have callers which want
to, probe with _lite(), not ckpt_obj_fetch().

This is needed before the next patch, which will make ckpt_err()
with non-zero err bail a restart.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/objhash.c       |   19 ++++++++++++-------
 checkpoint/signal.c        |   12 +++++++-----
 fs/pipe.c                  |    4 ++--
 include/linux/checkpoint.h |    2 ++
 mm/shmem.c                 |    2 +-
 5 files changed, 24 insertions(+), 15 deletions(-)

diff --git a/checkpoint/objhash.c b/checkpoint/objhash.c
index 2e50025..6b34426 100644
--- a/checkpoint/objhash.c
+++ b/checkpoint/objhash.c
@@ -999,20 +999,25 @@ int ckpt_obj_insert(struct ckpt_ctx *ctx, void *ptr,
  *
  * [This is used during restart].
  */
-void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref, enum obj_type type)
+void *ckpt_obj_try_fetch(struct ckpt_ctx *ctx, int objref, enum obj_type type)
 {
 	struct ckpt_obj *obj;
 
 	obj = obj_find_by_objref(ctx, objref);
-	if (!obj) {
-		ckpt_err(ctx, -EINVAL, "%(O)No such object (type %d)\n",
-			 objref, type);
+	if (!obj)
 		return ERR_PTR(-EINVAL);
-	}
 	ckpt_debug("%s ref %d\n", obj->ops->obj_name, obj->objref);
 	if (obj->ops->obj_type == type)
 		return obj->ptr;
-	ckpt_err(ctx, -ENOMSG, "%(O)Hashed objed was type %d, not %d\n",
-		 objref, obj->ops->obj_type, type);
 	return ERR_PTR(-ENOMSG);
 }
+
+void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref, enum obj_type type)
+{
+	void *ret = ckpt_obj_try_fetch(ctx, objref, type);
+
+	if (unlikely(IS_ERR(ret)))
+		ckpt_err(ctx, PTR_ERR(ret), "%(O)Fetching object (type %d)\n",
+			 objref, type);
+	return ret;
+}
diff --git a/checkpoint/signal.c b/checkpoint/signal.c
index 989b974..d8cb5d9 100644
--- a/checkpoint/signal.c
+++ b/checkpoint/signal.c
@@ -621,7 +621,7 @@ int restore_obj_signal(struct ckpt_ctx *ctx, int signal_objref)
 	struct signal_struct *signal;
 	int ret = 0;
 
-	signal = ckpt_obj_fetch(ctx, signal_objref, CKPT_OBJ_SIGNAL);
+	signal = ckpt_obj_try_fetch(ctx, signal_objref, CKPT_OBJ_SIGNAL);
 	if (!IS_ERR(signal)) {
 		/*
 		 * signal_struct is already shared properly as it is
@@ -629,15 +629,17 @@ int restore_obj_signal(struct ckpt_ctx *ctx, int signal_objref)
 		 * are already restore now, t->signal must match.
 		 */
 		if (signal != current->signal)
-			ret = -EINVAL;
-	} else if (PTR_ERR(signal) == -EINVAL) {
+			return -EINVAL;
+		return ret;
+	}
+
+	ret = PTR_ERR(signal);
+	if (ret == -EINVAL) {
 		/* first timer: add to hash and restore our t->signal */
 		ret = ckpt_obj_insert(ctx, current->signal,
 				      signal_objref, CKPT_OBJ_SIGNAL);
 		if (ret >= 0)
 			ret = restore_signal(ctx);
-	} else {
-		ret = PTR_ERR(signal);
 	}
 
 	return ret;
diff --git a/fs/pipe.c b/fs/pipe.c
index 65ad44e..7f350fc 100644
--- a/fs/pipe.c
+++ b/fs/pipe.c
@@ -919,7 +919,7 @@ struct file *pipe_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 	if (h->pipe_objref <= 0)
 		return ERR_PTR(-EINVAL);
 
-	file = ckpt_obj_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
+	file = ckpt_obj_try_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
 	/*
 	 * If ckpt_obj_fetch() returned ERR_PTR(-EINVAL), then this is
 	 * the first time we see this pipe so need to restore the
@@ -990,7 +990,7 @@ struct file *fifo_file_restore(struct ckpt_ctx *ctx, struct ckpt_hdr_file *ptr)
 	 * If ckpt_obj_fetch() returned ERR_PTR(-EINVAL), this is the
 	 * first time for this fifo.
 	 */
-	file = ckpt_obj_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
+	file = ckpt_obj_try_fetch(ctx, h->pipe_objref, CKPT_OBJ_FILE);
 	if (!IS_ERR(file))
 		first = 0;
 	else if (PTR_ERR(file) == -EINVAL)
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 8fd6cba..65765af 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -132,6 +132,8 @@ extern int ckpt_obj_lookup(struct ckpt_ctx *ctx, void *ptr,
 			   enum obj_type type);
 extern int ckpt_obj_lookup_add(struct ckpt_ctx *ctx, void *ptr,
 			       enum obj_type type, int *first);
+extern void *ckpt_obj_try_fetch(struct ckpt_ctx *ctx, int objref,
+			    enum obj_type type);
 extern void *ckpt_obj_fetch(struct ckpt_ctx *ctx, int objref,
 			    enum obj_type type);
 extern int ckpt_obj_insert(struct ckpt_ctx *ctx, void *ptr, int objref,
diff --git a/mm/shmem.c b/mm/shmem.c
index 2cfff8d..a0416d3 100644
--- a/mm/shmem.c
+++ b/mm/shmem.c
@@ -2419,7 +2419,7 @@ int shmem_restore(struct ckpt_ctx *ctx,
 	struct file *file;
 	int ret = 0;
 
-	file = ckpt_obj_fetch(ctx, h->ino_objref, CKPT_OBJ_FILE);
+	file = ckpt_obj_try_fetch(ctx, h->ino_objref, CKPT_OBJ_FILE);
 	if (PTR_ERR(file) == -EINVAL)
 		file = NULL;
 	if (IS_ERR(file))
-- 
1.6.1

[PATCH 6/6] have ckpt_err set ctx->errno

[serue-r/Jw6+rmf7HQT0dZR+AlfA]

From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>

Move setting of ctx->errno into do_ckpt_msg().  If the operation is
a restart, and errno was previously unset, then also wake all waiting
callers so they know to quit.

NOTE that the way this is set up, users of _ckpt_err().._ckpt_msg_complete()
do not cause restore_wake_all_on_error() to be called.  If a restart
path has to use _ckpt_err(), then we'll have to work around that, which
seems simplest to do by again using the CKPT_CTX_WOKEN flag separate from
CKPT_CTX_ERROR.

Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
---
 checkpoint/restart.c       |   32 ++++----------------------------
 checkpoint/sys.c           |   25 +++++++++++++++++--------
 include/linux/checkpoint.h |    6 ------
 3 files changed, 21 insertions(+), 42 deletions(-)

diff --git a/checkpoint/restart.c b/checkpoint/restart.c
index 6cdefc9..a45263e 100644
--- a/checkpoint/restart.c
+++ b/checkpoint/restart.c
@@ -719,29 +719,6 @@ static inline int is_task_active(struct ckpt_ctx *ctx, pid_t pid)
 	return get_active_pid(ctx) == pid;
 }
 
-/* should not be called under write_lock_irq(&tasklist_lock) */
-static void _restore_notify_error(struct ckpt_ctx *ctx, int errno)
-{
-	/* first to fail: notify everyone (racy but harmless) */
-	if (!ckpt_test_ctx_error(ctx)) {
-		ckpt_debug("setting restart error %d\n", errno); \
-		ckpt_set_ctx_error(ctx, errno);
-		complete(&ctx->complete);
-		wake_up_all(&ctx->waitq);
-		wake_up_all(&ctx->ghostq);
-	}
-}
-
-/*
- * Need to call ckpt_debug such that it will get the correct source
- * location.  Should not be called under write_lock_irq(&tasklist_lock)
-*/
-#define restore_notify_error(ctx, errno) \
-do { \
-	ckpt_debug("restart error %d, root pid %d\n", errno, ctx->root_pid); \
-	_restore_notify_error(ctx, errno); \
-} while(0)
-
 static inline struct ckpt_ctx *get_task_ctx(struct task_struct *task)
 {
 	struct ckpt_ctx *ctx;
@@ -812,8 +789,7 @@ static int restore_activate_next(struct ckpt_ctx *ctx)
 		rcu_read_unlock();
 
 		if (!task) {
-			ckpt_debug("could not find task %d\n", pid);
-			restore_notify_error(ctx, -ESRCH);
+			ckpt_err(ctx, -ESRCH, "could not find task %d\n", pid);
 			return -ESRCH;
 		}
 	} else {
@@ -898,7 +874,7 @@ static int do_ghost_task(void)
  out:
 	restore_debug_error(ctx, ret);
 	if (ret < 0)
-		restore_notify_error(ctx, ret);
+		ckpt_err(ctx, ret, "Error while restarting ghost\n");
 
 	current->exit_signal = -1;
 	restore_debug_exit(ctx);
@@ -1009,7 +985,7 @@ static int do_restore_task(void)
  out:
 	restore_debug_error(ctx, ret);
 	if (ret < 0)
-		restore_notify_error(ctx, ret);
+		ckpt_err(ctx, ret, "Error while restarting task\n");
 
 	post_restore_task();
 	current->flags &= ~PF_RESTARTING;
@@ -1272,7 +1248,7 @@ static int do_restore_coord(struct ckpt_ctx *ctx, pid_t pid)
 	restore_debug_error(ctx, ret);
 
 	if (ret < 0)
-		ckpt_set_ctx_error(ctx, ret);
+		ckpt_err(ctx, ret, "Error while restarting coordinator\n");
 
 	if (ckpt_test_ctx_error(ctx)) {
 		destroy_descendants(ctx);
diff --git a/checkpoint/sys.c b/checkpoint/sys.c
index bf66418..f22bdb7 100644
--- a/checkpoint/sys.c
+++ b/checkpoint/sys.c
@@ -376,6 +376,19 @@ static inline int is_special_flag(char *s)
 }
 
 /*
+ * If exiting a restart with error, then wake up all other tasks
+ * in the restart context.
+ */
+static void restore_wake_all_on_error(struct ckpt_ctx *ctx)
+{
+	if (!ctx->kflags & CKPT_CTX_RESTART)
+		return;
+	complete(&ctx->complete);
+	wake_up_all(&ctx->waitq);
+	wake_up_all(&ctx->ghostq);
+}
+
+/*
  * _ckpt_generate_fmt - handle the special flags in the enhanced format
  * strings used by checkpoint/restart error messages.
  * @ctx: checkpoint context
@@ -459,14 +472,6 @@ static void _ckpt_msg_appendv(struct ckpt_ctx *ctx, int err, char *fmt,
 	int len = ctx->msglen;
 
 	if (err) {
-		/* At restart we must use a more baroque helper to set
-		 * ctx->errno, which also wakes all other waiting restarting
-		 * tasks.  But at checkpoint we just set ctx->errno so that
-		 * _ckpt_msg_complete() will know to write the error message
-		 * to the checkpoint image.
-		 */
-		if (ctx->kflags & CKPT_CTX_CHECKPOINT && !ctx->errno)
-			ctx->errno = err;
 		len += snprintf(&ctx->msg[len], CKPT_MSG_LEN-len, "[err %d]",
 				 err);
 		if (len > CKPT_MSG_LEN)
@@ -543,6 +548,10 @@ void do_ckpt_msg(struct ckpt_ctx *ctx, int err, char *fmt, ...)
 {
 	if (!ctx) return;
 
+	if (err && !ckpt_test_and_set_ctx_kflag(ctx, CKPT_CTX_ERROR)) {
+		ctx->errno = err;
+		restore_wake_all_on_error(ctx);
+	}
 	ckpt_msg_lock(ctx);
 	__do_ckpt_msg(ctx, err, fmt);
 	_ckpt_msg_complete(ctx);
diff --git a/include/linux/checkpoint.h b/include/linux/checkpoint.h
index 65765af..470097d 100644
--- a/include/linux/checkpoint.h
+++ b/include/linux/checkpoint.h
@@ -104,12 +104,6 @@ extern int ckpt_sock_getnames(struct ckpt_ctx *ctx,
 
 #define ckpt_set_ctx_success(ctx)  ckpt_set_ctx_kflag(ctx, CKPT_CTX_SUCCESS)
 
-static inline void ckpt_set_ctx_error(struct ckpt_ctx *ctx, int errno)
-{
-	if (!ckpt_test_and_set_ctx_kflag(ctx, CKPT_CTX_ERROR))
-		ctx->errno = errno;
-}
-
 #define ckpt_test_ctx_error(ctx)  \
 	((ctx)->kflags & CKPT_CTX_ERROR)
 #define ckpt_test_ctx_complete(ctx)  \
-- 
1.6.1

Re: [PATCH 1/6] add image offset to ckpt_err output

[Oren Laadan]

Cool. Entire patchset queued for v19-rc2.

Thanks,

Oren.


serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org wrote:
> From: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> 
> Changelog:
> 	Nov 17: check for len overrun
> 
> Signed-off-by: Serge E. Hallyn <serue-r/Jw6+rmf7HQT0dZR+AlfA@public.gmane.org>
> ---
>  checkpoint/sys.c |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/checkpoint/sys.c b/checkpoint/sys.c
> index c1c4e99..d64f5a7 100644
> --- a/checkpoint/sys.c
> +++ b/checkpoint/sys.c
> @@ -471,6 +471,8 @@ static void _ckpt_msg_appendv(struct ckpt_ctx *ctx, int err, char *fmt,
>  			goto full;
>  	}
>  
> +	len += snprintf(&ctx->msg[len], CKPT_MSG_LEN-len, "[offset %lld]",
> +			ctx->total);
>  	len += vsnprintf(&ctx->msg[len], CKPT_MSG_LEN-len, fmt, ap);
>  	if (len > CKPT_MSG_LEN) {
>  full: