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

[Vladimir Davydov]

On 02/11/2014 12:56 PM, Ruslan Kuprieiev wrote:
> On 11.02.2014 12:37, Vladimir Davydov wrote:
>> 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'...
>
> Oh, now I see. Pardon me, I'm not experienced in existing daemons.

Never mind.

> Will redo, so service will just reopen log by SIGHUP.

I guess that in case with criu, you don't even have to handle any
signals - it's enough to add a good logrotate rule, something like this:

/var/log/criu.log {
    weekly
    rotate 4
    copytruncate
    notifempty
    missingok
}

`copytruncate' will make logrotate backup the content of the current log
file, and then truncate it so that you don't even need to reopen anything.

> Thanks=).
>
>>> 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);
>