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

lcm-logger: New --disk-quota flag #529

Open
wants to merge 7 commits into
base: master
Choose a base branch
from
Open

Conversation

judfs
Copy link
Contributor

@judfs judfs commented Oct 9, 2024

Fix #464

      --disk-quota=SIZE      Minimum amount of free space the disk written to
                             must maintain. lcm-logger will exit if the quota is
                             exceeded. This is NOT an explicit limit on how much
                             lcm-logger is allowed to write.
                             Units accepted: B
                             1024: K,   M,   G,   T,   P,   E,
                                   Ki,  Mi,  Gi,  Ti,  Pi,  Ei,
                                   KiB, MiB, GiB, TiB, PiB, EiB
                             1000: KB, MB, GB, TB, PB, EB
                             e.g. "50G" or "0.05 TiB". 
                             Note: for `df`, -h is 1024 and -H is 1000 units.
                             Units are treated case insensitively. gb is treated
                             as GB (gigabytes) and not as gigabits.
                             NOT currently supported on Windows!

Currently put the check in the write thread and does not cause an immediate exit(1). Please review.

lcm-logger/lcm_logger.c Outdated Show resolved Hide resolved
Comment on lines 468 to 470
// @Review: How should this exit? For now going with a graceful exit.
// But maybe this case warrants more urgency.
g_main_loop_quit(_mainloop);
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the current approach is ideal. If the user sets up a condition for the logger to end then a non-zero exit code makes sense to me, since everything is operating as expected. But I don't feel strongly at all about it.

lcm-logger/lcm_logger.c Outdated Show resolved Hide resolved
lcm-logger/lcm_logger.c Outdated Show resolved Hide resolved
* Fix macos
* improve help
* Instant exit on known failed quota
@judfs judfs marked this pull request as ready for review October 21, 2024 21:03
Copy link
Contributor

@nosracd nosracd left a comment

Choose a reason for hiding this comment

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

Looks good to me. Just a couple notes:

  • There are still a few@Review in-code comments, but I believe I already replied to them or they appear to be notes to self. Can they be removed?
  • There's also a man pages document (lcm-logger/lcm-logger.1) that should be updated when the UX changes. This wouldn't be the first thing that caused those documents to go out of sync so I think it's fine to fix it either here or later in Update man pages #537.

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.

Feature request: lcm-logger disk quota option
2 participants