[CRIU] [PATCH] service: SIGCHLD handler

[Ruslan Kuprieiev]

On 30.10.2013 15:40, Andrew Vagin wrote:
> 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?

I thought compiler will swear on unused variable.
But it turned, it isn't true.

>
>> +
>> +	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?

I believe so. man 7 signal says:
            Signal     Value     Action   Comment
        SIGCHLD   20,17,18    Ign     Child stopped or terminated

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

If we will not block SIGCHLD, it may be received during treating the 
status by handler.
In v2 patch, still block SIGCHLD, but handler works until there is no 
children that have changed their status.

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

Thank you very much for your reply!