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

[Vladimir Davydov]

On 02/11/2014 01:13 PM, Ruslan Kuprieiev wrote:
> 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?

No, leaving installation up to the user is not good.

You should make the criu service depend on (or recommend) the logrotate
package (if you do not build packages yet, mentioning this under an
appropriate section of README would be enough), add the logrotate rule
to the source tree, and make the Makefile install it to
/etc/logrotate.d, just like you install the systemd start-stop script.

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