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

[Pavel Emelyanov]

On 09/12/2013 02:54 PM, Ruslan Kuprieiev wrote:
> 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.

This function does ... what does it do? It sets things up for subsequent
dump, so let's call it setup_dump_from_req().

>>
>>> +{
>>> +	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.

You should call log_closedir() before doing it.

>>> +
>>> +	/* 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
>>>
>>>
> 
> .
>