[CRIU] [PATCHv6 3/5] crtools: cr_service meat

[Ruslan Kuprieiev]

On 09/12/2013 02:14 PM, Pavel Emelyanov wrote:
> On 09/12/2013 01:00 AM, Ruslan Kuprieiev wrote:
>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>
>
>> @@ -1,6 +1,250 @@
>> -int cr_service()
>> +static int treat_req(CriuDumpReq *req)
> Function name is bad.

handle_req? Please, suggest. I'm bad at naming.

>
>> +{
>> +	struct ucred ids;
>> +	struct stat st;
>> +	socklen_t ids_len = sizeof(struct ucred);
>> +	char images_dir_path[PATH_MAX];
>> +
>> +	if (getsockopt(cr_service_client->sk_fd, SOL_SOCKET, SO_PEERCRED,
>> +							  &ids, &ids_len)) {
>> +		pr_perror("Can't get socket options.");
>> +		return -1;
>> +	}
>> +
>> +	cr_service_client->pid = ids.pid;
>> +	cr_service_client->uid = ids.uid;
>> +
>> +	if (req->pid == 0)
>> +		req->pid = ids.pid;
>> +
>> +	if (fstat(cr_service_client->sk_fd, &st)) {
>> +		pr_perror("Can't get socket stat");
>> +		return -1;
>> +	}
>> +
>> +	cr_service_client->sk_ino = st.st_ino;
>> +
>> +	/* going to dir, where to place images*/
>> +	sprintf(images_dir_path, "/proc/%d/fd/%d",
>> +		cr_service_client->pid, req->images_dir_fd);
>> +
>> +	if (chdir(images_dir_path)) {
>> +		pr_perror("Can't chdir to images directory");
>> +		return -1;
>> +	}
>> +
>> +	if (open_image_dir() < 0)
>> +		return -1;
>> +
>> +	/* initiate log file in imgs dir */
>> +	opts.output = xmalloc(PATH_MAX);
>> +	strcpy(opts.output, images_dir_path);
>> +	strcat(opts.output, "/dump.log");
> Use relative path for log file name.

I'm not sure why, but relative path does not work for me.
If i'm passing "./dump.log" to log_init, it creates log in current 
directory -- not in the images directory, despite chdir() above.

>> +
>> +	/* setting loglevel */
>> +	/*
>> +	 * FIXME why relative path does not work?
>> +	 * Maybe child uses env of parent?
>> +	 */
>> +	log_set_loglevel(req->log_level);
>> +	if (log_init(opts.output) == -1) {
>> +		pr_perror("Can't initiate log.");
>> +		return -1;
>> +	}
>> +
>> +	/* checking dump flags from client */
>> +	if (req->leave_running)
>> +		opts.final_state = TASK_ALIVE;
>> +
>> +	opts.ext_unix_sk	= req->ext_unix_sk;
>> +	opts.tcp_established_ok	= req->tcp_established;
>> +	opts.evasive_devices	= req->evasive_devices;
>> +	opts.shell_job		= req->shell_job;
>> +	opts.handle_file_locks	= req->file_locks;
>> +
>> +	return 0;
>> +}
>> +
>> +int cr_service(bool daemon_mode)
>>   {
>> +	int server_fd;
>> +	int child_pid;
>> +
>> +	struct sockaddr_un server_addr;
>> +	struct sockaddr_un client_addr;
>> +
>> +	socklen_t server_addr_len;
>> +	socklen_t client_addr_len;
>> +
>> +	CriuDumpReq *req = 0;
>> +	CriuDumpResp *resp;
>> +
>> +	cr_service_client = malloc(sizeof(struct _cr_service_client));
>> +	resp = xmalloc(sizeof(CriuDumpResp));
> The xmalloc can fail, but if you make resp a struct variable, not a pointer,
> you wouldn't have to allocate it.
>
>> +	criu_dump_resp__init(resp);
>> +
>> +	server_fd = socket(AF_LOCAL, SOCK_SEQPACKET, 0);
> I have a subtle feeling that you didn't test this. Criu cannot dump
> seq-packet sockets due to check in can_dump_unix_sk().

I do test this. I added check to sk-unix.c.

>
>> +	if (server_fd == -1) {
>> +		pr_perror("Can't initialize service socket.");
>> +		return -1;
>> +	}
>> +
>> +	memset(&server_addr, 0, sizeof(server_addr));
>> +	memset(&client_addr, 0, sizeof(client_addr));
>> +	server_addr.sun_family = AF_LOCAL;
>> +
>> +	if (opts.addr == NULL)
>> +		opts.addr = CR_DEFAULT_SERVICE_ADDRESS;
>> +
>> +	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;
>> +	}
>> +
>> +	pr_info("The service socket was bound to %s\n", server_addr.sun_path);
>> +
>> +	/* change service socket permissions, so anyone can connect to it */
>> +	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;
>> +	}
>> +
>> +	if (daemon_mode) {
>> +		if (daemon(0, 0) == -1) {
>> +			pr_perror("Can't run service server in the background");
>> +			return -errno;
>> +		}
>> +	}
>> +
>> +	/* FIXME Do not ignore children's return values */
>> +	signal(SIGCHLD, SIG_IGN);
>> +
>> +	while (1) {
>> +		pr_info("Waiting for connection...\n");
>> +
>> +		cr_service_client->sk_fd = accept(server_fd,
>> +						  &client_addr,
>> +						  &client_addr_len);
>> +		if (cr_service_client->sk_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 (recv_req(cr_service_client->sk_fd, &req) == -1) {
>> +				pr_perror("Can't recv request");
>> +				goto exit;
>> +			}
>> +
>> +			if (treat_req(req) == -1) {
>> +				pr_perror("Arguments treating fail");
>> +				goto exit;
>> +			}
>> +
>> +			if (cr_dump_tasks(req->pid) == -1) {
>> +				pr_perror("Dump fail");
>> +				goto exit;
>> +			}
>> +
>> +			resp->success = true;
>> +
>> +exit:
>> +			if (req->leave_running) {
>> +				if (send_resp(cr_service_client->sk_fd,
>> +								resp) == -1) {
>> +					pr_perror("Can't send response");
>> +					resp->success = false;
>> +				}
>> +			}
>> +
>> +			close(cr_service_client->sk_fd);
>> +			exit(resp->success ? 0 : 1);
>> +
>> +		default:
>> +			close(cr_service_client->sk_fd);
>> +		}
>> +	}
>> +
>>   	return 0;
>>   }
>> @@ -1,6 +1,22 @@
>>   #ifndef __CR_SERVICE_H__
>>   #define __CR_SERVICE_H__
>>   
>> -int cr_service(void);
>> +#include "protobuf/rpc.pb-c.h"
>> +
>> +#define CR_DEFAULT_SERVICE_ADDRESS "/tmp/criu_service.socket"
>> +#define MAX_MSG_SIZE 1024
>> +
>> +int cr_service(bool deamon_mode);
>> +
>> +int send_resp(int socket_fd, CriuDumpResp *resp);
>> +
>> +struct _cr_service_client {
>> +	int sk_ino;
>> +	int uid;
>> +	int pid;
>> +	int sk_fd;
>> +};
>> +
>> +struct _cr_service_client *cr_service_client;
> A variable cannot be declared in a header file -- linker would object
> its presence in several places.
>
>>   
>>   #endif
>>
>>