[1/1] batctl: ping: Add millisecond precision to ping interval
Commit Message
Modify the batctl ping utility to accept both integer and floating-point
values for the interval between sending pings. This enhancement allows
specifying intervals with millisecond precision.
For example:
`sudo batctl ping aa:bb:cc:dd:ee:ff -i 0.5`
Also, improve error handling for invalid interval arguments.
Signed-off-by: Noah Peterson <noahbpeterson1997@gmail.com>
---
ping.c | 35 +++++++++++++++++++++++++++--------
1 file changed, 27 insertions(+), 8 deletions(-)
Comments
On Friday, 18 October 2024 23:47:13 CEST Noah Peterson wrote:
> Modify the batctl ping utility to accept both integer and floating-point
> values for the interval between sending pings. This enhancement allows
> specifying intervals with millisecond precision.
You use nanosleep - so this would be nanoseconds. But it is unlikely that we
would be able to send in this precise interval. So maybe rename it it
subsecond.
>
> For example:
> `sudo batctl ping aa:bb:cc:dd:ee:ff -i 0.5`
>
> Also, improve error handling for invalid interval arguments.
>
> Signed-off-by: Noah Peterson <noahbpeterson1997@gmail.com>
> ---
> ping.c | 35 +++++++++++++++++++++++++++--------
> 1 file changed, 27 insertions(+), 8 deletions(-)
>
> diff --git a/ping.c b/ping.c
> index 3681e7e..104b17c 100644
> --- a/ping.c
> +++ b/ping.c
> @@ -16,6 +16,7 @@
> #include <signal.h>
> #include <fcntl.h>
> #include <string.h>
> +#include <limits.h>
> #include <math.h>
> #include <stddef.h>
> #include <stdint.h>
#include <time.h> missing after <sys/time.h>.
> @@ -65,14 +66,15 @@ static int ping(struct state *state, int argc, char **argv)
> struct bat_host *bat_host, *rr_host;
> ssize_t read_len;
> int ret = EXIT_FAILURE, res, optchar, found_args = 1;
> - int loop_count = -1, loop_interval = 0, timeout = 1, rr = 0, i;
> + int loop_count = -1, timeout = 1, rr = 0, i;
> unsigned int seq_counter = 0, packets_out = 0, packets_in = 0, packets_loss;
> - char *dst_string, *mac_string, *rr_string;
> - double time_delta;
> + char *dst_string, *mac_string, *rr_string, *end, *loop_interval = NULL;
> + double time_delta, ping_interval = 0.0;
> float min = 0.0, max = 0.0, avg = 0.0, mdev = 0.0;
> uint8_t last_rr_cur = 0, last_rr[BATADV_RR_LEN][ETH_ALEN];
> size_t packet_len;
> int disable_translate_mac = 0;
> + struct timespec req;
>
> while ((optchar = getopt(argc, argv, "hc:i:t:RT")) != -1) {
> switch (optchar) {
> @@ -86,9 +88,7 @@ static int ping(struct state *state, int argc, char **argv)
> ping_usage();
> return EXIT_SUCCESS;
> case 'i':
> - loop_interval = strtol(optarg, NULL , 10);
> - if (loop_interval < 1)
> - loop_interval = 1;
> + loop_interval = strdup(optarg);
Why are you duplicating this string?
> found_args += ((*((char*)(optarg - 1)) == optchar ) ? 1 : 2);
> break;
> case 't':
> @@ -135,6 +135,25 @@ static int ping(struct state *state, int argc, char **argv)
> }
> }
>
> + if (loop_interval) {
> + errno = 0;
> + ping_interval = strtod(loop_interval, &end);
> + free(loop_interval);
> +
> + if (errno || end != (loop_interval + strlen(loop_interval))) {
Use after-free of loop_interval
> + fprintf(stderr, "Error - invalid ping interval '%s'\n", (ULONG_MAX / 1000000 - 1.0));
You can't print a double as string. Please use %f, %e or %g to print a double
value.
But the text actually suggest that you didn't want to print the calculated
value in the first place
> + goto out;
> + } else if (ping_interval >= (ULONG_MAX / 1000000 - 1.0)) {
Similar to the previously calculated value - this doesn't make a lot of sense.
You are dividing ULONG_MAX by 1e6 but we have no place where we handle
something related to Msecs or usecs. tv_sec is in time_t and not related to
ULONG_MAX / 1e6.
I can also not explain the -1.0 (at the moment). I first thought this was a
bad attempt to floor the ping interval - but this would be on the wrong side
of the comparison and there is actually a function for that. And since the
ULONG_MAX / 1e6 seems to be random, I am unable to find the reason for this.
> + fprintf(stderr, "Error - ping interval too large\n");
> + goto out;
> + } else {
> + ping_interval = fmax(ping_interval, 0.001);
> + }
> +
> + req.tv_sec = (long) (ping_interval);
time_t not long
> + req.tv_nsec = fmod(ping_interval, 1.0) * 1000000000l;
I think this would be a good place to use modf to split integral and
fractional part.
Kind regards,
Sven
@@ -16,6 +16,7 @@
#include <signal.h>
#include <fcntl.h>
#include <string.h>
+#include <limits.h>
#include <math.h>
#include <stddef.h>
#include <stdint.h>
@@ -65,14 +66,15 @@ static int ping(struct state *state, int argc, char **argv)
struct bat_host *bat_host, *rr_host;
ssize_t read_len;
int ret = EXIT_FAILURE, res, optchar, found_args = 1;
- int loop_count = -1, loop_interval = 0, timeout = 1, rr = 0, i;
+ int loop_count = -1, timeout = 1, rr = 0, i;
unsigned int seq_counter = 0, packets_out = 0, packets_in = 0, packets_loss;
- char *dst_string, *mac_string, *rr_string;
- double time_delta;
+ char *dst_string, *mac_string, *rr_string, *end, *loop_interval = NULL;
+ double time_delta, ping_interval = 0.0;
float min = 0.0, max = 0.0, avg = 0.0, mdev = 0.0;
uint8_t last_rr_cur = 0, last_rr[BATADV_RR_LEN][ETH_ALEN];
size_t packet_len;
int disable_translate_mac = 0;
+ struct timespec req;
while ((optchar = getopt(argc, argv, "hc:i:t:RT")) != -1) {
switch (optchar) {
@@ -86,9 +88,7 @@ static int ping(struct state *state, int argc, char **argv)
ping_usage();
return EXIT_SUCCESS;
case 'i':
- loop_interval = strtol(optarg, NULL , 10);
- if (loop_interval < 1)
- loop_interval = 1;
+ loop_interval = strdup(optarg);
found_args += ((*((char*)(optarg - 1)) == optchar ) ? 1 : 2);
break;
case 't':
@@ -135,6 +135,25 @@ static int ping(struct state *state, int argc, char **argv)
}
}
+ if (loop_interval) {
+ errno = 0;
+ ping_interval = strtod(loop_interval, &end);
+ free(loop_interval);
+
+ if (errno || end != (loop_interval + strlen(loop_interval))) {
+ fprintf(stderr, "Error - invalid ping interval '%s'\n", (ULONG_MAX / 1000000 - 1.0));
+ goto out;
+ } else if (ping_interval >= (ULONG_MAX / 1000000 - 1.0)) {
+ fprintf(stderr, "Error - ping interval too large\n");
+ goto out;
+ } else {
+ ping_interval = fmax(ping_interval, 0.001);
+ }
+
+ req.tv_sec = (long) (ping_interval);
+ req.tv_nsec = fmod(ping_interval, 1.0) * 1000000000l;
+ }
+
if (!disable_translate_mac)
dst_mac = translate_mac(state, dst_mac);
@@ -286,8 +305,8 @@ static int ping(struct state *state, int argc, char **argv)
if (loop_count == 0)
continue;
- if (loop_interval > 0)
- sleep(loop_interval);
+ if (ping_interval > 0.0)
+ nanosleep(&req, NULL);
else if ((tv.tv_sec != 0) || (tv.tv_usec != 0))
select(0, NULL, NULL, NULL, &tv);
}