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

Handle SIGHUP to cause the log to rotate/get a new one manually #494

Closed
wants to merge 3 commits into from

Conversation

andybarry
Copy link
Contributor

The zcm-logger help says that SIGHUP should cause the log to rotate, but that didn't work for me.

I have implemented that functionality here.

@jbendes
Copy link
Member

jbendes commented Oct 9, 2024

This looks great thanks for the contribution! What are your thoughts on behavior if args.rotate <= 0?

@andybarry
Copy link
Contributor Author

Good point. Perhaps we only support SIGHUP if rotate or increment is used? I'll implement that and add a print if SIGHUP is received and we're not in that configuration.

…ome more context to the --rotate / --increment error message.
@andybarry
Copy link
Contributor Author

I've updated the PR with a check that ignores SIGHUP if the user hasn't specified --increment or --rotate. I also was a bit confused about how to not have --increment specified, so I changed the error message there to add some more context, hopefully that makes sense to you.

tools/cpp/logger/main.cpp Outdated Show resolved Hide resolved
tools/cpp/logger/main.cpp Outdated Show resolved Hide resolved
@@ -683,6 +703,7 @@ int main(int argc, char *argv[])
signal(SIGINT, sighandler);
signal(SIGQUIT, sighandler);
signal(SIGTERM, sighandler);
signal(SIGHUP, sighup_handler);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can register this handler only in the cases you want to actually handle the signal. Specifically args.auto_increment || args.rotate > 0

@andybarry
Copy link
Contributor Author

All updated, note there is a slight behavior change in that if we don't register SIGHUP handler, the logger will quit when it receives a SIGHUP.

@jbendes
Copy link
Member

jbendes commented Oct 22, 2024

Ahh good point. Let's preserve the old behavior with the registering the signal. But you can just not set the variable in the callback unless it's relevant to

@jbendes
Copy link
Member

jbendes commented Oct 28, 2024

Addressed comments in #496

@jbendes jbendes closed this Oct 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants