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

[Vladimir Davydov]

On 02/11/2014 12:17 PM, Ruslan Kuprieiev wrote:
> On 11.02.2014 12:11, Vladimir Davydov wrote:
>> 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?
>
> In theory, no one, except service daemon, can write to that log.(In
> theory, because we are not shutting down log for children, but it
> needs to be done soon).

Oh, if it's single-threaded and have no children sharing the log fd,
then your approach is safe.

I just want to say that I've never seen a daemon implementing its own
logrotate instead of using existing one, just to eliminate any
possibility of loosing a message. Even those daemons that need to run
non-stop (e.g. libvirtd) prefer to use logrotate with `copytruncate'...

> And as it rotate log in signal handler, service can't write to log,
> while signal handler is working.
>
>>>>> 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);
>