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

Don't accept negative values. They are not valid. #26

Closed
wants to merge 1 commit into from

Conversation

NOYB
Copy link

@NOYB NOYB commented Sep 17, 2017

Negative values produce erroneous results like this:

dpinger -f -B 192.168.1.48 -s -1 192.168.1.1
the time period must be greater than twice the send interval plus the loss interval

dpinger -f -B 192.168.1.48 -s -1 -l 2000 -A 1 192.168.1.1
send_interval 18446744073709551615ms // 584 million years (2^64 - 1) (FFFFFFFFFFFFFFFF)
loss_interval 2000ms
time_period 60000ms
report_interval 1000ms
data_len 0
alert_interval 1ms
latency_alarm 0ms
loss_alarm 0%
dest_addr 192.168.1.1
bind_addr 192.168.1.48
identifier ""

Instead of clearer messages like this:

dpinger -f -B 192.168.1.48 -s -1 192.168.1.1
invalid send interval -1

dpinger -f -B 192.168.1.48 -s -1 -l 2000 -A 1 192.168.1.1
invalid send interval -1

@dennypage
Copy link
Owner

Couple of issues.: You can't use index 0 containing a '-' to detect negative numbers because numbers may have have leading white space; The check in percent_arg has no effect because there was already a limiter there.

Honestly, I'm not sure this is worth addressing. However, if it were a better approach would be to introduce value limiters for the parameters to prevent general stupidity rather than specifically targeting negative numbers. I'll have to think about it.

@NOYB
Copy link
Author

NOYB commented Sep 27, 2017

In the context of this code the numbers are input parameters and leading white space appears to be excluded.

Thought about removing the '-' in the parameters parsing but this was more concise. Yes there are a multitude of ways to address it. Doesn't really matter to me how or if it is address. Just putting it out there.

Aware of the percent t > 100 limiter but it does accept very large negative numbers that result in t not being > 100. Sure, it's a valid percent value but does not match what was entered as the parameter value. And thus likely results in unexpected result/behavior as well. Plus I like code consistency (doing similar things the same way). Easier to remember than, this way here, and that way there and some other way elsewhere.

Right, it's just some hand holding to prevent erroneous input. Whether it be due to stupidity or typo, etc. Doesn't add any capability or flexibility etc.

@dennypage
Copy link
Owner

"In the context of this code the numbers are input parameters and leading white space appears to be excluded."

For future, command line parameters can have leading spaces which is why strtoul() and friends supports same. You won't notice normally because you are typing in an interactive shell which trims extra spaces. But try "dpinger -t ' 300s -r ' 60s' '192.168.0.1'" as a command line and you'll see it. This kind of thing ends up happening a lot when parameters are entered by humans into storage systems that end up feeding scripting systems.

Anyway, thank you again for pointing out the negative number conversion issue.

Negative values produce erroneous results like this:

dpinger -f -B 192.168.1.48 -s -1 192.168.1.1
  the time period must be greater than twice the send interval plus the loss interval

dpinger -f -B 192.168.1.48 -s -1 -l 2000 -A 1 192.168.1.1
  send_interval 18446744073709551615ms	// 584 million years (2^64 - 1) (FFFFFFFFFFFFFFFF)
  loss_interval 2000ms
  time_period 60000ms
  report_interval 1000ms
  data_len 0
  alert_interval 1ms
  latency_alarm 0ms
  loss_alarm 0%
  dest_addr 192.168.1.1
  bind_addr 192.168.1.48
  identifier ""

Instead of clearer messages like this:

dpinger -f -B 192.168.1.48 -s -1 192.168.1.1
  invalid send interval -1

dpinger -f -B 192.168.1.48 -s -1 -l 2000 -A 1 192.168.1.1
  invalid send interval -1

Updated to accomodate quoted leading whitespace.  Test for '-' anywhere in the string.
s/arg[0] == '-'/strchr(arg, '-')/
@dennypage
Copy link
Owner

This has been addressed. Thank you again @NOYB.

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.

None yet

2 participants