Skip to content

Fix --timeout to accept s/ms suffix for user convenience#472

Closed
a2Fsa2k wants to merge 1 commit into
schweikert:developfrom
a2Fsa2k:fix/timeout-s-suffix
Closed

Fix --timeout to accept s/ms suffix for user convenience#472
a2Fsa2k wants to merge 1 commit into
schweikert:developfrom
a2Fsa2k:fix/timeout-s-suffix

Conversation

@a2Fsa2k

@a2Fsa2k a2Fsa2k commented Jun 10, 2026

Copy link
Copy Markdown

Summary

The --timeout option previously rejected values with a trailing s or
ms suffix (e.g. --timeout 5s) because strtod_strict() validates
that no trailing characters remain after the number.

This change strips recognized time unit suffixes (s for seconds,
ms for milliseconds) from the argument before parsing, so users can
write more natural forms like --timeout 5s instead of --timeout 500.

Changes

  • Modified the -t / --timeout case handler in src/fping.c to
    detect and strip trailing s or ms suffix
  • The multiplier is adjusted accordingly: s suffix uses 1,000,000
    (seconds to microseconds), ms suffix uses 1,000 (milliseconds to
    microseconds)
  • Backward compatible: bare numeric values without suffix continue to
    work as before

Test plan

  • fping --timeout 5s localhost → alive
  • fping --timeout 500ms localhost → alive
  • fping --timeout 5 localhost → alive (backward compat)
  • fping --timeout 5x localhost → usage error (invalid suffix rejected)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request adds support for 'ms' and 's' suffixes to the timeout option (-t) in fping.c. However, a critical bug was identified where the timeout multipliers are off by a factor of 1000 because opt_timeout is internally represented in nanoseconds rather than microseconds. Correcting these multipliers is necessary to prevent premature timeouts on remote hosts.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread src/fping.c
The --timeout option previously rejected values with a trailing 's' or
'ms' suffix (e.g. --timeout 5s) because strtod_strict() validates that
no trailing characters remain. Strip recognized time unit suffixes
before parsing — opt_timeout is in nanoseconds, so seconds multiply by
1e9 and milliseconds by 1e6.
@a2Fsa2k a2Fsa2k force-pushed the fix/timeout-s-suffix branch from 1850b27 to 06d03c3 Compare June 10, 2026 12:00
@a2Fsa2k

a2Fsa2k commented Jun 10, 2026

Copy link
Copy Markdown
Author

Good catch — opt_timeout is in nanoseconds (added to current_time_ns at line 2016). Fixed the multipliers in the amended commit: seconds now multiply by 1e9, milliseconds by 1e6.

@gsnw-sebast gsnw-sebast linked an issue Jun 14, 2026 that may be closed by this pull request
@a2Fsa2k a2Fsa2k closed this Jun 14, 2026
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.

5s in fping --timeout 5s bernard invalid in 5.5

1 participant