Skip to contentSkip to conversation

cr: add a few more ckpt_write_err()s

PATCH In review Serge E. Hallyn 2 participants 5 messages Sep 10, 2009 – Sep 14, 2009 4 messages not in the archive Kbuild

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 cr: add a few more ckpt_write_err()s

The main point is for new-comers to the checkpoint/restart tree to be able to help us debug their otherwise mysterious checkpoint failures.

---
 checkpoint/checkpoint.c |   28 +++++++++++++++++++++++-----
 checkpoint/files.c      |   37 ++++++++++++++++++++++++++++++-------
 checkpoint/memory.c     |   17 ++++++++++++++---
 3 files changed, 67 insertions(+), 15 deletions(-)
checkpoint/checkpoint.c +23 -5
index c19f812..7beee08 100644
@@ -295,6 +295,8 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 
 	if (t->exit_state == EXIT_DEAD) {
 		pr_warning("c/r: task %d is EXIT_DEAD\n", task_pid_vnr(t));
+		__ckpt_write_err(ctx, "task %d (%s) in state EXIT_DEAD",
+				 task_pid_vnr(t), t->comm);
 		return -EAGAIN;
 	}
 
@@ -335,12 +337,21 @@ static int may_checkpoint_task(struct ckpt_ctx *ctx, struct task_struct *t)
 
 	rcu_read_lock();
 	nsproxy = task_nsproxy(t);
-	if (nsproxy->mnt_ns != ctx->root_nsproxy->mnt_ns)
+	if (nsproxy->mnt_ns != ctx->root_nsproxy->mnt_ns) {
+		__ckpt_write_err(ctx, "task %d (%s) in bad mnt_ns",
+				 task_pid_vnr(t), t->comm);
 		ret = -EPERM;
-	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns)
+	}
+	if (nsproxy->pid_ns != ctx->root_nsproxy->pid_ns) {
+		__ckpt_write_err(ctx, "task %d (%s) in bad pid_ns",
+				 task_pid_vnr(t), t->comm);
 		ret = -EPERM;
-	if (nsproxy->net_ns != ctx->root_nsproxy->net_ns)
+	}
+	if (nsproxy->net_ns != ctx->root_nsproxy->net_ns) {
+		__ckpt_write_err(ctx, "task %d (%s) in bad net_ns",
+				 task_pid_vnr(t), t->comm);
 		ret = -EPERM;
+	}
 	rcu_read_unlock();
 
 	return ret;
@@ -629,8 +640,11 @@ static int init_checkpoint_ctx(struct ckpt_ctx *ctx, pid_t pid)
 	/* container init ? */
 	ctx->root_init = is_container_init(task);
 
-	if (!(ctx->uflags & CHECKPOINT_SUBTREE) && !ctx->root_init)
+	if (!(ctx->uflags & CHECKPOINT_SUBTREE) && !ctx->root_init) {
+		ckpt_write_err(ctx, "Task is not container init"
+			" and CHECKPOINT_SUBTREE was not specified");
 		return -EINVAL;  /* cleanup by ckpt_ctx_free() */
+	}
 
 	/* root vfs (FIX: WILL CHANGE with mnt-ns etc */
 	task_lock(ctx->root_task);
@@ -669,9 +683,12 @@ long do_checkpoint(struct ckpt_ctx *ctx, pid_t pid)
 		 * and then compare to the objects' real user counts.
 		 */
 		ret = collect_objects(ctx);
-		if (ret < 0)
+		if (ret < 0) {
+			ckpt_write_err(ctx, "Error %d collecting objects", ret);
 			goto out;
+		}
 		if (!ckpt_obj_contained(ctx)) {
+			ckpt_write_err(ctx, "Container had leaks");
 			ret = -EAGAIN;
 			goto out;
 		}
@@ -689,6 +706,7 @@ long do_checkpoint(struct ckpt_ctx *ctx, pid_t pid)
 
 	/* verify that all objects were indeed visited */
 	if (!ckpt_obj_visited(ctx)) {
+		ckpt_write_err(ctx, "Not all objects were visited");
 		ret = -EAGAIN;
 		goto out;
 	}
checkpoint/files.c +30 -7
index 204055b..d16916a 100644
@@ -84,8 +84,11 @@ int checkpoint_fname(struct ckpt_ctx *ctx, struct path *path, struct path *root)
 	if (!IS_ERR(fname))
 		ret = ckpt_write_obj_type(ctx, fname, flen,
 					  CKPT_HDR_FILE_NAME);
-	else
+	else {
+		ckpt_write_err(ctx, "Error writing file name for %s\n",
+			path->dentry->d_name.name);
 		ret = PTR_ERR(fname);
+	}
 
 	kfree(buf);
 	return ret;
@@ -192,16 +195,23 @@ EXPORT_SYMBOL(generic_file_checkpoint);
 int checkpoint_file(struct ckpt_ctx *ctx, void *ptr)
 {
 	struct file *file = (struct file *) ptr;
+	int ret;
 
 	if (!file->f_op || !file->f_op->checkpoint) {
+		ckpt_write_err(ctx, "Unsupproted file type: %pS", file->f_op);
 		ckpt_debug("f_op lacks checkpoint handler: %pS\n", file->f_op);
 		return -EBADF;
 	}
 	if (d_unlinked(file->f_dentry)) {
+		ckpt_write_err(ctx, "Unlinked file: %s", file->f_dentry->d_name.name);
 		ckpt_debug("unlinked files are unsupported\n");
 		return -EBADF;
 	}
-	return file->f_op->checkpoint(ctx, file);
+	ret = file->f_op->checkpoint(ctx, file);
+	if (ret)
+		ckpt_write_err(ctx, "f_op->checkpoint returned %d for %s",
+			ret, file->f_dentry->d_name.name);
+	return ret;
 }
 
 /**
@@ -238,8 +248,10 @@ static int checkpoint_file_desc(struct ckpt_ctx *ctx,
 
 	/* sanity check (although this shouldn't happen) */
 	ret = -EBADF;
-	if (!file)
+	if (!file) {
+		ckpt_write_err(ctx, "File no longer exists?");
 		goto out;
+	}
 
 	/*
 	 * if seen first time, this will add 'file' to the objhash, keep
@@ -312,8 +324,10 @@ int checkpoint_obj_file_table(struct ckpt_ctx *ctx, struct task_struct *t)
 	int objref;
 
 	files = get_files_struct(t);
-	if (!files)
+	if (!files) {
+		ckpt_write_err(ctx, "couldn't get files struct");
 		return -EBUSY;
+	}
 	objref = checkpoint_obj(ctx, files, CKPT_OBJ_FILE_TABLE);
 	put_files_struct(files);
 
@@ -332,8 +346,12 @@ int ckpt_collect_file(struct ckpt_ctx *ctx, struct file *file)
 	if (ret <= 0)
 		return ret;
 	/* if first time for this file (ret > 0), invoke ->collect() */
-	if (file->f_op->collect)
+	if (file->f_op->collect) {
 		ret = file->f_op->collect(ctx, file);
+		if (ret)
+			ckpt_write_err(ctx, "Error %d collecing file %s",
+				ret, file->f_dentry->d_name.name);
+	}
 	return ret;
 }
 
@@ -351,8 +369,10 @@ static int collect_file_desc(struct ckpt_ctx *ctx,
 		get_file(file);
 	rcu_read_unlock();
 
-	if (!file)
+	if (!file) {
+		ckpt_write_err(ctx, "Failed to get file");
 		return -EAGAIN;
+	}
 
 	ret = ckpt_collect_file(ctx, file);
 	fput(file);
@@ -392,8 +412,11 @@ int ckpt_collect_file_table(struct ckpt_ctx *ctx, struct task_struct *t)
 	int ret;
 
 	files = get_files_struct(t);
-	if (!files)
+	if (!files) {
+		ckpt_write_err(ctx, "Could not get files struct task %d (%s)",
+				 task_pid_vnr(t), t->comm);
 		return -EBUSY;
+	}
 	ret = collect_file_table(ctx, files);
 	put_files_struct(files);
 
checkpoint/memory.c +14 -3
index 487efbd..a6bbd1f 100644
@@ -556,6 +556,7 @@ int generic_vma_checkpoint(struct ckpt_ctx *ctx, struct vm_area_struct *vma,
 
 	if (vma->vm_flags & CKPT_VMA_NOT_SUPPORTED) {
 		pr_warning("c/r: unsupported VMA %#lx\n", vma->vm_flags);
+		ckpt_write_err(ctx, "unsupported VMA %#lx", vma->vm_flags);
 		return -ENOSYS;
 	}
 
@@ -645,6 +646,7 @@ static int anonymous_checkpoint(struct ckpt_ctx *ctx,
 	/* should be private anonymous ... verify that this is the case */
 	if (vma->vm_flags & CKPT_VMA_NOT_SUPPORTED) {
 		pr_warning("c/r: unsupported VMA %#lx\n", vma->vm_flags);
+		ckpt_write_err(ctx, "unsupported VMA %#lx", vma->vm_flags);
 		return -ENOSYS;
 	}
 
@@ -711,8 +713,11 @@ static int do_checkpoint_mm(struct ckpt_ctx *ctx, struct mm_struct *mm)
 			ret = (*vma->vm_ops->checkpoint)(ctx, vma);
 		else
 			ret = -ENOSYS;
-		if (ret < 0)
+		if (ret < 0) {
+			ckpt_write_err(ctx, "vma checkpoint returrned %d",
+				ret);
 			goto out;
+		}
 	}
 
 	ret = checkpoint_mm_context(ctx, mm);
@@ -758,15 +763,21 @@ static int collect_mm(struct ckpt_ctx *ctx, struct mm_struct *mm)
 	down_read(&mm->mmap_sem);
 	if (mm->exe_file) {
 		ret = ckpt_collect_file(ctx, mm->exe_file);
-		if (ret < 0)
+		if (ret < 0) {
+			ckpt_write_err(ctx, "Err %d collecting mm->exe_file",
+					ret);
 			goto out;
+		}
 	}
 	for (vma = mm->mmap; vma; vma = vma->vm_next) {
 		file = vma->vm_file;
 		if (file) {
 			ret = ckpt_collect_file(ctx, file);
-			if (ret < 0)
+			if (ret < 0) {
+				ckpt_write_err(ctx, "Err %d collecting vm_file",
+					ret);
 				break;
+			}
 		}
 	}
  out:
Signature · 1 line
1.6.1

A message that is not in the archive (sent off-list or not yet received): 20090910223344.GA11718-r/[email protected]

#2 lore raw

I wonder if it can be useful to decide on a common "format", that can be useful in the future for automatic error analysis. E.g:

"[PID %d ERR %d]: .....", for error with a specific task, and "[PID %d ERR %d OBJ %d]: ......" for error with an object, and so on.

Or even a bit more fancy, like:

	ckpt_write_err("EO", "error message %p blah", err, obj, ptr);
			SPEC   FMT		      VARS...

Which ckpt_write_err() will translate to

	sprintf(s, "[PID %d ERR %d] FMT", VARS...);

So the SPEC "EO" (stands for ERR, OBJ) becomes "[PID %d ERR %d OBJ %d]: " (pid is mandatory, the rest requested by the caller):

	E -> ERR %d
	O -> OBJ %d
	P -> PTR %p
	S -> SYM %pS
	etc...

?

Oren.

Quoted 10 lines · Serge E. Hallyn

The main point is for new-comers to the checkpoint/restart tree to be able to help us debug their otherwise mysterious checkpoint failures.

---
 checkpoint/checkpoint.c |   28 +++++++++++++++++++++++-----
 checkpoint/files.c      |   37 ++++++++++++++++++++++++++++++-------
 checkpoint/memory.c     |   17 ++++++++++++++---
 3 files changed, 67 insertions(+), 15 deletions(-)

[...]

A message that is not in the archive (sent off-list or not yet received): [email protected]

#4 lore raw
Quoted 16 lines · Oren Laadan

I wonder if it can be useful to decide on a common "format", that can be useful in the future for automatic error analysis. E.g:

"[PID %d ERR %d]: .....", for error with a specific task, and "[PID %d ERR %d OBJ %d]: ......" for error with an object, and so on.

Or even a bit more fancy, like:

	ckpt_write_err("EO", "error message %p blah", err, obj, ptr);
			SPEC   FMT		      VARS...

Which ckpt_write_err() will translate to

	sprintf(s, "[PID %d ERR %d] FMT", VARS...);
Quoted 7 lines

So the SPEC "EO" (stands for ERR, OBJ) becomes "[PID %d ERR %d OBJ %d]: " (pid is mandatory, the rest requested by the caller):

	E -> ERR %d
	O -> OBJ %d
	P -> PTR %p
	S -> SYM %pS
	etc...

Yes, it'd be useful. BTW it also would be useful to have mktree/ restart auto-detect such error strings and report them.

-serge

A message that is not in the archive (sent off-list or not yet received): 20090911212742.GB15329-r/[email protected]

#6 lore raw
Quoted 4 lines · Serge E. Hallyn

Yes, it'd be useful. BTW it also would be useful to have mktree/ restart auto-detect such error strings and report them.

'ckptinfo -e' should already do this, but sure, we can add a switch to 'restart' to check it too. An dyes, in the kernel definitely.

Oren.

A message that is not in the archive (sent off-list or not yet received): [email protected]

#8 lore raw
Quoted 8 lines · Oren Laadan
Quoted 4 lines · Serge E. Hallyn

Yes, it'd be useful. BTW it also would be useful to have mktree/ restart auto-detect such error strings and report them.

'ckptinfo -e' should already do this, but sure, we can add a switch

Oh, ha, I wouldn't know, bc ckptinfo has never compiled for me, and I'd never yet cared to beat it into compiling.

Looks like it will work for me if I do

export KERNELHDR=/usr/include

in my shell before compiling, and use the following patch:

Makefile +0 -0
index afff3f5..f7bd93b 100644
@@ -48,7 +48,7 @@ mktree:       CFLAGS += -D__REENTRANT -pthread
 

ckptinfo_types.o: ckptinfo_types.c

	@echo $(CC) -c $(CFLAGS) $<

- @$(CC) -c $(CFLAGS) $<
+ @$(CC) -D__GLIBC__=2 -c $(CFLAGS) $<

ckptinfo_types.c: $(CKPT_HEADERS) ckptinfo.py

	@echo cat $(CKPT_HEADERS) | ./ckptinfo.py > ckptinfo_types.c
Quoted 3 lines

to 'restart' to check it too. An dyes, in the kernel definitely.

Oren.