diff mbox series

[v5,1/1] batctl: ping: Add subsecond precision to ping interval

Message ID 20250105000908.66343-2-NoahBPeterson1997@gmail.com (mailing list archive)
State Accepted, archived
Delegated to: Simon Wunderlich
Headers
Series batctl: ping: Add subsecond precision to ping interval |

Commit Message

Noah Peterson Jan. 5, 2025, 12:09 a.m. UTC 
  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 subsecond precision.

For example:
`sudo batctl ping aa:bb:cc:dd:ee:ff -i 0.5`

Signed-off-by: Noah Peterson <noahbpeterson1997@gmail.com>
---
v5: Fixing whitespace misalignment, correcting changelogs, removing old
variable declaration, as noted by Sven <sven@narfation.org>
v4: Rebasing on latest commit, reformatting into reverse X-mas tree order based
on feedback from Sven <sven@narfation.org>
v3: Reformatting code based on comments by Sven
<sven@narfation.org>
v2: Fixing use-after-free, adding a missing header file, noted by Sven
<sven@narfation.org>
---
 ping.c | 27 ++++++++++++++++++++-------
 1 file changed, 20 insertions(+), 7 deletions(-)

Comments

Sven Eckelmann Jan. 5, 2025, 7:12 a.m. UTC | #1 
On Sunday, 5 January 2025 01:09:08 GMT+1 Noah Peterson wrote:
[...]
>v5: Fixing whitespace misalignment,
[...]
> @@ -330,7 +343,7 @@ static int ping(struct state *state, int argc, char **argv)
>         printf("%u packets transmitted, %u received, %u%% packet loss\n",
>                packets_out, packets_in, packets_loss);
>         printf("rtt min/avg/max/mdev = %.3f/%.3f/%.3f/%.3f ms\n",
> -              min, avg, max, mdev);
> +               min, avg, max, mdev);
>  
>         if (packets_in)
>                 ret = EXIT_SUCCESS;

Ehm.....


Kind regards,
	Sven
Sven Eckelmann Jan. 5, 2025, 8:47 a.m. UTC | #2 
On Sunday, 5 January 2025 01:09:08 GMT+1 Noah Peterson wrote:
> +                       errno = 0;
> +                       ping_interval = strtod(optarg, &end);
> +                       if (errno) {

You forgot to do anything with the end(ptr).

Btw. end is a bad choice for the name because it shadows the global variable 
for "the first address past the end of the uninitialized data segment (also 
known as the BSS segment)".


I will now change the patch myself because this amount of ping-pong for such a 
trivial patch is wasting everyones time and should be unnecessary. Please try 
to focus and recheck things before you submit your patches.

Kind regards,
	Sven
Remi Pommarel Jan. 6, 2025, 1:49 p.m. UTC | #3 
Hi,

On Sat, Jan 04, 2025 at 06:09:08PM -0600, 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 subsecond precision.
> 
> For example:
> `sudo batctl ping aa:bb:cc:dd:ee:ff -i 0.5`
> 
> Signed-off-by: Noah Peterson <noahbpeterson1997@gmail.com>
> ---
> v5: Fixing whitespace misalignment, correcting changelogs, removing old
> variable declaration, as noted by Sven <sven@narfation.org>
> v4: Rebasing on latest commit, reformatting into reverse X-mas tree order based
> on feedback from Sven <sven@narfation.org>
> v3: Reformatting code based on comments by Sven
> <sven@narfation.org>
> v2: Fixing use-after-free, adding a missing header file, noted by Sven
> <sven@narfation.org>
> ---
>  ping.c | 27 ++++++++++++++++++++-------
>  1 file changed, 20 insertions(+), 7 deletions(-)
> 
> diff --git a/ping.c b/ping.c
> index 14d9c21..6fd11b2 100644
> --- a/ping.c
> +++ b/ping.c
> @@ -19,6 +19,7 @@
>  #include <stdint.h>
>  #include <sys/select.h>
>  #include <sys/time.h>
> +#include <time.h>
>  #include <netinet/if_ether.h>
>  
>  #include "batadv_packet_compat.h"
> @@ -58,18 +59,21 @@ static int ping(struct state *state, int argc, char **argv)
>  	struct batadv_icmp_packet_rr icmp_packet_out;
>  	struct batadv_icmp_packet_rr icmp_packet_in;
>  	uint8_t last_rr[BATADV_RR_LEN][ETH_ALEN];
> +	struct timespec loop_interval = {0, 0};
>  	struct ether_addr *dst_mac = NULL;
>  	struct ether_addr *rr_mac = NULL;
>  	int disable_translate_mac = 0;
> +	double fractional_part = 0.0;
>  	unsigned int seq_counter = 0;
>  	unsigned int packets_out = 0;
>  	unsigned int packets_in = 0;
> +	double ping_interval = 0.0;
> +	double integral_part = 0.0;
>  	unsigned int packets_loss;
>  	struct bat_host *bat_host;
>  	struct bat_host *rr_host;
>  	uint8_t last_rr_cur = 0;
>  	int ret = EXIT_FAILURE;
> -	int loop_interval = 0;
>  	int loop_count = -1;
>  	int found_args = 1;
>  	size_t packet_len;
> @@ -86,6 +90,7 @@ static int ping(struct state *state, int argc, char **argv)
>  	int timeout = 1;
>  	int optchar;
>  	int rr = 0;
> +	char *end;
>  	int res;
>  	int i;
>  
> @@ -101,9 +106,17 @@ 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;
> +			errno = 0;
> +			ping_interval = strtod(optarg, &end);
> +			if (errno) {
> +				fprintf(stderr, "Error - invalid ping interval '%s'\n", optarg);
> +				goto out;
> +			} else {
> +				ping_interval = fmax(ping_interval, 0.001);
> +			}

If you are going to floor to 1 millisecond anyway why didn't you use
msleep() (or fsleep() if you needed precision) then ? This would avoid
the conversion to timespec and hence the use of modf().

Regards,
diff mbox series

Patch

diff --git a/ping.c b/ping.c
index 14d9c21..6fd11b2 100644
--- a/ping.c
+++ b/ping.c
@@ -19,6 +19,7 @@ 
 #include <stdint.h>
 #include <sys/select.h>
 #include <sys/time.h>
+#include <time.h>
 #include <netinet/if_ether.h>
 
 #include "batadv_packet_compat.h"
@@ -58,18 +59,21 @@  static int ping(struct state *state, int argc, char **argv)
 	struct batadv_icmp_packet_rr icmp_packet_out;
 	struct batadv_icmp_packet_rr icmp_packet_in;
 	uint8_t last_rr[BATADV_RR_LEN][ETH_ALEN];
+	struct timespec loop_interval = {0, 0};
 	struct ether_addr *dst_mac = NULL;
 	struct ether_addr *rr_mac = NULL;
 	int disable_translate_mac = 0;
+	double fractional_part = 0.0;
 	unsigned int seq_counter = 0;
 	unsigned int packets_out = 0;
 	unsigned int packets_in = 0;
+	double ping_interval = 0.0;
+	double integral_part = 0.0;
 	unsigned int packets_loss;
 	struct bat_host *bat_host;
 	struct bat_host *rr_host;
 	uint8_t last_rr_cur = 0;
 	int ret = EXIT_FAILURE;
-	int loop_interval = 0;
 	int loop_count = -1;
 	int found_args = 1;
 	size_t packet_len;
@@ -86,6 +90,7 @@  static int ping(struct state *state, int argc, char **argv)
 	int timeout = 1;
 	int optchar;
 	int rr = 0;
+	char *end;
 	int res;
 	int i;
 
@@ -101,9 +106,17 @@  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;
+			errno = 0;
+			ping_interval = strtod(optarg, &end);
+			if (errno) {
+				fprintf(stderr, "Error - invalid ping interval '%s'\n", optarg);
+				goto out;
+			} else {
+				ping_interval = fmax(ping_interval, 0.001);
+			}
+			fractional_part = modf(ping_interval, &integral_part);
+			loop_interval.tv_sec = (time_t)integral_part;
+			loop_interval.tv_nsec = (long)(fractional_part * 1000000000l);
 			found_args += ((*((char *)(optarg - 1)) == optchar) ? 1 : 2);
 			break;
 		case 't':
@@ -302,8 +315,8 @@  static int ping(struct state *state, int argc, char **argv)
 		if (loop_count == 0)
 			continue;
 
-		if (loop_interval > 0)
-			sleep(loop_interval);
+		if (loop_interval.tv_sec > 0 || loop_interval.tv_nsec > 0)
+			nanosleep(&loop_interval, NULL);
 		else if ((tv.tv_sec != 0) || (tv.tv_usec != 0))
 			select(0, NULL, NULL, NULL, &tv);
 	}
@@ -330,7 +343,7 @@  static int ping(struct state *state, int argc, char **argv)
 	printf("%u packets transmitted, %u received, %u%% packet loss\n",
 	       packets_out, packets_in, packets_loss);
 	printf("rtt min/avg/max/mdev = %.3f/%.3f/%.3f/%.3f ms\n",
-	       min, avg, max, mdev);
+		min, avg, max, mdev);
 
 	if (packets_in)
 		ret = EXIT_SUCCESS;