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

[Ruslan Kuprieiev]

On 11.02.2014 13:07, Vladimir Davydov wrote:
> 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.

Oh, cool. So I should just add this recommendation to service topic in 
man-page?

>> 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);