[CRIU] [PATCH] systemd socket activation support

[Pavel Emelyanov]

On 12/11/2013 07:34 AM, Shawn Landden wrote:
> Makes the criu RPC socket socket-activated with
> systemd [1], meaning that systemd will create and listen to
> the UNIX socket /var/run/criu-srvice.socket
> on behalf of criu until a connection comes in, when it will
> then pass control of the socket, along with the first connection
> over to a newly spawned criu daemon.
> 
> This is similar to inetd, but criu stays around after getting
> started, listening itsself on the socket.

Shawn, thank you for the contribution, it's really appreciated.
I have several comments inline.

> [1] http://0pointer.de/blog/projects/socket-activation.html
> 
> Signed-off-by: Shawn Landden <shawn at churchofgit.com>
> ---
>  Makefile         |   3 +
>  Makefile.crtools |   1 +
>  Makefile.inc     |   1 +
>  cr-service.c     |  73 ++++----
>  criu.service     |   5 +
>  criu.socket      |   8 +
>  sd-daemon.c      | 521 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sd-daemon.h      | 282 ++++++++++++++++++++++++++++++
>  8 files changed, 862 insertions(+), 32 deletions(-)
>  create mode 100644 criu.service
>  create mode 100644 criu.socket
>  create mode 100644 sd-daemon.c
>  create mode 100644 sd-daemon.h

> diff --git a/cr-service.c b/cr-service.c
> index a82ef9e..3b9cb02 100644
> --- a/cr-service.c
> +++ b/cr-service.c
> @@ -332,53 +333,61 @@ static int restore_sigchld_handler()
>  
>  int cr_service(bool daemon_mode)
>  {
> -	int server_fd = -1;
> +	int server_fd = -1, n;
>  	int child_pid;
>  
> -	struct sockaddr_un server_addr;
>  	struct sockaddr_un client_addr;
> -
> -	socklen_t server_addr_len;
>  	socklen_t client_addr_len;
>  
> -	server_fd = socket(AF_LOCAL, SOCK_SEQPACKET, 0);
> -	if (server_fd == -1) {
> -		pr_perror("Can't initialize service socket.");
> +	n = sd_listen_fds(0);
> +	if (n > 1) {
> +		pr_perror("Too many file descriptors (%d) recieved.", n);
>  		goto err;
> -	}
> +	} else if (n == 1)
> +		server_fd = SD_LISTEN_FDS_START + 0;
> +	else { 

When sd_listen_fds reports negative value, which means some error has
occurred, we get on this branch and start listening on our own socket.
Is that expected behavior?

> +		struct sockaddr_un server_addr;
> +		socklen_t server_addr_len;
> +
> +		server_fd = socket(AF_LOCAL, SOCK_SEQPACKET, 0);
> +		if (server_fd == -1) {
> +			pr_perror("Can't initialize service socket.");
> +			goto err;
> +		}
>  


> diff --git a/criu.service b/criu.service
> new file mode 100644
> index 0000000..803ce46
> --- /dev/null
> +++ b/criu.service
> @@ -0,0 +1,5 @@
> +[Unit]
> +Description=Checkpoint Restore in Userspace daemon
> +
> +[Service]
> +ExecStart=/usr/sbin/criu service
> diff --git a/criu.socket b/criu.socket
> new file mode 100644
> index 0000000..7ccf7b4
> --- /dev/null
> +++ b/criu.socket
> @@ -0,0 +1,8 @@
> +[Unit]
> +Description=Checkpoint Restore in Userspace socket
> +
> +[Socket]
> +ListenSequentialPacket=/var/run/criu-service.socket
> +
> +[Install]
> +WantedBy=sockets.target

Can you put these two files into e.g. scripts/sd/ subdirectory?

> diff --git a/sd-daemon.c b/sd-daemon.c
> new file mode 100644
> index 0000000..2bf506e
> --- /dev/null
> +++ b/sd-daemon.c
> @@ -0,0 +1,521 @@


I've noticed that quite a lot of code below (and in sd_daemon.h file) is
unused. Effectively only the sd_listen_fds() is called. Is there a reason 
for keeping the rest? Can you drop unused parts from the patch otherwise?

Thanks,
Pavel