[CRIU] [PATCH] service: SIGCHLD handler

[Andrew Vagin]

On Wed, Oct 30, 2013 at 03:35:12AM +0400, Ruslan Kuprieiev wrote:
> Hi!
> 
> Now we're ignoring SIGCHLD from forked children.
> This patch provides SIGCHLD handler, that will wait for child and write some
> information about it's exit status to logfile.
> 
> P.S. This is my first signal handler, so please take a good look at this patch.
> 
> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
> ---
>  cr-service.c | 68 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 65 insertions(+), 3 deletions(-)
> 
> diff --git a/cr-service.c b/cr-service.c
> index a79ebc7..f966036 100644
> --- a/cr-service.c
> +++ b/cr-service.c
> @@ -19,6 +19,8 @@
>  #include "pstree.h"
>  #include "cr-service.h"
>  
> +#define MAX_WORKERS 16
> +
>  unsigned int service_sk_ino = -1;
>  
>  static int recv_criu_msg(int socket_fd, CriuReq **msg)
> @@ -253,6 +255,46 @@ err:
>  	return -1;
>  }
>  
> +static void reap_worker(int signo)
> +{
> +	int saved_errno;
> +	int status;
> +	pid_t pid;
> +
> +	(void)signo;

for what?

> +
> +	saved_errno = errno;

I have never seen that someone save errno in a signal handler,
but it really has sense.

> +
> +	errno = 0;
for what?
> +	pid = waitpid(-1, &status, WNOHANG|WUNTRACED|WCONTINUED);

signal(7)
       1.  Multiple instances of real-time signals can be queued.  By
           contrast, if multiple instances of a standard signal are
           delivered while that signal is currently blocked, then only one
           instance is queued.

SIGCHLD is a standard signal, so sigchld handler can be executed once,
even if more than one worker completed.

> +
> +	if (pid <= 0) {
> +		errno = saved_errno;
> +		return;
> +	}
> +
> +	if (WIFEXITED(status))
> +		pr_info("Worker(pid %d) exited with %d\n",
> +			pid, WEXITSTATUS(status));
> +
> +	else if (WIFSIGNALED(status)) {
> +		pr_info("Worker(pid %d) ", pid);
> +#ifdef WCOREDUMP
> +		if (WCOREDUMP(status))
> +			pr_info("prodused a core dump and ");
> +#endif
> +		pr_info("was killed by %d\n", WTERMSIG(status));
> +
> +	} else if (WIFSTOPPED(status))
> +		pr_info("Worker(pid %d) was stopped by %d\n",
> +			pid, WSTOPSIG(status));

Do we get SIGCHLD if a task was stopped?

> +
> +	else if (WIFCONTINUED(status))
> +		pr_info("Worker(pid %d) was continued\n", pid);
> +
> +	errno = saved_errno;
> +}
> +
>  int cr_service(bool daemon_mode)
>  {
>  	int server_fd = -1;
> @@ -264,6 +306,9 @@ int cr_service(bool daemon_mode)
>  	socklen_t server_addr_len;
>  	socklen_t client_addr_len;
>  
> +	struct sigaction setup_action;
> +	sigset_t block_mask;
> +
>  	server_fd = socket(AF_LOCAL, SOCK_SEQPACKET, 0);
>  	if (server_fd == -1) {
>  		pr_perror("Can't initialize service socket.");
> @@ -299,7 +344,7 @@ int cr_service(bool daemon_mode)
>  		goto err;
>  	}
>  
> -	if (listen(server_fd, 16) == -1) {
> +	if (listen(server_fd, MAX_WORKERS) == -1) {
>  		pr_perror("Can't listen for socket connections.");
>  		goto err;
>  	}
> @@ -323,16 +368,31 @@ int cr_service(bool daemon_mode)
>  		}
>  	}
>  
> -	/* FIXME Do not ignore children's return values */
> -	signal(SIGCHLD, SIG_IGN);
> +	/* Set SIGCHLD handler */
> +	sigemptyset(&block_mask);
> +
> +	sigaddset(&block_mask, SIGCHLD);
> +
> +	/* FIXME May need to block some other signals too */

I think we don't need to block only SIGCHLD.

> +	sigaddset(&block_mask, SIGINT);
> +	sigaddset(&block_mask, SIGQUIT);
> +
> +	setup_action.sa_handler = reap_worker;
> +	setup_action.sa_mask	= block_mask;
> +	setup_action.sa_flags	= 0;

Pls, add SA_RESTART or you must handle EINTR for each syscall

> +
> +	sigaction(SIGCHLD, &setup_action, NULL);
>  
>  	while (1) {
>  		int sk;
>  
>  		pr_info("Waiting for connection...\n");
>  
> +		errno = 0;

It's not required.

>  		sk = accept(server_fd, &client_addr, &client_addr_len);
>  		if (sk == -1) {
> +			if (errno == EINTR)
> +				continue;
>  			pr_perror("Can't accept connection.");
>  			goto err;
>  		}
> @@ -340,6 +400,8 @@ int cr_service(bool daemon_mode)
>  		pr_info("Connected.\n");
>  		child_pid = fork();
>  		if (child_pid == 0) {
> +			/* Set default SIGCHLD handler back */
> +			signal(SIGCHLD, SIG_DFL);

warning: ISO C90 forbids mixed declarations and code [-Wdeclaration-after-statement]

>  			int ret;
>  
>  			close(server_fd);
> -- 
> 1.8.1.2
> 
> _______________________________________________
> CRIU mailing list
> CRIU at openvz.org
> https://lists.openvz.org/mailman/listinfo/criu