Skip to contentSkip to conversation

tc_util: fix incorrect bare number process in get_rate.

PATCH In review Li Wei 2 participants 5 messages Jul 11, 2012 – Jul 12, 2012

Subsystems (2)
lore
Share & tools
lore

Li Wei

author
#0 lore raw

PATCH 1/1 tc_util: fix incorrect bare number process in get_rate.

As the comment and manpage indicated that the bare number means bytes per second, so the division is not needed.

---
 tc/tc_util.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
tc/tc_util.c +1 -1
index 926ed08..e8d89c1 100644
@@ -153,7 +153,7 @@ int get_rate(unsigned *rate, const char *str)
 		return -1;
 
 	if (*p == '\0') {
-		*rate = bps / 8.;	/* assume bytes/sec */
+		*rate = bps;	/* assume bytes/sec */
 		return 0;
 	}
 
Signature · 1 line
1.7.1
#1 lore raw
Quoted 12 lines · On Wed, 11 Jul 2012 15:24:50 +0800 Li Wei <[email protected]> wrote:

As the comment and manpage indicated that the bare number means bytes per second, so the division is not needed.

---
 tc/tc_util.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
tc/tc_util.c +0 -0
index 926ed08..e8d89c1 100644

tc/tc_util.c int get_rate(unsigned *rate, const char *str) · line 159

Quoted 8 lines
tc/tc_util.c +1 -1
@@ -153,7 +153,7 @@ int get_rate(unsigned *rate, const char *str)
 		return -1;
 
 	if (*p == '\0') {
-		*rate = bps / 8.;	/* assume bytes/sec */
+		*rate = bps;	/* assume bytes/sec */
 		return 0;
 	}
 

Thanks for finding this. The documentation, code and comment do all need to be the same!

But changing the code as you propose would break existing usage by scripts. Instead, the man page and comment need to change to match the reality of the existing application.

author
#2 lore raw
Quoted 26 lines · Stephen Hemminger
Quoted 12 lines · On Wed, 11 Jul 2012 15:24:50 +0800 Li Wei <[email protected]> wrote:

As the comment and manpage indicated that the bare number means bytes per second, so the division is not needed.

---
 tc/tc_util.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
tc/tc_util.c +0 -0
index 926ed08..e8d89c1 100644

tc/tc_util.c int get_rate(unsigned *rate, const char *str) · line 159

Quoted 8 lines
tc/tc_util.c +1 -1
@@ -153,7 +153,7 @@ int get_rate(unsigned *rate, const char *str)
 		return -1;
 
 	if (*p == '\0') {
-		*rate = bps / 8.;	/* assume bytes/sec */
+		*rate = bps;	/* assume bytes/sec */
 		return 0;
 	}
 

Thanks for finding this. The documentation, code and comment do all need to be the same!

Quoted 5 lines

But changing the code as you propose would break existing usage by scripts. Instead, the man page and comment need to change to match the reality of the existing application.

Well, I see, I'll send another patch to take care of this.

Thanks,

Wei

author
#3 lore raw

PATCH 1/1 tc: man: change man page and comment to confirm to code's behavior.

In reply to Stephen Hemminger

Since the get_rate() code incorrectly interpreted bare number, the behavior is not the same as man page and comment described.

We need to change the man page and comment for compatible with the existing usage by scripts.

---
 man/man8/tc.8 |    7 +++++--
 tc/tc_util.c  |    2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)
man/man8/tc.8 +5 -2
index 958ab98..f0e5613 100644
@@ -259,6 +259,9 @@ All parameters accept a floating point number, possibly followed by a unit.
 .P
 Bandwidths or rates can be specified in:
 .TP
+bps
+Bytes per second
+.TP
 kbps
 Kilobytes per second
 .TP
@@ -271,8 +274,8 @@ Kilobits per second
 mbit
 Megabits per second
 .TP
-bps or a bare number
-Bytes per second
+bit or a bare number
+Bits per second
 .P
 Amounts of data can be specified in:
 .TP
tc/tc_util.c +1 -1
index 926ed08..ccf8fa4 100644
@@ -153,7 +153,7 @@ int get_rate(unsigned *rate, const char *str)
 		return -1;
 
 	if (*p == '\0') {
-		*rate = bps / 8.;	/* assume bytes/sec */
+		*rate = bps / 8.;	/* assume bits/sec */
 		return 0;
 	}
 
Signature · 1 line
1.7.1
#4 lore raw

Re: [PATCH] tc: man: change man page and comment to confirm to code's behavior.

Quoted 10 lines · On Thu, 12 Jul 2012 09:56:57 +0800 Li Wei <[email protected]> wrote:

Since the get_rate() code incorrectly interpreted bare number, the behavior is not the same as man page and comment described.

We need to change the man page and comment for compatible with the existing usage by scripts.

---
 man/man8/tc.8 |    7 +++++--
 tc/tc_util.c  |    2 +-
 2 files changed, 6 insertions(+), 3 deletions(-)

Thanks for fixing. Accepted.