[CRIU] [PATCH] parasite: Refactor code to follow calling convention
Cyrill Gorcunov
gorcunov at openvz.org
Mon Mar 19 04:02:18 EDT 2012
Hi Pavel,
this patch should simplify parasite code error handling. There is
only one helper for setting error code SET_PARASITE_RET. Also it's
possible to call parasite_execute without arguments at all (in
case if we have kind of function which never fails inside parasite).
For example
parasite_dump_pages_seized
...
parasite_execute(PARASITE_CMD_DUMPPAGES_FINI, ctl, NULL, 0);
the parasite_execute in turn has own BUG_ON just to check noone is breaking
this convention.
Overall I think this should simplify the code. Please review when you get
opportunity.
Cyrill
-------------- next part --------------
>From dfcf5e043310635b372afa03b0b28722a8f8ce2a Mon Sep 17 00:00:00 2001
From: Cyrill Gorcunov <gorcunov at openvz.org>
Date: Mon, 19 Mar 2012 11:56:48 +0400
Subject: [PATCH] parasite: Refactor code to follow calling convention
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_RET helper. 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 | 25 ++------
parasite-syscall.c | 26 +++-----
parasite.c | 178 ++++++++++++++++++++++++++++++----------------------
3 files changed, 118 insertions(+), 111 deletions(-)
diff --git a/include/parasite.h b/include/parasite.h
index 413f21d..5d01fae 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,16 +35,14 @@ 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_RET(st, err) \
+ do { \
+ (st)->ret = err, \
+ (st)->line = __LINE__; \
} while (0)
struct parasite_init_args {
diff --git a/parasite-syscall.c b/parasite-syscall.c
index d1d63a8..5b0753f 100644
--- a/parasite-syscall.c
+++ b/parasite-syscall.c
@@ -248,19 +248,21 @@ static int parasite_execute_by_pid(unsigned long cmd, struct parasite_ctl *ctl,
}
memcpy(ctl->addr_cmd, &cmd, sizeof(cmd));
- memcpy(ctl->addr_args, args, args_size);
+ if (args)
+ memcpy(ctl->addr_args, args, args_size);
parasite_setup_regs(ctl->parasite_ip, ®s);
ret = __parasite_execute(ctl, pid, ®s);
- memcpy(args, ctl->addr_args, args_size);
- if (!ret)
- ret = args->ret;
+ if (args)
+ memcpy(args, ctl->addr_args, args_size);
+
+ BUG_ON(ret && !args);
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, ®s_orig)) {
@@ -542,9 +544,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 +583,8 @@ int parasite_dump_pages_seized(struct parasite_ctl *ctl, struct list_head *vma_a
(parasite_status_t *) ¶site_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;
@@ -594,7 +594,7 @@ int parasite_dump_pages_seized(struct parasite_ctl *ctl, struct list_head *vma_a
nrpages_dumped += parasite_dumppages.nrpages_dumped;
}
- parasite_execute(PARASITE_CMD_DUMPPAGES_FINI, ctl, st, sizeof(*st));
+ parasite_execute(PARASITE_CMD_DUMPPAGES_FINI, ctl, NULL, 0);
if (write_img(cr_fdset->fds[CR_FD_PAGES], &zero_page_entry))
goto out;
@@ -620,11 +620,7 @@ int parasite_cure_seized(struct parasite_ctl *ctl)
if (ctl->parasite_ip) {
ctl->signals_blocked = 0;
-
- if (parasite_execute(PARASITE_CMD_FINI, ctl, &args, sizeof(args))) {
- pr_err("Can't finalize parasite (pid: %d) task\n", ctl->pid);
- ret = -1;
- }
+ parasite_execute(PARASITE_CMD_FINI, ctl, NULL, 0);
}
if (ctl->remote_map) {
diff --git a/parasite.c b/parasite.c
index 29fd562..028e02e 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;
@@ -98,21 +117,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_RET(st, ret);
return -1;
}
@@ -126,7 +146,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;
@@ -145,8 +165,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_RET(st, -ENOMEM);
goto err;
}
@@ -162,8 +181,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_RET(st, ret);
goto err_free;
}
}
@@ -176,8 +194,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_RET(st, ret);
goto err_free;
}
@@ -196,8 +213,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_RET(st, -EIO);
goto err_free;
}
@@ -214,15 +231,12 @@ 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_RET(st, ret);
goto err_free;
}
}
- /* on success ret = 0 */
- SET_PARASITE_STATUS(st, ret, ret);
-
+ ret = 0;
err_free:
brk_free(nrpages);
err:
@@ -233,6 +247,7 @@ static int dump_pages_fini(void)
{
sys_close(fd_pages[PG_PRIV]);
sys_close(fd_pages[PG_SHARED]);
+
return 0;
}
@@ -241,12 +256,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_RET(st, fd);
return fd;
+ }
sys_lseek(fd, MAGIC_OFFSET, SEEK_SET);
@@ -257,8 +273,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_RET(st, ret);
goto err_close;
}
@@ -270,15 +285,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_RET(st, -EIO);
goto err_close;
}
}
ret = 0;
- SET_PARASITE_STATUS(st, 0, ret);
-
err_close:
sys_close(fd);
return ret;
@@ -293,8 +305,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_RET(st, ret);
+ return ret;
}
ie.isec = val.it_interval.tv_sec;
@@ -305,8 +317,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_RET(st, -EIO);
+ return -1;
}
return 0;
@@ -317,12 +329,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_RET(st, fd);
return fd;
+ }
sys_lseek(fd, MAGIC_OFFSET, SEEK_SET);
@@ -338,9 +351,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;
@@ -357,7 +367,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;
}
@@ -367,12 +376,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_RET(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;
@@ -386,8 +398,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
@@ -395,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_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.
@@ -409,17 +419,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;
}
@@ -435,12 +442,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) {
@@ -449,23 +455,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;
@@ -477,21 +485,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_RET(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;
@@ -499,19 +506,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_RET(st, ret);
return -1;
+ }
tsock = sys_socket(PF_UNIX, SOCK_DGRAM, 0);
if (tsock < 0) {
+ SET_PARASITE_RET(st, tsock);
return -1;
}
ret = sys_bind(tsock, (struct sockaddr *) &args->saddr, args->sun_len);
if (ret < 0) {
+ SET_PARASITE_RET(st, ret);
return -1;
}
@@ -522,14 +535,22 @@ 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)
@@ -539,6 +560,7 @@ static int fini(void)
sys_close(logfd);
sys_close(tsock);
brk_fini();
+
return 0;
}
@@ -556,7 +578,7 @@ static int __used parasite_service(unsigned long cmd, void *args)
case PARASITE_CMD_FINI:
return fini();
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:
@@ -574,7 +596,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_RET(st, -EINVAL);
+ sys_write_msg("Unknown command to parasite\n");
+ }
break;
}
--
1.7.7.6
More information about the CRIU
mailing list