Re: [PATCH v2 iproute2] get_rate: detect 32bit overflows

[Ben Hutchings <bhutchings@solarflare.com>]

On Mon, 2013-06-03 at 08:19 -0700, Eric Dumazet wrote:
> On Mon, 2013-06-03 at 15:37 +0100, Ben Hutchings wrote:
> 
> > This would be more understandable without a goto.  I think this ordering
> > would work:
> 
> Yeah you're right, thanks !
> 
> [PATCH v3] get_rate: detect 32bit overflows
> 
> Current rate limit is 34.359.738.360 bit per second, and
> unfortunately 40Gbps links are above it.
> 
> overflows in get_rate() are currently not detected, and some
> users are confused. Let's detect this and complain.
> 
> Note that some qdisc are ready to get extended range, but this will
> need additional attributes and new iproute2
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  tc/tc_util.c |   22 +++++++++++++---------
>  1 file changed, 13 insertions(+), 9 deletions(-)
> 
> diff --git a/tc/tc_util.c b/tc/tc_util.c
> index 8e62a01..912216c 100644
> --- a/tc/tc_util.c
> +++ b/tc/tc_util.c
> @@ -146,25 +146,29 @@ static const struct rate_suffix {
>  int get_rate(unsigned *rate, const char *str)
>  {
>  	char *p;
> -	double bps = strtod(str, &p);
> +	long long bps = strtoll(str, &p, 0);

Oops, I read this as being strtol() currently, not strtod().  Currently
'1.5gbit' will work, but this change will break that.  So I think you
need to keep bps as a double.

>  	const struct rate_suffix *s;
>  
>  	if (p == str)
>  		return -1;
>  
> -	if (*p == '\0') {
> -		*rate = bps / 8.;	/* assume bits/sec */
> -		return 0;
> -	}
> -
>  	for (s = suffixes; s->name; ++s) {
>  		if (strcasecmp(s->name, p) == 0) {
> -			*rate = (bps * s->scale) / 8.;
> -			return 0;
> +			bps *= s->scale;
> +			p += strlen(p);
> +			break;
>  		}
>  	}
>  
> -	return -1;
> +	if (*p)
> +		return -1; /* unknown suffix */
> +	bps /= 8; /* -> bytes per second */
> +	*rate = bps;
> +	/* detect if an overflow happened */
> +	if (*rate != bps)

Then here I think the check should be *rate != floor(bps), i.e. accept
rounding down of a non-integer number of bytes but any other change is
assumed to be overflow.

Ben.

> +		return -1;
> +	return 0;
>  }
>  
>  void print_rate(char *buf, int len, __u32 rate)
> 
> 

-- 
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.