[CRIU] [PATCH] parasite: Refactor code to follow calling convention

Cyrill Gorcunov gorcunov at openvz.org
Fri Mar 16 10:04:54 EDT 2012


It happened some routines in parasite service code
were not following calling convention so I fixed the
callers and added a comment about adding new code here.

At the same time the 3 error code madness is dropped
as being requested by Pavel -- now we return one error
code only.

The PARASITE_ERR_ codes were dropped as well due to
become redundant.

The status of parasite service routine is set via
SET_PARASITE helpers

 - SET_PARASITE_ERR, if you have some error code
   to return

 - SET_PARASITE_RET, if you have some value to return,
   but you don't know if it's error or not (basically
   it's simply semanatic wrapper over SET_PARASITE_ERR,
   should be used in places where _ERR version migh confuse
   the reader)

In case if there is no error -- just return 0. The calling
code should not expect to find anything sane in parasite_status_t
because parasite code might not touch it at all.

Same time, due to this convention the parasite's
dump_socket_queue is getting rid of third @err
member, because it's now returned as a regular
error code.

Signed-off-by: Cyrill Gorcunov <gorcunov at openvz.org>
---
 include/parasite.h |   28 ++------
 parasite-syscall.c |   11 +--
 parasite.c         |  186 +++++++++++++++++++++++++++++----------------------
 3 files changed, 117 insertions(+), 108 deletions(-)

diff --git a/include/parasite.h b/include/parasite.h
index 413f21d..dbad64d 100644
--- a/include/parasite.h
+++ b/include/parasite.h
@@ -16,19 +16,6 @@
 
 #define PARASITE_MAX_SIZE	(64 << 10)
 
-/* we need own error code for diagnostics */
-#define PARASITE_ERR_FAIL	-1024
-#define PARASITE_ERR_OPEN	-1025
-#define PARASITE_ERR_MMAP	-1026
-#define PARASITE_ERR_MINCORE	-1027
-#define PARASITE_ERR_MUNMAP	-1028
-#define PARASITE_ERR_CLOSE	-1029
-#define PARASITE_ERR_WRITE	-1030
-#define PARASITE_ERR_MPROTECT	-1031
-#define PARASITE_ERR_SIGACTION  -1032
-#define PARASITE_ERR_GETITIMER  -1033
-#define PARASITE_ERR_IOCTL	-1034
-
 enum {
 	PARASITE_CMD_INIT,
 	PARASITE_CMD_SET_LOGFD,
@@ -48,18 +35,19 @@ enum {
 };
 
 typedef struct  {
-	long			ret;		/* custom ret code */
-	long			sys_ret;	/* syscall ret code */
+	long			ret;		/* ret code */
 	long			line;		/* where we're failed */
 } parasite_status_t;
 
-#define SET_PARASITE_STATUS(st, ret_code, sys_ret_code)	\
-	do {						\
-		(st)->ret	= ret_code,		\
-		(st)->sys_ret	= sys_ret_code,		\
-		(st)->line	= __LINE__;		\
+#define SET_PARASITE_ERR(st, err)		\
+	do {					\
+		(st)->ret	= err,		\
+		(st)->line	= __LINE__;	\
 	} while (0)
 
+#define SET_PARASITE_RET(st, ret)		\
+	SET_PARASITE_ERR(st, ret)
+
 struct parasite_init_args {
 	parasite_status_t	status;
 
diff --git a/parasite-syscall.c b/parasite-syscall.c
index d1d63a8..35c3ed7 100644
--- a/parasite-syscall.c
+++ b/parasite-syscall.c
@@ -255,12 +255,9 @@ static int parasite_execute_by_pid(unsigned long cmd, struct parasite_ctl *ctl,
 	ret = __parasite_execute(ctl, pid, &regs);
 
 	memcpy(args, ctl->addr_args, args_size);
-	if (!ret)
-		ret = args->ret;
-
 	if (ret)
 		pr_err("Parasite exited with %d ret (%li at %li)\n",
-		       ret, args->sys_ret, args->line);
+		       ret, args->ret, args->line);
 
 	if (ctl->pid != pid)
 		if (ptrace(PTRACE_SETREGS, pid, NULL, &regs_orig)) {
@@ -542,9 +539,8 @@ int parasite_dump_pages_seized(struct parasite_ctl *ctl, struct list_head *vma_a
 
 	ret = parasite_execute(PARASITE_CMD_DUMPPAGES_INIT, ctl, st, sizeof(*st));
 	if (ret < 0) {
-		pr_err("Dumping pages failed with %li (%li) at %li\n",
+		pr_err("Dumping pages failed with %li at %li\n",
 				parasite_dumppages.status.ret,
-				parasite_dumppages.status.sys_ret,
 				parasite_dumppages.status.line);
 		goto out;
 	}
@@ -582,9 +578,8 @@ int parasite_dump_pages_seized(struct parasite_ctl *ctl, struct list_head *vma_a
 				       (parasite_status_t *) &parasite_dumppages,
 				       sizeof(parasite_dumppages));
 		if (ret) {
-			pr_err("Dumping pages failed with %li (%li) at %li\n",
+			pr_err("Dumping pages failed with %li at %li\n",
 				 parasite_dumppages.status.ret,
-				 parasite_dumppages.status.sys_ret,
 				 parasite_dumppages.status.line);
 
 			goto out;
diff --git a/parasite.c b/parasite.c
index 3ae474e..53b34a3 100644
--- a/parasite.c
+++ b/parasite.c
@@ -9,6 +9,25 @@
 #include "util.h"
 #include "util-net.h"
 
+/*
+ * Some notes on parasite code overall. There are a few
+ * calling convention specfics the caller must follow
+ *
+ * - on success, 0 must be returned, anything else
+ *   treated as error; note that if 0 returned the
+ *   caller code should not expect anything sane in
+ *   parasite_status_t, parasite may not touch it at
+ *   all
+ *
+ * - every routine which takes arguments and called from
+ *   parasite_head
+ *     parasite_service
+ *   must provide parasite_status_t argument either via
+ *   plain pointer or as first member of an embedding
+ *   structure so service routine will pass error code
+ *   there
+ */
+
 #ifdef CONFIG_X86_64
 
 static void *brk_start, *brk_end, *brk_tail;
@@ -106,21 +125,22 @@ static int fd_pages[2] = { -1, -1 };
 
 static int dump_pages_init(parasite_status_t *st)
 {
-	fd_pages[PG_PRIV] = recv_fd(tsock);
-	if (fd_pages[PG_PRIV] < 0)
+	int ret;
+
+	ret = fd_pages[PG_PRIV] = recv_fd(tsock);
+	if (ret < 0)
 		goto err;
 
-	fd_pages[PG_SHARED] = recv_fd(tsock);
-	if (fd_pages[PG_SHARED] < 0)
+	ret = fd_pages[PG_SHARED] = recv_fd(tsock);
+	if (ret < 0)
 		goto err_s;
 
-	SET_PARASITE_STATUS(st, 0, 0);
 	return 0;
 
 err_s:
 	sys_close(fd_pages[PG_PRIV]);
 err:
-	SET_PARASITE_STATUS(st, PARASITE_ERR_FAIL, -1);
+	SET_PARASITE_ERR(st, ret);
 	return -1;
 }
 
@@ -134,7 +154,7 @@ static int dump_pages(struct parasite_dump_pages_args *args)
 	unsigned long nrpages, pfn, length;
 	unsigned long prot_old, prot_new;
 	unsigned char *map;
-	int ret = PARASITE_ERR_FAIL, fd;
+	int ret = -1, fd;
 
 	args->nrpages_dumped = 0;
 	prot_old = prot_new = 0;
@@ -153,8 +173,7 @@ static int dump_pages(struct parasite_dump_pages_args *args)
 	 */
 	map = brk_alloc(nrpages);
 	if (!map) {
-		SET_PARASITE_STATUS(st, PARASITE_ERR_MMAP, (long)map);
-		ret = st->ret;
+		SET_PARASITE_ERR(st, -ENOMEM);
 		goto err;
 	}
 
@@ -170,8 +189,7 @@ static int dump_pages(struct parasite_dump_pages_args *args)
 				   prot_new);
 		if (ret) {
 			sys_write_msg("sys_mprotect failed\n");
-			SET_PARASITE_STATUS(st, PARASITE_ERR_MPROTECT, ret);
-			ret = st->ret;
+			SET_PARASITE_ERR(st, ret);
 			goto err_free;
 		}
 	}
@@ -184,8 +202,7 @@ static int dump_pages(struct parasite_dump_pages_args *args)
 	ret = sys_mincore((unsigned long)args->vma_entry.start, length, map);
 	if (ret) {
 		sys_write_msg("sys_mincore failed\n");
-		SET_PARASITE_STATUS(st, PARASITE_ERR_MINCORE, ret);
-		ret = st->ret;
+		SET_PARASITE_ERR(st, ret);
 		goto err_free;
 	}
 
@@ -204,8 +221,8 @@ static int dump_pages(struct parasite_dump_pages_args *args)
 			written += sys_write(fd, &vaddr, sizeof(vaddr));
 			written += sys_write(fd, (void *)vaddr, PAGE_SIZE);
 			if (written != sizeof(vaddr) + PAGE_SIZE) {
-				SET_PARASITE_STATUS(st, PARASITE_ERR_WRITE, written);
-				ret = st->ret;
+				ret = -1;
+				SET_PARASITE_ERR(st, -EIO);
 				goto err_free;
 			}
 
@@ -222,25 +239,23 @@ static int dump_pages(struct parasite_dump_pages_args *args)
 				   prot_old);
 		if (ret) {
 			sys_write_msg("PANIC: Ouch! sys_mprotect failed on restore\n");
-			SET_PARASITE_STATUS(st, PARASITE_ERR_MPROTECT, ret);
-			ret = st->ret;
+			SET_PARASITE_ERR(st, ret);
 			goto err_free;
 		}
 	}
 
-	/* on success ret = 0 */
-	SET_PARASITE_STATUS(st, ret, ret);
-
+	ret = 0;
 err_free:
 	brk_free(nrpages);
 err:
 	return ret;
 }
 
-static int dump_pages_fini(void)
+static int dump_pages_fini(parasite_status_t *st)
 {
 	sys_close(fd_pages[PG_PRIV]);
 	sys_close(fd_pages[PG_SHARED]);
+
 	return 0;
 }
 
@@ -249,12 +264,13 @@ static int dump_sigact(parasite_status_t *st)
 	rt_sigaction_t act;
 	struct sa_entry e;
 	int fd, sig;
-
-	int ret = PARASITE_ERR_FAIL;
+	int ret;
 
 	fd = recv_fd(tsock);
-	if (fd < 0)
+	if (fd < 0) {
+		SET_PARASITE_ERR(st, fd);
 		return fd;
+	}
 
 	sys_lseek(fd, MAGIC_OFFSET, SEEK_SET);
 
@@ -265,8 +281,7 @@ static int dump_sigact(parasite_status_t *st)
 		ret = sys_sigaction(sig, NULL, &act);
 		if (ret < 0) {
 			sys_write_msg("sys_sigaction failed\n");
-			SET_PARASITE_STATUS(st, PARASITE_ERR_SIGACTION, ret);
-			ret = st->ret;
+			SET_PARASITE_ERR(st, ret);
 			goto err_close;
 		}
 
@@ -278,15 +293,12 @@ static int dump_sigact(parasite_status_t *st)
 		ret = sys_write(fd, &e, sizeof(e));
 		if (ret != sizeof(e)) {
 			sys_write_msg("sys_write failed\n");
-			SET_PARASITE_STATUS(st, PARASITE_ERR_WRITE, ret);
-			ret = st->ret;
+			SET_PARASITE_ERR(st, -EIO);
 			goto err_close;
 		}
 	}
 
 	ret = 0;
-	SET_PARASITE_STATUS(st, 0, ret);
-
 err_close:
 	sys_close(fd);
 	return ret;
@@ -301,8 +313,8 @@ static int dump_itimer(int which, int fd, parasite_status_t *st)
 	ret = sys_getitimer(which, &val);
 	if (ret < 0) {
 		sys_write_msg("getitimer failed\n");
-		SET_PARASITE_STATUS(st, PARASITE_ERR_GETITIMER, ret);
-		return st->ret;
+		SET_PARASITE_ERR(st, ret);
+		return ret;
 	}
 
 	ie.isec = val.it_interval.tv_sec;
@@ -313,8 +325,8 @@ static int dump_itimer(int which, int fd, parasite_status_t *st)
 	ret = sys_write(fd, &ie, sizeof(ie));
 	if (ret != sizeof(ie)) {
 		sys_write_msg("sys_write failed\n");
-		SET_PARASITE_STATUS(st, PARASITE_ERR_WRITE, ret);
-		return st->ret;
+		SET_PARASITE_ERR(st, -EIO);
+		return -1;
 	}
 
 	return 0;
@@ -325,12 +337,13 @@ static int dump_itimers(parasite_status_t *st)
 	rt_sigaction_t act;
 	struct sa_entry e;
 	int fd, sig;
-
-	int ret = PARASITE_ERR_FAIL;
+	int ret = -1;
 
 	fd = recv_fd(tsock);
-	if (fd < 0)
+	if (fd < 0) {
+		SET_PARASITE_ERR(st, fd);
 		return fd;
+	}
 
 	sys_lseek(fd, MAGIC_OFFSET, SEEK_SET);
 
@@ -346,9 +359,6 @@ static int dump_itimers(parasite_status_t *st)
 	if (ret < 0)
 		goto err_close;
 
-	ret = 0;
-	SET_PARASITE_STATUS(st, 0, ret);
-
 err_close:
 	sys_close(fd);
 	return ret;
@@ -365,7 +375,6 @@ static int dump_misc(struct parasite_dump_misc *args)
 	args->brk = sys_brk(0);
 	args->blocked = old_blocked;
 
-	SET_PARASITE_STATUS(st, 0, 0);
 	return 0;
 }
 
@@ -375,12 +384,15 @@ static int dump_tid_addr(struct parasite_dump_tid_addr *args)
 	int ret;
 
 	ret = sys_prctl(PR_GET_TID_ADDR, (unsigned long) &args->tid_addr, 0, 0, 0);
+	if (ret) {
+		SET_PARASITE_ERR(st, ret);
+		return ret;
+	}
 
-	SET_PARASITE_STATUS(st, 0, ret);
 	return 0;
 }
 
-static int dump_socket_queue(int img_fd, struct sk_queue_item *item, int *err)
+static int dump_socket_queue(int img_fd, struct sk_queue_item *item)
 {
 	struct sk_packet_entry *pe;
 	unsigned long size;
@@ -394,8 +406,7 @@ static int dump_socket_queue(int img_fd, struct sk_queue_item *item, int *err)
 	ret = sys_getsockopt(sock_fd, SOL_SOCKET, SO_PEEK_OFF, &orig_peek_off, &tmp);
 	if (ret < 0) {
 		sys_write_msg("getsockopt failed\n");
-		*err = ret;
-		return PARASITE_ERR_FAIL;
+		return ret;
 	}
 	/*
 	 * Discover max DGRAM size
@@ -403,8 +414,7 @@ static int dump_socket_queue(int img_fd, struct sk_queue_item *item, int *err)
 	ret = sys_getsockopt(sock_fd, SOL_SOCKET, SO_SNDBUF, &ret, &tmp);
 	if (ret < 0) {
 		sys_write_msg("getsockopt failed\n");
-		*err = ret;
-		return PARASITE_ERR_FAIL;
+		return ret;
 	}
 	/*
 	 * Note: 32 bytes will be used by kernel for protocol header.
@@ -417,17 +427,14 @@ static int dump_socket_queue(int img_fd, struct sk_queue_item *item, int *err)
 	pe = brk_alloc(size + sizeof(struct sk_packet_entry));
 	if (!pe) {
 		sys_write_msg("not enough mem for skb\n");
-		*err = -ENOMEM;
-		return PARASITE_ERR_MMAP;
+		return -ENOMEM;
 	}
 	/*
 	 * Enable peek offset incrementation.
 	 */
-	ret = 0;
-	*err = sys_setsockopt(sock_fd, SOL_SOCKET, SO_PEEK_OFF, &ret, sizeof(int));
-	if (*err < 0) {
+	ret = sys_setsockopt(sock_fd, SOL_SOCKET, SO_PEEK_OFF, &ret, sizeof(int));
+	if (ret < 0) {
 		sys_write_msg("setsockopt fail\n");
-		ret = PARASITE_ERR_FAIL;
 		goto err_brk;
 	}
 
@@ -443,12 +450,11 @@ static int dump_socket_queue(int img_fd, struct sk_queue_item *item, int *err)
 			.msg_iovlen	= 1,
 		};
 
-		*err = pe->length = sys_recvmsg(sock_fd, &msg, MSG_DONTWAIT | MSG_PEEK);
-		if (*err < 0) {
-			if (*err == -EAGAIN)
+		ret = pe->length = sys_recvmsg(sock_fd, &msg, MSG_DONTWAIT | MSG_PEEK);
+		if (ret < 0) {
+			if (ret == -EAGAIN)
 				break; /* we're done */
 			sys_write_msg("sys_recvmsg fail: error\n");
-			ret = PARASITE_ERR_FAIL;
 			goto err_set_sock;
 		}
 		if (msg.msg_flags & MSG_TRUNC) {
@@ -457,23 +463,25 @@ static int dump_socket_queue(int img_fd, struct sk_queue_item *item, int *err)
 			 * to check...
 			 */
 			sys_write_msg("sys_recvmsg failed: truncated\n");
-			ret = PARASITE_ERR_FAIL;
-			*err = -E2BIG;
+			ret = -E2BIG;
 			goto err_set_sock;
 		}
-		*err = sys_write(img_fd, pe, sizeof(pe) + pe->length);
-		if (*err != sizeof(pe) + pe->length) {
+		ret = sys_write(img_fd, pe, sizeof(pe) + pe->length);
+		if (ret != sizeof(pe) + pe->length) {
 			sys_write_msg("sys_write failed\n");
-			ret = PARASITE_ERR_WRITE;
+			ret = -EIO;
 			goto err_set_sock;
 		}
 	}
 	ret = 0;
+
 err_set_sock:
 	/*
 	 * Restore original peek offset. 
 	 */
-	sys_setsockopt(sock_fd, SOL_SOCKET, SO_PEEK_OFF, &orig_peek_off, sizeof(int));
+	ret = sys_setsockopt(sock_fd, SOL_SOCKET, SO_PEEK_OFF, &orig_peek_off, sizeof(int));
+	if (ret < 0)
+		sys_write_msg("setsockopt failed on restore\n");
 err_brk:
 	brk_free(size + sizeof(struct sk_packet_entry));
 	return ret;
@@ -485,21 +493,20 @@ static int dump_skqueues(struct parasite_dump_sk_queues *args)
 	int img_fd, i, ret = -1;
 
 	img_fd = recv_fd(tsock);
-	if (img_fd < 0)
+	if (img_fd < 0) {
+		SET_PARASITE_ERR(st, img_fd);
 		return img_fd;
+	}
 
 	for (i = 0; i < args->nr_items; i++) {
-		int err;
-
-		ret = dump_socket_queue(img_fd, &args->items[i], &err);
+		ret = dump_socket_queue(img_fd, &args->items[i]);
 		if (ret < 0) {
-			SET_PARASITE_STATUS(st, ret, err);
+			SET_PARASITE_RET(st, ret);
 			goto err_dmp;
 		}
 	}
 
 	ret = 0;
-	SET_PARASITE_STATUS(st, 0, 0);
 err_dmp:
 	sys_close(img_fd);
 	return ret;
@@ -507,19 +514,25 @@ err_dmp:
 
 static int init(struct parasite_init_args *args)
 {
-	int ret;
+	parasite_status_t *st = &args->status;
 	k_rtsigset_t to_block;
+	int ret;
 
-	if (brk_init() < 0)
+	ret = brk_init();
+	if (ret) {
+		SET_PARASITE_ERR(st, ret);
 		return -1;
+	}
 
 	tsock = sys_socket(PF_UNIX, SOCK_DGRAM, 0);
 	if (tsock < 0) {
+		SET_PARASITE_ERR(st, tsock);
 		return -1;
 	}
 
 	ret = sys_bind(tsock, (struct sockaddr *) &args->saddr, args->sun_len);
 	if (ret < 0) {
+		SET_PARASITE_ERR(st, ret);
 		return -1;
 	}
 
@@ -530,23 +543,32 @@ static int init(struct parasite_init_args *args)
 	else
 		reset_blocked = 1;
 
-	SET_PARASITE_STATUS(&args->status, ret, ret);
+	SET_PARASITE_RET(st, ret);
 	return ret;
 }
 
-static int parasite_set_logfd(void)
+static int parasite_set_logfd(parasite_status_t *st)
 {
-	logfd = recv_fd(tsock);
-	return logfd;
+	int ret;
+
+	ret = recv_fd(tsock);
+	if (ret >= 0) {
+		logfd = ret;
+		ret = 0;
+	}
+
+	SET_PARASITE_RET(st, ret);
+	return ret;
 }
 
-static int fini(void)
+static int fini(parasite_status_t *st)
 {
 	if (reset_blocked == 1)
 		sys_sigprocmask(SIG_SETMASK, &old_blocked, NULL);
 	sys_close(logfd);
 	sys_close(tsock);
 	brk_fini();
+
 	return 0;
 }
 
@@ -562,13 +584,13 @@ static int __used parasite_service(unsigned long cmd, void *args)
 	case PARASITE_CMD_INIT:
 		return init((struct parasite_init_args *) args);
 	case PARASITE_CMD_FINI:
-		return fini();
+		return fini((parasite_status_t *)args);
 	case PARASITE_CMD_SET_LOGFD:
-		return parasite_set_logfd();
+		return parasite_set_logfd((parasite_status_t *)args);
 	case PARASITE_CMD_DUMPPAGES_INIT:
 		return dump_pages_init((parasite_status_t *) args);
 	case PARASITE_CMD_DUMPPAGES_FINI:
-		return dump_pages_fini();
+		return dump_pages_fini((parasite_status_t *)args);
 	case PARASITE_CMD_DUMPPAGES:
 		return dump_pages((struct parasite_dump_pages_args *)args);
 	case PARASITE_CMD_DUMP_SIGACTS:
@@ -582,7 +604,11 @@ static int __used parasite_service(unsigned long cmd, void *args)
 	case PARASITE_CMD_DUMP_SK_QUEUES:
 		return dump_skqueues((struct parasite_dump_sk_queues *)args);
 	default:
-		sys_write_msg("Unknown command to parasite\n");
+		{
+			parasite_status_t *st = (parasite_status_t *)args;
+			SET_PARASITE_ERR(st, -EINVAL);
+			sys_write_msg("Unknown command to parasite\n");
+		}
 		break;
 	}
 
-- 
1.7.7.6



More information about the CRIU mailing list