Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unix reopen command #3081

Closed
wants to merge 2 commits into from
Closed

Unix reopen command #3081

wants to merge 2 commits into from

Conversation

regit
Copy link
Contributor

@regit regit commented Dec 12, 2017

Add a unix socket command to trigger log files reopening.

Link to redmine ticket:

Describe changes:

  • Add the command
  • Documentation

PRScript output:

We did had a race condition with running logrotate with multiple
EVE Json files. Consequence was one of the file not being reopen
by suricata that did continue to write to the rotated one.

Trying fix on signal handler did fail so this patch implements
log rotation support by adding a dedicated command to unix socket
to reopen the log files.
@regit regit requested review from norg and a team as code owners December 12, 2017 19:40
@victorjulien
Copy link
Member

victorjulien commented Dec 13, 2017

@jasonish can you have a look at this?

Looks ok to me but I do wonder if there is any risk around synchronization/threading. E.g. if both unix-socket and signals code would act in parallel.

@pevma
Copy link
Member

pevma commented Dec 13, 2017

for info - this PR solves a problem of logrotating that i have observed on live traffic boxes that i have discussed with @jasonish a while back ago.

@jasonish
Copy link
Member

What about making the "flag" value volatile? Looking at this, I think it should have been made volatile from the start. It should not require synchronization then over what volatile already provides.

@inliniac
Copy link
Contributor

inliniac commented Dec 13, 2017 via email

@jasonish
Copy link
Member

In that case I prefer making it an atomic. Those are free except for 'sets', so should be a good fit.

Yup, even better.

But the list we're walking is static after init, right?

Correct.

@victorjulien
Copy link
Member

Added the atomic flag thing as a ticket: https://redmine.openinfosecfoundation.org/issues/2378

I don't think it should block this PR.

@victorjulien victorjulien mentioned this pull request Dec 19, 2017
@victorjulien
Copy link
Member

Merged through #3104, thanks Eric!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
5 participants