[CRIU] [PATCH] service: rotate log by SIGHUP

[Vladimir Davydov]

On 02/11/2014 11:49 AM, Ruslan Kuprieiev wrote:
> On 11.02.2014 11:28, Vladimir Davydov wrote:
>> On 02/11/2014 11:11 AM, Ruslan Kuprieiev wrote:
>>> Because we wan't service to work non-stop.
>>> And, as far as I know, by just using logrotate we will need to restart
>>> service.
>> Not necessarily, you can specify what should be done after rotate
>> (`postrotate'), e.g. send a signal to your service so that it could
>> reopen the logfile. However, I guess that `copytruncate' would be enough
>> in your case.
>
> But what if service will write something between the moment when
> logrotate will copy log and the moment when it will send signal to
> service?

Oh, if your logs are so valuable that loosing a message or two once a
day/week/month is a disaster, then logrotate is, of course, not an
option for you, sorry.

However, from what I see you have the same race in your implementation:
what if somebody prints to log while you're renaming it (after
log_fini(), but before log_init())? Errors will go to stderr, which
should be closed for a daemon?

>
>>> On 11.02.2014 10:59, Vladimir Davydov wrote:
>>>> Sorry for butting in, but why don't you use logrotate(8)?
>>>>
>>>> On 02/11/2014 10:41 AM, Ruslan Kuprieiev wrote:
>>>>> Many existing daemons rotate logs by SIGHUP, so lets do the same
>>>>> with our service.
>>>>>
>>>>> Signed-off-by: Ruslan Kuprieiev <kupruser at gmail.com>
>>>>> ---
>>>>>    cr-service.c | 30 ++++++++++++++++++++++++++----
>>>>>    1 file changed, 26 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/cr-service.c b/cr-service.c
>>>>> index 26dd3e2..49454e7 100644
>>>>> --- a/cr-service.c
>>>>> +++ b/cr-service.c
>>>>> @@ -489,12 +489,21 @@ static void reap_worker(int signo)
>>>>>        }
>>>>>    }
>>>>>    -static int setup_sigchld_handler()
>>>>> +static void rotate_log(int signo)
>>>>> +{
>>>>> +    pr_info("Rotating log.\n");
>>>>> +
>>>>> +    if (log_rotate())
>>>>> +        exit(1);
>>>>> +}
>>>>> +
>>>>> +static int setup_signal_handlers()
>>>>>    {
>>>>>        struct sigaction action;
>>>>>          sigemptyset(&action.sa_mask);
>>>>>        sigaddset(&action.sa_mask, SIGCHLD);
>>>>> +    sigaddset(&action.sa_mask, SIGHUP);
>>>>>        action.sa_handler    = reap_worker;
>>>>>        action.sa_flags        = SA_RESTART;
>>>>>    @@ -503,15 +512,23 @@ static int setup_sigchld_handler()
>>>>>            return -1;
>>>>>        }
>>>>>    +    action.sa_handler    = rotate_log;
>>>>> +
>>>>> +    if (sigaction(SIGHUP, &action, NULL)) {
>>>>> +        pr_perror("Can't setup SIGUSR1 handler");
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    -static int restore_sigchld_handler()
>>>>> +static int restore_signal_handlers()
>>>>>    {
>>>>>        struct sigaction action;
>>>>>          sigemptyset(&action.sa_mask);
>>>>>        sigaddset(&action.sa_mask, SIGCHLD);
>>>>> +    sigaddset(&action.sa_mask, SIGHUP);
>>>>>        action.sa_handler    = SIG_DFL;
>>>>>        action.sa_flags        = SA_RESTART;
>>>>>    @@ -520,6 +537,11 @@ static int restore_sigchld_handler()
>>>>>            return -1;
>>>>>        }
>>>>>    +    if (sigaction(SIGHUP, &action, NULL)) {
>>>>> +        pr_perror("Can't restore SIGHUP handler");
>>>>> +        return -1;
>>>>> +    }
>>>>> +
>>>>>        return 0;
>>>>>    }
>>>>>    @@ -596,7 +618,7 @@ int cr_service(bool daemon_mode)
>>>>>            }
>>>>>        }
>>>>>    -    if (setup_sigchld_handler())
>>>>> +    if (setup_signal_handlers())
>>>>>            goto err;
>>>>>          while (1) {
>>>>> @@ -615,7 +637,7 @@ int cr_service(bool daemon_mode)
>>>>>            if (child_pid == 0) {
>>>>>                int ret;
>>>>>    -            if (restore_sigchld_handler())
>>>>> +            if (restore_signal_handlers())
>>>>>                    exit(1);
>>>>>                  close(server_fd);
>