[CRIU] [PATCH] service: rotate log by SIGHUP
Ruslan Kuprieiev
kupruser at gmail.com
Tue Feb 11 01:43:17 PST 2014
On 11.02.2014 13:39, Vladimir Davydov wrote:
> 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.
Ok, will do.
Thanks! =)
>>>> 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);
More information about the CRIU
mailing list