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

Fix for GitHub issue #377 #378

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

Conversation

auerswal
Copy link
Collaborator

@auerswal auerswal commented Feb 1, 2025

This pull request has two commits:

  1. Additional tests for useful features of the current code that reads target names from file (or standard input).
  2. A fix for the reported issue, i.e., splitting long lines, and not supporting long, but possibly valid, DNS names.

@coveralls
Copy link

coveralls commented Feb 1, 2025

Coverage Status

coverage: 87.698% (-0.06%) from 87.758%
when pulling 266690e on auerswal:issue377
into 57d91b1 on schweikert:develop.

@auerswal auerswal linked an issue Feb 1, 2025 that may be closed by this pull request
@auerswal auerswal marked this pull request as draft February 6, 2025 09:18
@auerswal
Copy link
Collaborator Author

auerswal commented Feb 6, 2025

There are at least two bugs still lurking in the new code. I plan to add the test cases below, and fix the behavior. For now, I have converted the pull request to a "draft".

When the first part of the line read into the buffer ends with the target name, and the first character of the second part of the line is a '#', then the data from the second line part is ignored instead of added to the target name.

Observed behavior:

$ perl -e 'for (my $i = 1; $i < 4; $i++) { print " "x(255-11+$i)."127.0.0.$i#host.name.invalid\n" }' | ./src/fping 
127.0.0.1#host.name.invalid: Name or service not known
127.0.0.3#host.name.invalid: Name or service not known
127.0.0.2 is alive

Expected behavior:

$ perl -e 'for (my $i = 1; $i < 4; $i++) { print " "x(255-11+$i)."127.0.0.$i#host.name.invalid\n" }' | ./src/fping 
127.0.0.1#host.name.invalid: Name or service not known
127.0.0.2#host.name.invalid: Name or service not known
127.0.0.3#host.name.invalid: Name or service not known

Then there is the problem that a comment in the first part of the line still results in splitting long lines.

Observed behavior:

$ perl -e 'print "#"." "x300 ."localhost\n"' | ./src/fping 
localhost is alive

Expected behavior:

$ perl -e 'print "#"." "x300 ."localhost\n"' | ./src/fping 

@auerswal auerswal marked this pull request as ready for review February 8, 2025 22:43
@auerswal
Copy link
Collaborator Author

I plan to first merge #376, then rebase this PR. I would like to merge this PR in about a week, unless there are objections. If you would like me to wait for a positive review of the PR, please let me know.

* whitespace in sufficiently short lines is skipped
* additional words after the target are ignored in sufficiently
  short lines
* whitespace-only (i.e., blank, but not empty) lines are skipped
* DOS-format text files, i.e., lines terminated with CRLF, work
* input need not be a correct POSIX text file, i.e., non-empty
  input does not need to end with LF
This addresses GitHub isse schweikert#377.

Before, input lines longer than the static buffer used to read
data would be handled as if each buffer part were one line.
This could result in splitting input target names into multiple
different target names.

Since the static buffer was significantly smaller then the maximum
length of DNS names (just short of 255 one octet characters),
valid long DNS names could not be read as targets via file.

This commit fixes the splitting of long lines, and also increases
the maximum length of target names to 255 one octet characters.

The additional line parsing code is kept similar to the existing
code and attempts to keep all intended and/or useful features to
minimize the risk of regressions.
Copy link
Collaborator

@gsnw-sebast gsnw-sebast 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

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.

Splitting with a long URL and a pipe
3 participants