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

[Remi Pommarel <repk@triplefau.lt>]

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,

-- 
Remi