Modernize flock: CI, bug fixes, comprehensive test suite, and refactoring#43
Merged
josephholsten merged 5 commits intodiscoteq:masterfrom Apr 17, 2026
Merged
Conversation
Contributor
Author
|
Hi @josephholsten, I did a thing. Hope you find this helpful in modernizing |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a multi-platform GitHub Actions CI workflow, augments Makefile/README doc build behavior, expands the manpage OPTIONS, tightens argument parsing and usage handling in src/flock.c, and greatly expands/rewrites the Bats test suite for timing, concurrency, and new CLI behaviors. Changes
Sequence Diagram(s)sequenceDiagram
participant GH as GitHub Actions
participant Runner as Runner (ubuntu/macos/FreeBSD VM)
participant Checkout as Checkout
participant Tooling as Build Tools (autoconf/automake/bats/ronn)
participant Build as Build/Configure
participant Tests as Test Suite
participant Artifacts as Artifact Storage
GH->>Runner: trigger on push/pull_request to master
Runner->>Checkout: checkout repo
Runner->>Tooling: install needed tools/packages
Tooling-->>Runner: tools available
Runner->>Build: autoreconf && ./configure (matrix CC)
Build->>Build: make
Build->>Tests: make check
alt tests pass
Tests-->>Runner: exit 0
Runner->>GH: report success
else tests fail
Tests-->>Runner: exit non-zero
Runner->>Artifacts: upload test-suite.log and t/*.log
Runner->>GH: report failure
end
Runner->>Artifacts: optionally upload man pages (docs job)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
.github/workflows/ci.yml (1)
18-27: Keep oneronn-enabled path in CI.Every job currently exercises the “ronn missing” fallback, so a broken
man/flock.1.ronnor stale checked-in manpage can still merge green. One small docs leg that installsronnand runsmake docwould preserve the optional-build behavior without dropping coverage.Also applies to: 41-50, 64-74
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/ci.yml around lines 18 - 27, Add a CI job (or a matrix variant) that installs ronn and runs the documentation build so we exercise the ronn-enabled path; modify the existing "Install dependencies" step used by the job(s) that run autoreconf/configure/make to also install ronn (e.g., apt-get install ronn or gem install ronn) for one job, then add a "Build docs" step that runs make doc (or runs `make doc` after the "Build" step) to ensure man/flock.1.ronn is processed; reference the existing steps named "Install dependencies", "Bootstrap", "Configure", "Build" and add the new installation and "Build docs" step only to one CI variant so other jobs continue exercising the ronn-missing fallback.Makefile.am (1)
59-65: Don't satisfyman/flock.1with an empty placeholder.When
ronnis absent and no pre-built page exists,touch $@creates a zero-byte manual thatmake installcan ship as if it were real. Failing this branch, or explicitly skipping manpage install, is safer than silently publishing blank docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Makefile.am` around lines 59 - 65, The current Makefile.am rule that tries to build man/flock.1 uses touch $@ when ronn is missing, producing an empty man page; replace the touch fallback with a failing exit so builds don't install blank docs: inside the recipe that references $(RONN), $(RONN_FLAGS), and man/flock.1.ronn, change the branches that currently do "touch $@" to instead print a clear error to stderr (mention ronn missing and man/flock.1 not available) and exit with non‑zero (e.g., echo "Error: ronn not found and no pre-built man/flock.1" >&2; exit 1) so the build fails rather than creating an empty $@. Ensure both occurrences of touch $@ in that conditional are updated.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 23-25: Update the macOS MacPorts section of README.md to either
prepend sudo to the installation command so it reads "sudo port install flock"
or add a short note explaining why sudo is not required; modify the MacPorts
block where "port install flock" appears to use the sudo-prefixed command (or
add the clarification text) to match the Homebrew example's behavior and avoid
permission surprises for users.
In `@src/flock.c`:
- Around line 214-219: The parsing code using strtol in the option handlers
(e.g., the case 'E' branch that sets status_time_conflict and the
file-descriptor parser at lines ~294-298) must reject empty/non-numeric inputs
and detect overflow: ensure endptr != optarg to confirm at least one digit was
parsed, check errno for ERANGE after strtol, and for values stored into int
verify the long is within INT_MIN..INT_MAX before narrowing; for the exit-code
path also enforce the 0..255 range after these checks. Update both the 'E'
handler and the FD parsing code to perform these validations and return the
existing errx(EX_USAGE, ...) on failure.
In `@t/default.bats`:
- Around line 350-354: The test "-c must come after lockfile" currently uses a
vacuous assertion `[ "$status" -ne 0 ] || true` which always succeeds; change it
to assert failure properly by replacing that line with an explicit non-zero
check such as `[ "$status" -ne 0 ]` (or an if that calls fail when `"$status"
-eq 0`) so the invocation `run ${FLOCK} -c "echo 1" ${LOCKFILE}` actually must
fail to pass the test; reference `${FLOCK}`, `${LOCKFILE}`, and the test name
"-c must come after lockfile" to locate and update the assertion.
---
Nitpick comments:
In @.github/workflows/ci.yml:
- Around line 18-27: Add a CI job (or a matrix variant) that installs ronn and
runs the documentation build so we exercise the ronn-enabled path; modify the
existing "Install dependencies" step used by the job(s) that run
autoreconf/configure/make to also install ronn (e.g., apt-get install ronn or
gem install ronn) for one job, then add a "Build docs" step that runs make doc
(or runs `make doc` after the "Build" step) to ensure man/flock.1.ronn is
processed; reference the existing steps named "Install dependencies",
"Bootstrap", "Configure", "Build" and add the new installation and "Build docs"
step only to one CI variant so other jobs continue exercising the ronn-missing
fallback.
In `@Makefile.am`:
- Around line 59-65: The current Makefile.am rule that tries to build
man/flock.1 uses touch $@ when ronn is missing, producing an empty man page;
replace the touch fallback with a failing exit so builds don't install blank
docs: inside the recipe that references $(RONN), $(RONN_FLAGS), and
man/flock.1.ronn, change the branches that currently do "touch $@" to instead
print a clear error to stderr (mention ronn missing and man/flock.1 not
available) and exit with non‑zero (e.g., echo "Error: ronn not found and no
pre-built man/flock.1" >&2; exit 1) so the build fails rather than creating an
empty $@. Ensure both occurrences of touch $@ in that conditional are updated.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3ce9bb1f-21ba-4930-aae6-7623c51c9c85
📒 Files selected for processing (7)
.github/workflows/ci.ymlMakefile.amREADME.mdman/flock.1.ronnsrc/flock.ct/default.batst/smartos.bats
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+59
to
+66
| @if command -v $(RONN) >/dev/null 2>&1; then \ | ||
| $(RONN) $(RONN_FLAGS) -r man/flock.1.ronn; \ | ||
| elif [ -f $@ ]; then \ | ||
| touch $@; \ | ||
| else \ | ||
| echo "Warning: ronn not found and no pre-built man page available" >&2; \ | ||
| touch $@; \ | ||
| fi |
There was a problem hiding this comment.
2. Empty man page installed 🐞 Bug ✓ Correctness
man/flock.1 is created via touch when ronn is missing and no prebuilt man page exists, producing an empty man page. Because man_MANS installs this file, users can end up with a blank manual silently.
Agent Prompt
## Issue description
The build currently creates an empty `man/flock.1` (via `touch`) when `ronn` is unavailable and there is no prebuilt man page. Since `man_MANS` installs `man/flock.1`, this can silently install a blank manual.
## Issue Context
Goal is to make `ronn` optional without shipping broken docs.
## Fix Focus Areas
- Makefile.am[57-66]
## Suggested approach
- Prefer a configure-time check (e.g., `AC_PATH_PROG([RONN], [ronn], [no])` + `AM_CONDITIONAL([HAVE_RONN], ...)`) and only set `man_MANS` / `doc` targets when `ronn` is available OR a real prebuilt `man/flock.1` is present.
- If you must keep a fallback, generate a minimal *non-empty* stub man page that clearly states documentation wasn’t built (instead of `touch`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Contributor
|
hey there, it's been a while since I've worked on this so I've recruited some automatic code review. I'm only concerned about the strtol() check and the |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
src/flock.c (1)
215-220:⚠️ Potential issue | 🟠 MajorEmpty arguments still slip through both
strtolparsers.Because Lines 219 and 299 only reject trailing characters,
strtol("", &endptr, 10)still gets accepted here:-E ''becomes conflict code0, andflock ''is treated as fd0. Please add the explicit “no digits parsed” check in both branches and add a regression for the empty-string case.🐛 Minimal hardening
- if (errno != 0 || *endptr != '\0' || val < 0 || val > 255) + if (endptr == optarg || errno != 0 || *endptr != '\0' || val < 0 || val > 255) errx(EX_USAGE, "conflict exit code must be 0-255, was '%s'", optarg); ... - if (errno != 0 || *endptr != '\0' || fdval < 0 || fdval > INT_MAX) + if (endptr == argv[optind] || errno != 0 || *endptr != '\0' || fdval < 0 || fdval > INT_MAX) errx(EX_USAGE, "bad file descriptor: %s", argv[optind]);#!/bin/bash # Show both numeric guards and confirm the explicit no-digits checks are still absent. sed -n '215,221p;296,301p' src/flock.c printf '\nExplicit no-digits guards:\n' rg -n 'endptr\s*==\s*(optarg|argv\[optind\])' src/flock.c || trueAlso applies to: 296-300
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flock.c` around lines 215 - 220, The strtol checks in the '-E' handling (the case 'E' block using strtol with variables optarg and endptr) and the similar fd-parsing branch must reject empty strings by verifying that strtol actually parsed digits; add an explicit check that endptr != optarg (or endptr == optarg -> error) after calling strtol and before using val, and keep the existing errno/trailing-char bounds checks; mirror the same "no digits parsed" check in the other branch that parses argv[optind] (the second strtol usage around lines ~296-300); finally add a regression test that calls the binary with '-E ""' and with an empty fd argument to assert both return the usage/error path.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@t/default.bats`:
- Around line 557-564: The test "-o closes fd before exec so grandchildren don't
inherit lock" currently only runs a short command and never creates a long-lived
grandchild; change it so the command run under ${FLOCK} -o ${LOCKFILE} spawns a
long-lived grandchild (e.g., background sleep or tail) and then exits, and in
the test body immediately attempt to reacquire the same lock with a non-blocking
${FLOCK} -n against ${LOCKFILE} to assert it succeeds while the grandchild is
still alive; this ensures the fd was closed in the exec'd child and not
inherited by the grandchild.
- Around line 571-573: The test "--verbose produces output about lock
acquisition" currently uses a broad glob matching "flock:" which can hide
regressions; update the assertion to explicitly check for the acquisition
message and the timing line instead of just "flock:". Replace the single
assertion ([[ "$result" == *"flock:"* ]]) with two more specific checks that
"$result" contains the acquisition string (e.g., "flock: executing" or the exact
acquisition wording used) and the timing string (e.g., "took" or "took .*
microseconds") — using two [[ ... == *...* ]] checks or grep/egrep with both
patterns so the test fails if either piece is missing.
- Around line 491-502: The test "lock then unlock on file descriptor" currently
can't prove the fd was unlocked before the holder exited; update the test to
either (A) call get_elapsed using a nonblocking lock attempt (pass -n to
${FLOCK}) so the second lock must succeed while the background holder is still
alive and assert it succeeded, referencing ${FLOCK}, ${LOCKFILE} and
get_elapsed, or (B) keep the blocking get_elapsed but tighten the assertion in
was_blocked to include an upper bound (e.g. assert elapsed < 1.5s) so the
measured wait is bounded and cannot be satisfied by waiting until the background
process exits; apply the change inside the test body for "lock then unlock on
file descriptor".
- Around line 31-36: The helper hold_lock() currently uses a fixed sleep which
is racy; replace the sleep-based wait with an explicit readiness check by
polling with a non-blocking probe so you only return once the background holder
truly holds the lock. Concretely: after spawning the background ${FLOCK}
${flags} ${LOCKFILE} sleep 2 &, loop using a non-blocking probe (e.g., attempt
${FLOCK} -n ${LOCKFILE} or equivalent and treat a nonzero/failed acquire as
“lock held”) with a short backoff until the probe indicates contention, then
exit the helper; also update any inline holder snippets that use sleep 0.2 to
the same polling/readiness approach for deterministic tests.
---
Duplicate comments:
In `@src/flock.c`:
- Around line 215-220: The strtol checks in the '-E' handling (the case 'E'
block using strtol with variables optarg and endptr) and the similar fd-parsing
branch must reject empty strings by verifying that strtol actually parsed
digits; add an explicit check that endptr != optarg (or endptr == optarg ->
error) after calling strtol and before using val, and keep the existing
errno/trailing-char bounds checks; mirror the same "no digits parsed" check in
the other branch that parses argv[optind] (the second strtol usage around lines
~296-300); finally add a regression test that calls the binary with '-E ""' and
with an empty fd argument to assert both return the usage/error path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3fdb2602-5ce8-43c2-88e4-0b453ccbc8be
📒 Files selected for processing (2)
src/flock.ct/default.bats
voltechs
added a commit
to TwilightCoders/flock
that referenced
this pull request
Mar 29, 2026
Add endptr == input check to catch empty string input (e.g., -E "" or flock ""), which strtol silently accepts as 0. Add regression tests. Addresses coderabbitai review feedback on PR discoteq#43.
voltechs
added a commit
to TwilightCoders/flock
that referenced
this pull request
Mar 29, 2026
- Check in man/flock.1 so users always get a real man page, even when ronn is not installed. Previously, builds without ronn created an empty man page via touch, which installed a blank manual. - Remove man/flock.1 from .gitignore. - Make 'make doc' fail with a clear error when ronn is absent instead of silently producing incomplete output. - Make 'html-man' fail clearly if man/flock.1.html was not built. Addresses qodo-code-review feedback on PR discoteq#43.
- Make -h/--help print to stdout and exit 0 instead of stderr/EX_USAGE (fixes discoteq#15) - Fix printf format specifier for suseconds_t: %1u -> %ld to resolve GCC -Werror=format build failure (fixes discoteq#33) - Fix verbose timing: include tv_sec in elapsed calculation to avoid underflow across second boundaries; terminate "getting lock" message with newline so output isn't left dangling on timeout/error - Fix typo in error message: "waidpid" -> "waitpid" - Validate -E exit code with strtol: reject non-numeric, empty, negative, >255, and overflow input (was atoi, silently accepted garbage as 0) - Validate fd argument with strtol: reject non-numeric, empty, negative, and overflow input (previously 'flock abc' silently locked stdin) - Use direct assignment for fl.l_type in fcntl fallback (|= on zero was misleading) - Normalize indentation to tabs throughout; fix double-tab err() calls; remove extraneous blank lines
Replace the original ~30 timing-sensitive tests with 86 comprehensive tests. Key improvements: - Poll-based wait_for_lock/hold_lock helpers that probe with flock -n until the lock is actually held, replacing fixed sleep delays - Threshold-based was_blocked/was_immediate helpers replacing exact timing comparisons like [ "$result" = 0.05 ] - Per-test setup/teardown with isolated lockfiles and background process cleanup New coverage includes: -e flag, --nb alias, -c shell features (pipes, variables, command substitution), child exit status propagation (0, 1, 42, 255, nonexistent command), -E validation (non-numeric, empty, negative, >255, overflow), fd validation (non-numeric, empty, negative, overflow), -o/--close grandchild fd inheritance, --verbose output assertions, option combinations and flag precedence (-s -n, -x -n, -w -E, -n -E 0), file types (spaces in path, nonexistent directory), and -h/--help exit status. Fixes discoteq#5. Addresses discoteq#42.
…tions - Add MacPorts install instructions to README (fixes discoteq#32) - Document bats-core as a test suite prerequisite in README - Document -c/--command, -h/--help, -V/--version, and --verbose in the man page (these options existed in code but were not documented)
CI test matrix (fixes discoteq#17): - Ubuntu latest with GCC and Clang - macOS latest - FreeBSD 14 via vmactions/freebsd-vm - Dedicated build-docs job that installs ronn-ng and uploads man pages as artifacts (nroff and HTML) - Test log artifacts uploaded on failure Build changes: - Man page build gracefully skips when ronn is not installed: uses pre-existing file if available, warns otherwise (fixes discoteq#41) - make doc and html-man fail with clear errors when ronn is absent instead of silently producing incomplete output
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
t/default.bats (2)
167-174: Minor timing dependency in shared lock test.The
sleep 0.2is used becausewait_for_lock(which uses exclusive nonblock probe) can't detect shared locks. This is a minor race condition, but acceptable given that shared lock acquisition is fast. Consider documenting this limitation in a comment if flakiness is observed on slow CI.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/default.bats` around lines 167 - 174, The test "multiple shared locks can coexist" relies on a small sleep (sleep 0.2) because wait_for_lock (which uses an exclusive nonblock probe) cannot detect already-held shared locks, introducing a minor timing dependency; update the test in the multiple shared locks can coexist block to add a clear comment explaining this limitation and why sleep 0.2 is required (mentioning wait_for_lock, the exclusive nonblock probe, and that shared locks are fast but may race on slow CI), and optionally note that get_elapsed and was_immediate are used to confirm immediate acquisition so future maintainers won't remove or change the sleep without accounting for the probe limitation.
615-619: Consider adding--verbosetest for FD mode.This test only covers
--verbosein command mode. Adding a test for FD mode would catch the uninitializedt_l_reqbug inflock.c:`@test` "--verbose in fd mode produces output" { ( ${FLOCK} --verbose 8 ) 8> ${LOCKFILE} 2>&1 | grep -q "getting lock" }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/default.bats` around lines 615 - 619, Add a new bats test that exercises --verbose in FD mode to catch the t_l_req uninitialized bug: create a test named "--verbose in fd mode produces output" that runs the flock binary with "--verbose 8" while redirecting FD 8 to the lock file (e.g. run ( ${FLOCK} --verbose 8 ) 8> ${LOCKFILE} 2>&1 | grep -q "getting lock") so the test asserts verbose output is produced in FD mode similarly to the existing command-mode verbose test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flock.c`:
- Around line 332-337: The verbose timing uses t_l_req even when it was only set
in command mode; fix by ensuring t_l_req is initialized for FD mode too—either
move the initial gettimeofday(&t_l_req, ...) earlier (before the mode dispatch)
so both code paths set t_l_req, or add a separate gettimeofday(&t_l_req, NULL)
in the FD-mode branch before waiting for the lock; alternatively guard the
printf behind a flag that confirms t_l_req was set. Update references to
t_l_req/t_l_acq and the verbose block so elapsed_us is computed only after
t_l_req has been initialized.
---
Nitpick comments:
In `@t/default.bats`:
- Around line 167-174: The test "multiple shared locks can coexist" relies on a
small sleep (sleep 0.2) because wait_for_lock (which uses an exclusive nonblock
probe) cannot detect already-held shared locks, introducing a minor timing
dependency; update the test in the multiple shared locks can coexist block to
add a clear comment explaining this limitation and why sleep 0.2 is required
(mentioning wait_for_lock, the exclusive nonblock probe, and that shared locks
are fast but may race on slow CI), and optionally note that get_elapsed and
was_immediate are used to confirm immediate acquisition so future maintainers
won't remove or change the sleep without accounting for the probe limitation.
- Around line 615-619: Add a new bats test that exercises --verbose in FD mode
to catch the t_l_req uninitialized bug: create a test named "--verbose in fd
mode produces output" that runs the flock binary with "--verbose 8" while
redirecting FD 8 to the lock file (e.g. run ( ${FLOCK} --verbose 8 ) 8>
${LOCKFILE} 2>&1 | grep -q "getting lock") so the test asserts verbose output is
produced in FD mode similarly to the existing command-mode verbose test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f7ccfbf0-9587-44eb-885c-ce52b8a4c34e
📒 Files selected for processing (7)
.github/workflows/ci.ymlMakefile.amREADME.mdman/flock.1.ronnsrc/flock.ct/default.batst/smartos.bats
✅ Files skipped from review due to trivial changes (2)
- README.md
- .github/workflows/ci.yml
🚧 Files skipped from review as they are similar to previous changes (3)
- t/smartos.bats
- man/flock.1.ronn
- Makefile.am
Move gettimeofday(&t_l_req) from command-mode-only block to just before the flock() loop, so it covers both command and FD paths. Previously, FD mode with --verbose read uninitialized stack memory, producing garbage like "took 1774858164251193 microseconds". Add regression test verifying verbose FD mode timing is sane (<1s).
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flock.c (1)
207-213:⚠️ Potential issue | 🟠 MajorHarden
-w/--timeoutparsing before populatingtimer.it_value.Lines 207-213 still use
strtod(optarg, NULL), so inputs like1junk,nan, andinfare accepted, and a tiny positive like0.0000001truncates to{0,0}, which disables the timer entirely. Validate full consumption,ERANGE, and finiteness first, then reject or round up sub-microsecond positives before callingsetitimer().Suggested hardening
+#include <math.h> ... - case 'w': + case 'w': { + char *endptr; have_timeout = true; - raw_timeval = strtod(optarg, NULL); - if (0 >= raw_timeval) - errx(EX_USAGE, "timeout must be greater than 0, was %f", raw_timeval); + errno = 0; + raw_timeval = strtod(optarg, &endptr); + if (errno == ERANGE || endptr == optarg || *endptr != '\0' || + !isfinite(raw_timeval) || raw_timeval <= 0) + errx(EX_USAGE, "timeout must be greater than 0, was '%s'", optarg); timer.it_value.tv_sec = (time_t) raw_timeval; timer.it_value.tv_usec = (suseconds_t) ((raw_timeval - timer.it_value.tv_sec) * 1000000); + if (timer.it_value.tv_sec == 0 && timer.it_value.tv_usec == 0) + timer.it_value.tv_usec = 1; break; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/flock.c` around lines 207 - 213, The timeout parsing in the case 'w' branch currently uses raw_timeval = strtod(optarg, NULL) and then assigns timer.it_value directly; change this to use strtod with an endptr and check errno for ERANGE, verify endptr consumed the entire optarg (no trailing characters), and use isfinite() to reject nan/inf; also enforce raw_timeval > 0 and if a positive value would round to {0,0} (sub-microsecond) set timer.it_value.tv_usec to 1 to avoid disabling the timer; keep the existing variables have_timeout, raw_timeval, and timer.it_value but perform these validations and rounding before populating timer.it_value and calling setitimer().
♻️ Duplicate comments (1)
t/default.bats (1)
232-236:⚠️ Potential issue | 🟠 MajorWait for contention in the timeout-success tests.
Lines 232-236, 251-255, and 258-262 still use
sleep 0.1as a proxy for lock acquisition. On a slow runner, the secondflockcan start before the holder owns${LOCKFILE}, so these cases pass without ever exercising-w/--timeout. Reusewait_for_lockhere instead of a fixed sleep.✅ Deterministic replacement
`@test` "-w succeeds if lock is released before timeout" { ${FLOCK} ${LOCKFILE} sleep 0.3 & - sleep 0.1 + wait_for_lock result=$(${FLOCK} -w 3 ${LOCKFILE} echo run || echo err) [ "$result" = run ] } @@ `@test` "-w with fractional seconds works" { ${FLOCK} ${LOCKFILE} sleep 0.2 & - sleep 0.1 + wait_for_lock result=$(${FLOCK} -w 1.5 ${LOCKFILE} echo run || echo err) [ "$result" = run ] } @@ `@test` "--timeout succeeds if lock is released before timeout" { ${FLOCK} ${LOCKFILE} sleep 0.3 & - sleep 0.1 + wait_for_lock result=$(${FLOCK} --timeout 3 ${LOCKFILE} echo run || echo err) [ "$result" = run ] }Also applies to: 251-255, 258-262
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@t/default.bats` around lines 232 - 236, The timeout-success tests (e.g. the `@test` "-w succeeds if lock is released before timeout" that uses ${FLOCK} and ${LOCKFILE}) rely on a fixed sleep to let the holder acquire the lock; replace the sleep 0.1 call with the existing wait_for_lock helper to wait deterministically for the holder to own ${LOCKFILE} before starting the second ${FLOCK} invocation, and apply the same replacement in the other two test blocks around the same logic (the blocks at the other occurrences) so the -w/--timeout behavior is actually exercised.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/flock.c`:
- Around line 121-122: Update the usage() help text to document the missing
flags: add a short description for -E (the same as --conflict-exit-code) and
include -v (the same as --verbose) so the printed help matches the parser;
modify the usage() string where the flags are listed to show "-E
--conflict-exit-code Exit with non-zero code on conflict" (or equivalent) and
"-v --verbose Increase verbosity" so the option names (E and v) and their
long forms match the parser behavior in usage().
---
Outside diff comments:
In `@src/flock.c`:
- Around line 207-213: The timeout parsing in the case 'w' branch currently uses
raw_timeval = strtod(optarg, NULL) and then assigns timer.it_value directly;
change this to use strtod with an endptr and check errno for ERANGE, verify
endptr consumed the entire optarg (no trailing characters), and use isfinite()
to reject nan/inf; also enforce raw_timeval > 0 and if a positive value would
round to {0,0} (sub-microsecond) set timer.it_value.tv_usec to 1 to avoid
disabling the timer; keep the existing variables have_timeout, raw_timeval, and
timer.it_value but perform these validations and rounding before populating
timer.it_value and calling setitimer().
---
Duplicate comments:
In `@t/default.bats`:
- Around line 232-236: The timeout-success tests (e.g. the `@test` "-w succeeds if
lock is released before timeout" that uses ${FLOCK} and ${LOCKFILE}) rely on a
fixed sleep to let the holder acquire the lock; replace the sleep 0.1 call with
the existing wait_for_lock helper to wait deterministically for the holder to
own ${LOCKFILE} before starting the second ${FLOCK} invocation, and apply the
same replacement in the other two test blocks around the same logic (the blocks
at the other occurrences) so the -w/--timeout behavior is actually exercised.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5f5d5057-e5fb-4e77-b3e5-53848694f250
📒 Files selected for processing (2)
src/flock.ct/default.bats
Comment on lines
121
to
122
| -E --conflict-exit-code\n\ | ||
| --verbose Increase verbosity\n" |
There was a problem hiding this comment.
Document -E and -v correctly in usage().
Lines 121-122 currently give -E no description, and -v is accepted by the parser but never shown in the help text. That leaves usage() out of sync with the actual CLI.
📝 Suggested help-text fix
- -E --conflict-exit-code\n\
- --verbose Increase verbosity\n"
+ -E --conflict-exit-code Set exit code used for conflicts/timeouts\n\
+ -v --verbose Increase verbosity\n"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| -E --conflict-exit-code\n\ | |
| --verbose Increase verbosity\n" | |
| -E --conflict-exit-code Set exit code used for conflicts/timeouts\n\ | |
| -v --verbose Increase verbosity\n" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/flock.c` around lines 121 - 122, Update the usage() help text to document
the missing flags: add a short description for -E (the same as
--conflict-exit-code) and include -v (the same as --verbose) so the printed help
matches the parser; modify the usage() string where the flags are listed to show
"-E --conflict-exit-code Exit with non-zero code on conflict" (or equivalent)
and "-v --verbose Increase verbosity" so the option names (E and v) and their
long forms match the parser behavior in usage().
Contributor
Author
|
Hey @josephholsten, thanks for the engagement. Sorry it took me so long to circle back to this. I worked through all of the code rabbits comments best I could, but it started to become whackamole. I'd fix everything on its list and the it'd find something else to complain about. I think it did ultimately give it a pass. Let me know how you're feeling about this. I rebased onto master after your couple merges earlier, which addressed a few things the PR did as well. |
Contributor
|
Sorry about the delay in merging, you got the relevant stuff absolutely. It's trying to critique larger issues in the codebase, and those don't make sense in this changeset. You're awesome, thank you! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
make && make installworks without extra dependenciesBug fixes
-h/--helpnow exits 0 (was EX_USAGE/64) — fixes Add -h/--help option #15%1uformat specifier forsuseconds_tcausing GCC-Werrorbuild failure — fixes Compilation error with GCC #33 (supersedes flock.c: printf: ensure format & param types match #38)tv_usec, producing wrong/negative values when lock acquisition crossed a second boundary-Econflict exit code (wasatoi, now rejects non-numeric and out-of-range values)flock abcsilently locked stdin)Already fixed (can be closed)
-xis already supported as a no-op (default is exclusive) — Should support -x as a no-op like util-linux does #9-calready wraps command with$SHELL -cas expected — The -c option behaves differently to Linux #10Test suite
[ "$result" = 0.05 ]) with threshold-based helpers (was_blocked/was_immediate) — fixes convert tests to not be time sensitive #5, addressesmake checkfailures (and what they mean) #42setup/teardownwith per-test lockfiles and background process cleanup-eflag,--nbalias,-cshell features (pipes, variables, subst), child exit propagation,-Ewith-n/-w,-o/--close,--verbose, option combinations and flag precedence, file with spaces, nonexistent pathsCI & build
ubuntu-latest(gcc, clang),macos-latest, FreeBSD 15 viavmactions/freebsd-vmronnis absent) — fixes Unstated reliance on Bash Automated Testing System (bats) #41Documentation
-c,-h,-V, and--verboseoptions in man pageCode quality
flock.c|=forfl.l_typeassignment in fcntl fallbackSummary by CodeRabbit
New Features
-c/--command,-h/--help, and-V/--versionoptions; improved verbose timing outputBug Fixes
Documentation
Tests