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

[Ruslan Kuprieiev]

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.
Will redo, so service will just reopen log by SIGHUP.
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);