discard late packets, auto-adjust timeout for -c/-C/-l, fixes #32

diff --git a/ChangeLog b/ChangeLog
index fc361d65d3278fdf9dca9342c4b762927043064e..3ce3b8bc74f23a9f79ab150cd2f42e2391cb6d75 100644 (file)
--- a/ChangeLog
+++ b/ChangeLog
@@ -24,6 +24,18 @@
     fping -n NAME    NAME->IP->IPNAME       NAME
     fping -d NAME    NAME->IP->IPNAME       NAME->IP->IPNAME
 
+  * INCOMPATIBILITY WARNING 3:
+    fping will now reject replies, if they arrive after the defined timeout
+    for reply packets, specified with -t. This change is relevant only for
+    for the counting and looping modes, where now the measured times should
+    be more consistent (see github issue #32 for details).
+
+    To prevent loosing reply packets because of this change, the default
+    timeout in counting and looping modes is now automatically adjusted
+    to the period interval (up to 2000ms), but it can be overriden with
+    the -t option. The default timeout for non-looping/counting modes
+    remains 500ms.
+
   * (feature) Unified 'fping' and 'fping6' into one binary (#80)
   * (feature) Long option names for all options
   * (feature) --enable-ipv6 is now default
@@ -32,6 +44,8 @@
   * (feature) Keep original name if a hostname is given with -n/--name
   * (feature) Option -d/--rdns now always does a rdns-lookup, even for names
               (name->IP->name), as '-n' was doing until now
+  * (feature) Enforce -t timeout on reply packets, by discarding late packets (#32)
+  * (feature) Auto-adjust timeout for -c/-C/-l mode to value of -p
   * (bugfix)  Fix compatibility issue with GNU Hurd
   * (other)   A C99 compiler is now required
   * (other)   Option parsing with optparse (https://github.com/skeeto/optparse)

diff --git a/ci/test-09-option-r-t.pl b/ci/test-09-option-r-t.pl
index 2ea7a68d2a03b06307f52bfd53e545772e8b52b8..bd118a3ed9827185c670b7ae8a5319e77a2ec422 100755 (executable)
--- a/ci/test-09-option-r-t.pl
+++ b/ci/test-09-option-r-t.pl
@@ -1,6 +1,6 @@
 #!/usr/bin/perl -w
 
-use Test::Command tests => 21;
+use Test::Command tests => 23;
 use Test::More;
 
 #  -R         random bytes
@@ -103,4 +103,10 @@ $cmd->stdout_is_eq("");
 $cmd->stderr_is_eq("fping: can't parse source address: bla\n");
 }
 
-# fping -t tested in test-4-options-a-b.pl
+# (note: fping -t also tested in test-4-options-a-b.pl)
+
+{
+my $cmd = Test::Command->new(cmd => "fping -c 2 -p 100 -t 200 127.0.0.1");
+$cmd->exit_is_num(0);
+$cmd->stderr_like(qr{^fping: warning: timeout \(-t\) value larger than period \(-p\) produces unexpected results\n.*});
+}

diff --git a/doc/fping.pod b/doc/fping.pod
index 4baed418e1591155474d7b985040bf98ccf89b5d..9dcf19b69b78af5898753f7cf4ac572dd07f8f36 100644 (file)
--- a/doc/fping.pod
+++ b/doc/fping.pod
@@ -199,11 +199,19 @@ Set source address.
 
 =item B<-t>, B<--timeout>=I<MSEC>
 
-Initial target timeout in milliseconds (default 500). In the default mode, this
-is the amount of time that B<fping> waits for a response to its first request.
-Successive timeouts are multiplied by the backoff factor specified with B<-B>.
-Note that this option has no effect looping or counting modes (B<-l>, B<-c>, or
-B<-C>).
+Initial target timeout in milliseconds. In the default, non-loop mode, the
+default timeout is 500ms, and it represents the amount of time that B<fping>
+waits for a response to its first request. Successive timeouts are multiplied
+by the backoff factor specified with B<-B>.
+
+In loop/count mode, the default timeout is automatically adjusted to match
+the "period" value (but not more than 2000ms). You can still adjust the timeout
+value with this option, if you wish to, but note that setting a value larger
+than "period" produces inconsistent results, because the timeout value can
+be respected only for the last ping.
+
+Also note that any received replies that are larger than the timeout value, will
+be discarded.
 
 =item B<-T> I<n>
 

diff --git a/src/fping.c b/src/fping.c
index ee2da5fd2f362e44be4b8c372c2e88e85cb4cae7..a9b175bd6a23cbc34241496a2848d9bbb24ac8ee 100644 (file)
--- a/src/fping.c
+++ b/src/fping.c
@@ -282,7 +282,7 @@ int generate_flag = 0; /* flag for IP list generation */
 int verbose_flag, quiet_flag, stats_flag, unreachable_flag, alive_flag;
 int elapsed_flag, version_flag, count_flag, loop_flag, netdata_flag;
 int per_recv_flag, report_all_rtts_flag, name_flag, addr_flag, backoff_flag, rdns_flag;
-int multif_flag;
+int multif_flag, timeout_flag;
 int outage_flag = 0;
 int timestamp_flag = 0;
 int random_data_flag = 0;
@@ -453,6 +453,7 @@ int main(int argc, char** argv)
         case 't':
             if (!(timeout = (unsigned int)atoi(optparse_state.optarg) * 100))
                 usage(1);
+            timeout_flag = 1;
 
             break;
 
@@ -729,6 +730,22 @@ int main(int argc, char** argv)
 
     trials = (count > retry + 1) ? count : retry + 1;
 
+    /* auto-tune default timeout for count/loop modes
+     * see also github #32 */
+    if (loop_flag || count_flag) {
+        if(!timeout_flag) {
+            timeout = perhost_interval;
+            if(timeout > AUTOTUNE_TIMEOUT_MAX*100) {
+                timeout = AUTOTUNE_TIMEOUT_MAX*100;
+            }
+        }
+        else {
+            if(timeout > perhost_interval && (loop_flag || (count_flag && count > 1))) {
+                fprintf(stderr, "%s: warning: timeout (-t) value larger than period (-p) produces unexpected results\n", prog);
+            }
+        }
+    }
+
 #if defined(DEBUG) || defined(_DEBUG)
     if (debugging & DBG_TRACE)
         trace_flag = 1;
@@ -1995,7 +2012,7 @@ int wait_for_reply(long wait_time)
         );
 
     if (result <= 0) {
-        return 0;
+        return 1;
     }
 
     gettimeofday(&current_time, &tz);
@@ -2046,6 +2063,13 @@ int wait_for_reply(long wait_time)
     this_count = seqmap_value->ping_count;
     this_reply = timeval_diff(&recv_time, sent_time);
 
+
+    /* discard reply if delay is larger than timeout
+     * (see also: github #32) */
+    if(this_reply > h->timeout) {
+        return 1;
+    }
+
     if (loop_flag || h->resp_times[this_count] == RESP_WAITING) {
         /* only for non-duplicates: */
         h->waiting = 0;
@@ -2684,9 +2708,9 @@ void usage(int is_error)
     fprintf(out, "Probing options:\n");
     fprintf(out, "   -4, --ipv4         only ping IPv4 addresses\n");
     fprintf(out, "   -6, --ipv6         only ping IPv6 addresses\n");
-    fprintf(out, "   -b, --size=BYTES   amount of ping data to send, in bytes (default %d)\n", DEFAULT_PING_DATA_SIZE);
+    fprintf(out, "   -b, --size=BYTES   amount of ping data to send, in bytes (default: %d)\n", DEFAULT_PING_DATA_SIZE);
     fprintf(out, "   -B, --backoff=N    set exponential backoff factor to N\n");
-    fprintf(out, "   -c, --count=N      count of pings to send to each target (default %d)\n", count);
+    fprintf(out, "   -c, --count=N      count of pings to send to each target (default: %d)\n", count);
     fprintf(out, "   -f, --file=FILE    read list of targets from a file ( - means stdin)\n");
     fprintf(out, "   -g, --generate     generate target list (only if no -f specified)\n");
     fprintf(out, "                      (give start and end IP in the target list, or a CIDR address)\n");
@@ -2699,12 +2723,13 @@ void usage(int is_error)
     fprintf(out, "   -m, --all          use all IPs of provided hostnames (e.g. IPv4 and IPv6), use with -A\n");
     fprintf(out, "   -M, --dontfrag     set the Don't Fragment flag\n");
     fprintf(out, "   -O, --tos=N        set the type of service (tos) flag on the ICMP packets\n");
-    fprintf(out, "   -p, --period=MSEC  interval between ping packets to one target (in millisec)\n");
-    fprintf(out, "                      (in looping and counting modes, default %d)\n", perhost_interval / 100);
-    fprintf(out, "   -r, --retry=N      number of retries (default %d)\n", DEFAULT_RETRY);
+    fprintf(out, "   -p, --period=MSEC  interval between ping packets to one target (in ms)\n");
+    fprintf(out, "                      (in looping and counting modes, default: %d ms)\n", perhost_interval / 100);
+    fprintf(out, "   -r, --retry=N      number of retries (default: %d)\n", DEFAULT_RETRY);
     fprintf(out, "   -R, --random       random packet data (to foil link data compression)\n");
     fprintf(out, "   -S, --src=IP       set source address\n");
-    fprintf(out, "   -t, --timeout=MSEC individual target initial timeout (in millisec) (default %d)\n", timeout / 100);
+    fprintf(out, "   -t, --timeout=MSEC individual target initial timeout (in ms)\n");
+    fprintf(out, "                      (default: %d ms, except with -l/-c/-C where it is the -p period)\n", timeout / 100);
     fprintf(out, "\n");
     fprintf(out, "Output options:\n");
     fprintf(out, "   -a, --alive        show targets that are alive\n");
@@ -2712,7 +2737,7 @@ void usage(int is_error)
     fprintf(out, "   -C, --vcount=N     same as -c, report results in verbose format\n");
     fprintf(out, "   -D, --timestamp    print timestamp before each output line\n");
     fprintf(out, "   -e, --elapsed      show elapsed time on return packets\n");
-    fprintf(out, "   -i, --interval=MSEC  interval between sending ping packets (in ms) (default %d)\n", interval / 100);
+    fprintf(out, "   -i, --interval=MSEC  interval between sending ping packets (default: %d ms)\n", interval / 100);
     fprintf(out, "   -n, --name         show targets by name (-d is equivalent)\n");
     fprintf(out, "   -N, --netdata      output compatible for netdata (-l -Q are required)\n");
     fprintf(out, "   -o, --outage       show the accumulated outage time (lost packets * packet interval)\n");

diff --git a/src/options.h b/src/options.h
index bbc93d11e8dcec8e0b4d9435cbd72a242a9409fd..75a127750d388968c9ed77c2fe47b1cf8f591e4c 100644 (file)
--- a/src/options.h
+++ b/src/options.h
@@ -29,6 +29,7 @@
 
 #ifndef DEFAULT_TIMEOUT
 #define DEFAULT_TIMEOUT 500       /* individual host timeouts */
+#define AUTOTUNE_TIMEOUT_MAX  2000
 #endif