[CRIU] [PATCH]v2 criu: cr_service for crtools and criu_dump_me for criu library
Pavel Emelyanov
xemul at parallels.com
Thu Aug 8 05:13:33 EDT 2013
On 08/08/2013 05:10 AM, Ruslan Kuprieiev wrote:
> Hello!
>
> I make a cr_service() function for crtools and criu_dump_me for criu
> library. It is used to interact with programs though function criu
> library. cr_service() opens a socket and listening to connections on it.
> criu_dump_me() gets a proper argument from program and sends argument to
> socket.
>
> commit 4429040cbe0b159b367a0d5da23090746b4ac2fd
> Author: Ruslan Kuprieiev <kupruser at gmail.com>
> Date: Thu Aug 8 03:46:18 2013 +0300
This trash left from git show shouldn't be here. Plz, look at how people
on criu@ format their patches and do the same.
Next, linux/scripts/checkpatch.pl reports
total: 13 errors, 8 warnings, 504 lines checked
Plz, fix them _all_
And more comments inline.
> +int cr_service(void)
> +{
> + int server_fd;
> + int client_fd;
> +
> + server_fd = socket(AF_LOCAL, SOCK_STREAM, 0);
> + if (server_fd == -1) {
> + pr_perror("Can't initialize service socket.");
> + return -1;
> + }
> +
> + struct sockaddr_un server_addr;
> + struct sockaddr_un client_addr;
> +
> + memset(&server_addr, 0, sizeof(server_addr));
> + memset(&client_addr, 0, sizeof(client_addr));
> + server_addr.sun_family = AF_LOCAL;
> +
> + socklen_t server_addr_len;
> + socklen_t client_addr_len;
> +
> + if (opts.addr == NULL) {
> + opts.addr = strdup(CR_DEFAULT_SERVICE_ADDRESS);
What is strdup()-ing for?
> + }
> +
> + strcpy(server_addr.sun_path, opts.addr);
>
> + server_addr_len = strlen(server_addr.sun_path)
> + + sizeof(server_addr.sun_family);
> + client_addr_len = sizeof(client_addr);
> +
> + unlink(server_addr.sun_path);
> +
> + if (bind(server_fd, (struct sockaddr *) &server_addr,
> + server_addr_len) == -1) {
> + pr_perror("Can't bind.");
> + return -1;
> + }
> +
> + if (chmod(server_addr.sun_path, 0666)) {
> + pr_perror("Can't change permissions of the service socket.");
> + return -1;
> + }
> +
> + if (listen(server_fd, 16) == -1) {
> + pr_perror("Can't listen for socket connections.");
> + return -1;
> + }
> +
> + struct criu_dump_args *criu_arg;
> + criu_arg = malloc(sizeof(*criu_arg));
> +
> + pid_t child_pid;
> +
> + int ret;
> +
> + struct ucred ids;
> + socklen_t ids_len;
> + ids_len = sizeof(struct ucred);
> +
> + while(1) {
> + pr_info("Waiting for connection...\n");
> +
> + client_fd = accept(server_fd, &client_addr, &client_addr_len);
> + if (client_fd == -1) {
> + pr_perror("Can't accept connection.");
> + continue;
> + }
> +
> + pr_info("Connected.\n");
> +
> + switch(child_pid = fork()) {
> + case -1:
> + pr_perror("Can't fork a child.");
> + continue;
> + case 0:
> + if (getsockopt(client_fd, SOL_SOCKET, SO_PEERCRED, &ids, &ids_len)) {
> + pr_perror("Can't get socket options.");
> + goto child_err;
> + }
> +
> + client_addr_len = criu_recv_dump_arg(client_fd, criu_arg);
> + if (client_addr_len == -1) {
> + pr_perror("Can't get msg from socket.");
> + goto child_err;
> + }
> +
> + if (fchdir(criu_arg->images_dir_fd)) {
> + pr_perror("Can't change directory.");
> + goto child_err;
> + }
> +
> + opts.output = malloc(sizeof(char)*PATH_MAX);
> + strcpy(opts.output, getcwd(NULL, PATH_MAX));
> + strcat(opts.output, "/dump.log");
> +
> + if (open_image_dir() < 0) {
> + pr_perror("Can't open current directory.");
> + goto child_err;
> + }
> +
> + log_set_loglevel(3);/*loglevel is temporary constant*/
I wouldn't change the loglevel, but left it with the one service is launched.
> + if (log_init(opts.output) == -1) {
> + pr_perror("Can't initiate log.");
> + goto child_err;
> + }
> +
> + switch (criu_arg->flags) {
> + case CRIU_DUMP_AND_CONTINUE:
> + opts.final_state = TASK_ALIVE;
> + }
The whole switch is wrong.
1. You should check that flags don't contain unknown values,
2. You should check particlar flags with if (flag & value), not
if (flag == value).
> +
> + struct stat tmp;
> + if (fstat(client_fd, &tmp)) {
> + pr_perror("Can't get socket stat.");
> + goto child_err;
> + }
> + cr_service_sk_ino = tmp.st_ino;
> +
> + if (cr_dump_tasks(ids.pid) < 0) {
> + pr_perror("Dumping failed.");
> + goto child_err;
> + }
> +
> + if (opts.final_state == TASK_ALIVE) {
> + ret = CRIU_DUMP_SUCCESS;
> + send(client_fd, &ret, sizeof(ret), 0);
Error checking
> + }
> +
> + close(client_fd);
> + exit(0);
> +
> +child_err: ret = CRIU_DUMP_FAIL;
> + send(client_fd, &ret, sizeof(ret), 0);
Error checking
> + close(client_fd);
> + exit(-1);
> +
> + default:
> + close(client_fd);
> +
> + pid_t who;
> + while ((who = waitpid(-1, &ret, WNOHANG)) != 0) {
You only collect zombies after serving another request? This is wrong.
> + if (ret)
> + pr_perror("Child %d exited with problems.\n", who);
> + }
> + }
> + }
> +
> + return 0;
> +}
> @@ -994,7 +994,7 @@ static int collect_pstree_ids(void)
> return 0;
> }
>
> -static int collect_pstree(pid_t pid)
> +int collect_pstree(pid_t pid)
What for? Unused.
> {
> int ret, attempts = 5;
>
> @@ -235,14 +235,11 @@ int main(int argc, char *argv[])
> opts.use_page_server = true;
> break;
> case 51:
> - if (!inet_aton(optarg, &opts.ps_addr.sin_addr)) {
> - pr_perror("Bad address");
> - return -1;
> - }
> + opts.addr = strdup(optarg);
> break;
> case 52:
> - opts.ps_addr.sin_port = htons(atoi(optarg));
> - if (!opts.ps_addr.sin_port) {
> + opts.port = htons(atoi(optarg));
Converting addr from sockaddr into char * should be in separate patch.
> + if (!opts.port) {
> pr_err("Bad port\n");
> return -1;
> }
> +++ b/include/libcriu.h
> @@ -0,0 +1,19 @@
> +#ifndef __CRIU_LIB__
> +#define __CRIU_LIB__
> +
> +#define CR_DEFAULT_SERVICE_ADDRESS "/tmp/criu_service.socket"
> +
> +#define CRIU_DUMP_AND_CONTINUE 1
Comment that this thing is for flags is required.
> +
> +#define CRIU_DUMP_FAIL -1
> +#define CRIU_RESUME 1
> +#define CRIU_DUMP_SUCCESS 0
Comment what this all means is also required.
> +
> +struct criu_dump_args {
> + int images_dir_fd;
> + unsigned long flags;
Fields should be described in comments.
> +};
> +
> +int criu_dump_me(char sk_address [], struct criu_dump_args * arg);
The 1st arg should be
a) a member on the criu_dump_args
b) optional and CR_DEFAULT_SERVICE_ADDRESS is to be used by default
> +
> +#endif
> +int criu_dump_me(char sk_address[], struct criu_dump_args *arg)
> +{
> + int socket_fd;
> + socket_fd = socket(AF_LOCAL, SOCK_STREAM, 0);
> + if (socket_fd == -1) {
> + perror("socket() failed");
> + return -1;
> + }
> +
> + struct sockaddr_un server_addr;
> +
> + memset(&server_addr, 0, sizeof(server_addr));
> + server_addr.sun_family = AF_LOCAL;
> + strcpy(server_addr.sun_path, sk_address);
> +
> + socklen_t server_addr_len;
> +
> + server_addr_len = strlen(server_addr.sun_path)
> + + sizeof(server_addr.sun_family);
> +
> + if (connect(socket_fd, (struct sockaddr *) &server_addr,
> + server_addr_len) != 0) {
> + perror("connect() fail");
> + return -1;
> + }
> +
> + if (criu_send_dump_arg(socket_fd, arg) == -1) {
> + perror("criu_send_dump_arg() failed");
> + close(socket_fd);
> + return -1;
> + }
> +
> + int ret;
> + int c;
> +
> + c = read(socket_fd, &ret, sizeof(ret));
> + if (c == -1)
> + ret = CRIU_RESUME;
This is not correct. If read from socket failed, we should report failure.
In order to tell it we have resumed, we should put the respective packet
into this sockets' queue, so it actually _reads_ one after resume.
> +
> + close(socket_fd);
> +
> + return ret;
> +}
> @@ -169,7 +178,6 @@ static int dump_one_unix_fd(int lfd, u32 id, const struct fd_parms *p)
> goto err;
> }
> }
> -
> /*
> * It can be external socket, so we defer dumping
> * until all sockets the program owns are processed.
What's that?
> --- a/lib/Makefile
> +++ b/lib/Makefile
> @@ -3,7 +3,8 @@
> all: libcriu.so
>
> libcriu.so: criu.o
> - $(Q) $(CC) $(CFLAGS) -shared -o $@ criu.o
> + $(Q) $(CC) $(CFLAGS) -c -fPIC -D __CRIU_LIB_LINKER__ ../pie/util-net.c -I ../include/ -I ../ -I ../arch/$(ARCH)/include
> + $(Q) $(CC) $(CFLAGS) -shared -o $@ criu.o util-net.o
I'm not sure this is correct way of linking library out of several .c files.
Please, rework pie/util-net.c compilation, so that it compiled into two .o files,
the first one is to be linked with pie/ and the other one -- with crtools _and_
this library. And in latter case the CR_NO_GLIBC macro should not be defined.
Ask Cyrill (in Cc) for help if you need.
>
> criu.o:
> $(Q) $(CC) $(CFLAGS) -fPIC -c criu.c -I ../include/
Thanks,
Pavel
More information about the CRIU
mailing list