[v2,1/1] ip-link: in human readable output use dynamic precision length

---
 ip/ipaddress.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

From: Christian Hesse
...
> diff --git a/ip/ipaddress.c b/ip/ipaddress.c
> index e240bb5..55cbc77 100644
> --- a/ip/ipaddress.c
> +++ b/ip/ipaddress.c
> @@ -324,6 +324,7 @@ static void print_num(FILE *fp, unsigned width, uint64_t count)
>  	const char *prefix = "kMGTPE";
>  	const unsigned int base = use_iec ? 1024 : 1000;
>  	uint64_t powi = 1;
> +	int precision;
>  	char buf[64];
> 
>  	if (!human_readable || count < base) {
> @@ -343,8 +344,11 @@ static void print_num(FILE *fp, unsigned width, uint64_t count)
>  		++prefix;
>  	}
> 
> -	snprintf(buf, sizeof(buf), "%.1f%c%s", (double) count / powi,
> -		 *prefix, use_iec ? "i" : "");
> +	if ((precision = 3 - snprintf(NULL, 0, "%"PRIu64, count / powi)) < 0)

Don't put assignments in conditionals.

> +		precision = 0;
> +
> +	snprintf(buf, sizeof(buf), "%.*f%c%s", precision,
> +		(double) count / powi, *prefix, use_iec ? "i" : "");
> 
>  	fprintf(fp, "%-*s ", width, buf);
>  }
> --
> 2.1.3

The above will go wrong in all sorts of horrid ways....
For instance you are doing a truncating integer divide, but the FP
value will get rounded for display.

It would be safer to use integers throughout.

Oh, and a 2Mbit E1 link is actually 2048000 :-)

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

David Laight <David.Laight@ACULAB.COM> on Tue, 2014/11/04 11:06:
> From: Christian Hesse
> ...
> > ...
> > @@ -343,8 +344,11 @@ static void print_num(FILE *fp, unsigned width,
> > uint64_t count) ++prefix;
> >  	}
> > 
> > -	snprintf(buf, sizeof(buf), "%.1f%c%s", (double) count / powi,
> > -		 *prefix, use_iec ? "i" : "");
> > +	if ((precision = 3 - snprintf(NULL, 0, "%"PRIu64, count / powi))
> > < 0)
> 
> Don't put assignments in conditionals.

Ok. :D

I do not like this at all... snprintf() would be nice for a catch-all, but we
have to take care of negative values. So let's try something different.
I will think about it and send a new patch.

> > +		precision = 0;
> > +
> > +	snprintf(buf, sizeof(buf), "%.*f%c%s", precision,
> > +		(double) count / powi, *prefix, use_iec ? "i" : "");
> > 
> >  	fprintf(fp, "%-*s ", width, buf);
> >  }
> 
> The above will go wrong in all sorts of horrid ways....
> For instance you are doing a truncating integer divide, but the FP
> value will get rounded for display.
> 
> It would be safer to use integers throughout.

My implementation used integers, but Stephen changes this to floating point
with his cleanups.

IMHO the rounding is ok. This is for *human* readability. ;)
Whoever wants correct values should not ask ip to print human readable values
but rely on pure numbers.

> Oh, and a 2Mbit E1 link is actually 2048000 :-)

Sorry? Did not get the point.

From: Christian Hesse
...
> 
> I do not like this at all... snprintf() would be nice for a catch-all, but we
> have to take care of negative values. So let's try something different.
> I will think about it and send a new patch.
> 
> > > +		precision = 0;
> > > +
> > > +	snprintf(buf, sizeof(buf), "%.*f%c%s", precision,
> > > +		(double) count / powi, *prefix, use_iec ? "i" : "");
> > >
> > >  	fprintf(fp, "%-*s ", width, buf);
> > >  }
> >
> > The above will go wrong in all sorts of horrid ways....
> > For instance you are doing a truncating integer divide, but the FP
> > value will get rounded for display.
> >
> > It would be safer to use integers throughout.
> 
> My implementation used integers, but Stephen changes this to floating point
> with his cleanups.
> 
> IMHO the rounding is ok. This is for *human* readability. ;)
> Whoever wants correct values should not ask ip to print human readable values
> but rely on pure numbers.

Rounding the value is fine, the difference between 1000 and 1024
is not that great either.
The problem is that the two calculations get rounded differently.
If the FP value ends up being 9.999999 it will be printed as 10.0
but your field width calculation will only have allowed for
a single digit.

> > Oh, and a 2Mbit E1 link is actually 2048000 :-)
> 
> Sorry? Did not get the point.
That was really in reference to a much earlier comment about comms
always using 1000.

	David



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html