[CRIU] [PATCH] criu: cr_service for crtools and criu_dump_me for libcriu

[Pavel Emelyanov]

On 07/30/2013 01:42 AM, Ruslan Kupreev wrote:
> Hello all!
> 
> I make a cr_service() function for crtools. It will be used to interact with programs though function
> criu_dump_me() from criu library. cr_service() opens socket and listening to connections to it.
> criu_dump_me() gets as an argument a link to struct criu_dump_args, that, for now, contain images_dir_fd
> and flags(temporarily unused). And sends it to socket.
> criu.org/Self_dump for more details.
> 
> Signed-off-by: Ruslan Kupreev kupruser at gmail.com <mailto:kupruser at gmail.com>
> 
> 

First of all -- read the kernel's coding style document and fix your patch according
to this. You may use kernel's scripts/checkpatch.pl script for help.

Now, some comments to the patch.

> +int cr_dump_tasks_continue(pid_t pid)
> +{
> +	struct pstree_item *item;
> +	int post_dump_ret = 0;
> +	int ret = -1;
> +
> +	pr_info("========================================\n");
> +	pr_info("Dumping processes (pid: %d)\n", pid);
> +	pr_info("========================================\n");
> ...

No, no, no. Don't do copy-n-paste, especially of such HUGE amounts of code.
Better equip the existing cr_dump_tasks() with a bool argument and check it
to decide how the dump work-flow should be changed.

> +int cr_service(void) {
> +
> +	/*sockets initialization*/
> +	
> +	int server_fd = socket(AF_LOCAL, SOCK_STREAM, 0);

It's not in the coding style, but initializing a variable in declaration
with anything, that is not constant, is not welcome.

> +	int client_fd;
> +	
> +	if (server_fd == -1) {
> +		pr_perror("socket() failed");
> +		exit(1);
> +	}
> +	
> +	struct sockaddr_un server_addr, client_addr;
> +	memset(&server_addr, 0, sizeof(server_addr));
> +	memset(&client_addr, 0, sizeof(client_addr));
> +	socklen_t server_addr_len, client_addr_len;
> +	server_addr.sun_family = AF_LOCAL;
> +	strcpy(server_addr.sun_path, "/tmp/criu_service.soc");

Hard-coded socket name is not cool. Do this:

1. Patch the crtools to be able to accept socket address via existing
   --address option. It's now struct sockaddr_in, but should be char *
   and address should be treated and converted properly where required.

2. Introduce a macro named CR_DEFAULT_SERVICE_ADDRESS with this string
   and use _it_.

> +		/* calling fork() twice to avoid zombies */

No, crtools instances dumping tasks should not be reparented to init.
Main crtools task should have full control over them.

> +		switch(child_pid = fork()) {
> +		case -1:
> +			pr_perror("fork() failed");
> +			continue;

> +static int recv_fd(int socket_fd) {

This version of recvfd is excessive -- cr_service is withing the crtools
binary and have existing fd receiving routine linked in.

> +int criu_recv_dump_arg(int socket_fd, struct criu_dump_args* arg) {

Should be static.

> @@ -0,0 +1,10 @@
> +struct criu_dump_args {
> +	int images_dir_fd;
> +	unsigned long flags;

In the next version of patch, introduce one flag:

CRIU_DUMP_AND_CONTINUE

which would mean the same as --leave-running option for cmdline.

> +};
> +
> +int criu_dump_me(struct criu_dump_args * arg);
> +
> +/*разобраться с возвращаемым типом*/

No Russian comments please.

> +int criu_send_dump_arg(int socket_fd, struct criu_dump_args * arg);
> +int criu_recv_dump_arg(int socket_fd, struct criu_dump_args * arg);

Besides, this header should be splitted into two -- one would be shipped
in -devel package to be used by those, who want to use the library. The
other one is to be used internally in criu sources.

> -libcriu.so: criu.o
> -	$(Q) $(CC) $(CFLAGS) -shared -o $@ criu.o
>  
> -criu.o:
> -	$(Q) $(CC) $(CFLAGS) -fPIC -c criu.c -I ../include/
> +libcriu.so: libcriu.o
> +	$(Q) $(CC) $(CFLAGS) -shared -o $@ libcriu.o
> +
> +libcriu.o: libcriu.c
> +	$(Q) $(CC) $(CFLAGS) -fPIC -c libcriu.c -I ../include/

The .c file name for library is criu.c and I see no reasons for changing it.

> +const char* criu_lib_version = version;
> +

This thing is already there. Update your repo and prepare patches on top of
HEAD, not some month-old version of it.

> +int criu_dump_me(struct criu_dump_args * arg) {
> ...
> +	ret = read(socket_fd, &buf, 1);
> +	(void)ret;

We should convert this value into an error code returned from the library.
The values should be stated in libcriu.h file. Now I see 3 values for it:

CRIU_DUMP_SUCCESS == 0 (when CRIU_DUMP_AND_CONTINUE flag is given)
CRIU_DUMP_FAIL == -1
CRIU_RESUME == 1

And we'll state, that "negative value means an error", so we'll be able to
extend the set of errors later.

> +	close(socket_fd);
> +	
> +	return 0;
> +}

> +static int send_fd(int socket_fd, int fd) {

I don't quite like that we copy the text of send_fd. I understand the linking
problem, let's do the thing: link library with the pie/util-net.c and make it
be compiled with "-D sys_sendmsg=sendmsg" option not to use the raw system call.

Thanks,
Pavel