[PATCH iproute2 1/2] utils: add s32 parser

[PATCH iproute2 1/2] utils: add s32 parser

[Hagen Paul Pfeifer]

Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
---
 include/utils.h |    1 +
 lib/utils.c     |   14 ++++++++++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 47f8e07..496db68 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -85,6 +85,7 @@ extern int get_time_rtt(unsigned *val, const char *arg, int *raw);
 #define get_short get_s16
 extern int get_u64(__u64 *val, const char *arg, int base);
 extern int get_u32(__u32 *val, const char *arg, int base);
+extern int get_s32(__s32 *val, const char *arg, int base);
 extern int get_u16(__u16 *val, const char *arg, int base);
 extern int get_s16(__s16 *val, const char *arg, int base);
 extern int get_u8(__u8 *val, const char *arg, int base);
diff --git a/lib/utils.c b/lib/utils.c
index efaf377..6788dd9 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -198,6 +198,20 @@ int get_u8(__u8 *val, const char *arg, int base)
 	return 0;
 }
 
+int get_s32(__s32 *val, const char *arg, int base)
+{
+	long res;
+	char *ptr;
+
+	if (!arg || !*arg)
+		return -1;
+	res = strtoul(arg, &ptr, base);
+	if (!ptr || ptr == arg || *ptr || res > INT32_MAX || res < INT32_MIN)
+		return -1;
+	*val = res;
+	return 0;
+}
+
 int get_s16(__s16 *val, const char *arg, int base)
 {
 	long res;
-- 
1.7.7

[PATCH iproute2 2/2] tc: netem ratelatency and cell extension

[Hagen Paul Pfeifer]

This patch add ratelatency as well as cell support. Ratelatency can be
added with ratelatency options. Three optional arguments control the
cell knobs: packet-overhead, cell-size, cell-overhead. To ratelimit eth0
root queue to 5kbit/s, with a 20 byte packet overhead, 100 byte cell
size and a 5 byte per cell overhead:

	tc qdisc add dev eth0 root netem ratelatency 5kbit 20 100 5

Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
---
 include/linux/pkt_sched.h |    8 ++++++
 tc/q_netem.c              |   53 ++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 60 insertions(+), 1 deletions(-)

diff --git a/include/linux/pkt_sched.h b/include/linux/pkt_sched.h
index c533670..76b26a2 100644
--- a/include/linux/pkt_sched.h
+++ b/include/linux/pkt_sched.h
@@ -465,6 +465,7 @@ enum {
 	TCA_NETEM_REORDER,
 	TCA_NETEM_CORRUPT,
 	TCA_NETEM_LOSS,
+	TCA_NETEM_RATELATENCY,
 	__TCA_NETEM_MAX,
 };
 
@@ -495,6 +496,13 @@ struct tc_netem_corrupt {
 	__u32	correlation;
 };
 
+struct tc_netem_ratelatency {
+	__u32	ratelatency;	 /* byte/s */
+	__s32   packet_overhead;
+	__u32   cell_size;
+	__s32   cell_overhead;
+};
+
 enum {
 	NETEM_LOSS_UNSPEC,
 	NETEM_LOSS_GI,		/* General Intuitive - 4 state model */
diff --git a/tc/q_netem.c b/tc/q_netem.c
index 6dc40bd..d6a3266 100644
--- a/tc/q_netem.c
+++ b/tc/q_netem.c
@@ -34,7 +34,8 @@ static void explain(void)
 "                 [ drop PERCENT [CORRELATION]] \n" \
 "                 [ corrupt PERCENT [CORRELATION]] \n" \
 "                 [ duplicate PERCENT [CORRELATION]]\n" \
-"                 [ reorder PRECENT [CORRELATION] [ gap DISTANCE ]]\n");
+"                 [ reorder PRECENT [CORRELATION] [ gap DISTANCE ]]\n" \
+"                 [ ratelatency RATE [PACKETOVERHEAD] [CELLSIZE] [CELLOVERHEAD]]\n");
 }
 
 static void explain1(const char *arg)
@@ -131,6 +132,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	struct tc_netem_corr cor;
 	struct tc_netem_reorder reorder;
 	struct tc_netem_corrupt corrupt;
+	struct tc_netem_ratelatency ratelatency;
 	__s16 *dist_data = NULL;
 	int present[__TCA_NETEM_MAX];
 
@@ -139,6 +141,7 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	memset(&cor, 0, sizeof(cor));
 	memset(&reorder, 0, sizeof(reorder));
 	memset(&corrupt, 0, sizeof(corrupt));
+	memset(&ratelatency, 0, sizeof(ratelatency));
 	memset(present, 0, sizeof(present));
 
 	while (argc > 0) {
@@ -244,6 +247,34 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 				free(dist_data);
 				return -1;
 			}
+		} else if (matches(*argv, "ratelatency") == 0) {
+			++present[TCA_NETEM_RATELATENCY];
+			NEXT_ARG();
+			if (get_rate(&ratelatency.ratelatency, *argv)) {
+				explain1("ratelatency");
+				return -1;
+			}
+			if (NEXT_IS_NUMBER()) {
+				NEXT_ARG();
+				if (get_s32(&ratelatency.packet_overhead, *argv, 0)) {
+					explain1("ratelatency");
+					return -1;
+				}
+			}
+			if (NEXT_IS_NUMBER()) {
+				NEXT_ARG();
+				if (get_u32(&ratelatency.cell_size, *argv, 0)) {
+					explain1("ratelatency");
+					return -1;
+				}
+			}
+			if (NEXT_IS_NUMBER()) {
+				NEXT_ARG();
+				if (get_s32(&ratelatency.cell_overhead, *argv, 0)) {
+					explain1("ratelatency");
+					return -1;
+				}
+			}
 		} else if (strcmp(*argv, "help") == 0) {
 			explain();
 			return -1;
@@ -290,6 +321,10 @@ static int netem_parse_opt(struct qdisc_util *qu, int argc, char **argv,
 	    addattr_l(n, 1024, TCA_NETEM_CORRUPT, &corrupt, sizeof(corrupt)) < 0)
 		return -1;
 
+	if (present[TCA_NETEM_RATELATENCY] &&
+	    addattr_l(n, 1024, TCA_NETEM_RATELATENCY, &ratelatency, sizeof(ratelatency)) < 0)
+		return -1;
+
 	if (dist_data) {
 		if (addattr_l(n, MAX_DIST * sizeof(dist_data[0]),
 			      TCA_NETEM_DELAY_DIST,
@@ -306,6 +341,7 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 	const struct tc_netem_corr *cor = NULL;
 	const struct tc_netem_reorder *reorder = NULL;
 	const struct tc_netem_corrupt *corrupt = NULL;
+	const struct tc_netem_ratelatency *ratelatency = NULL;
 	struct tc_netem_qopt qopt;
 	int len = RTA_PAYLOAD(opt) - sizeof(qopt);
 	SPRINT_BUF(b1);
@@ -339,6 +375,11 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 				return -1;
 			corrupt = RTA_DATA(tb[TCA_NETEM_CORRUPT]);
 		}
+		if (tb[TCA_NETEM_RATELATENCY]) {
+			if (RTA_PAYLOAD(tb[TCA_NETEM_RATELATENCY]) < sizeof(*ratelatency))
+				return -1;
+			ratelatency = RTA_DATA(tb[TCA_NETEM_RATELATENCY]);
+		}
 	}
 
 	fprintf(f, "limit %d", qopt.limit);
@@ -382,6 +423,16 @@ static int netem_print_opt(struct qdisc_util *qu, FILE *f, struct rtattr *opt)
 				sprint_percent(corrupt->correlation, b1));
 	}
 
+	if (ratelatency && ratelatency->ratelatency) {
+		fprintf(f, " ratelatency %s", sprint_rate(ratelatency->ratelatency, b1));
+		if (ratelatency->packet_overhead)
+			fprintf(f, " packetoverhead %d", ratelatency->packet_overhead);
+		if (ratelatency->cell_size)
+			fprintf(f, " cellsize %u", ratelatency->cell_size);
+		if (ratelatency->cell_overhead)
+			fprintf(f, " celloverhead %d", ratelatency->cell_overhead);
+	}
+
 	if (qopt.gap)
 		fprintf(f, " gap %lu", (unsigned long)qopt.gap);
 
-- 
1.7.7

RE: [PATCH iproute2 1/2] utils: add s32 parser

[David Laight]

> +int get_s32(__s32 *val, const char *arg, int base)
> +{
> +	long res;
> +	char *ptr;
> +
> +	if (!arg || !*arg)
> +		return -1;

No need to check *arg, picked up below.

> +	res = strtoul(arg, &ptr, base);
> +	if (!ptr || ptr == arg || *ptr || res > INT32_MAX || res <
INT32_MIN)

No need to check !ptr.

> +		return -1;
> +	*val = res;
> +	return 0;
> +}

Seems to me this be commoned with get_s16() by using
a function that takes the bounds as parameters.
Or possibly with a wrapper to strtoul() that does all the
validation checks.

	David

Re: [PATCH iproute2 1/2] utils: add s32 parser

[Hagen Paul Pfeifer]

* David Laight | 2011-11-25 09:46:09 [-0000]:

>> +int get_s32(__s32 *val, const char *arg, int base)
>> +{
>> +	long res;
>> +	char *ptr;
>> +
>> +	if (!arg || !*arg)
>> +		return -1;
>
>No need to check *arg, picked up below.

Yes, it is a little bit explicit/duplicate, but I decided to follow all
get_[su][16 32 64] functions to be consistent. If you want you can send a
separate patch afterwards.

HGN

Re: [PATCH iproute2 1/2] utils: add s32 parser

[Stephen Hemminger]

On Fri, 25 Nov 2011 09:46:09 -0000
"David Laight" <David.Laight@ACULAB.COM> wrote:

> > +	res = strtoul(arg, &ptr, base);
> > +	if (!ptr || ptr == arg || *ptr || res > INT32_MAX || res <  
> INT32_MIN)
> 
> No need to check !ptr.

Also don't you want signed value?  Reading strtol() man page,
the correct way is:
	errno = 0;
	res = strtol(arg, &ptr, base);
	if (ptr == arg || errno)
		return -1;

"RETURN VALUE
       The strtol() function returns the result of the conversion, unless  the
       value  would  underflow  or overflow.  If an underflow occurs, strtol()
       returns LONG_MIN.  If an overflow occurs,  strtol()  returns  LONG_MAX.
       In  both  cases,  errno is set to ERANGE.  Precisely the same holds for
       strtoll()  (with  LLONG_MIN  and  LLONG_MAX  instead  of  LONG_MIN  and
       LONG_MAX).

RE: [PATCH iproute2 1/2] utils: add s32 parser

[David Laight]

> From: Stephen Hemminger [mailto:shemminger@vyatta.com] 
> "David Laight" <David.Laight@ACULAB.COM> wrote:
> 
> > > +	res = strtoul(arg, &ptr, base);
> > > +	if (!ptr || ptr == arg || *ptr || res > INT32_MAX || res <  
> > INT32_MIN)
> > 
> > No need to check !ptr.
> 
> Also don't you want signed value?  Reading strtol() man page,
> the correct way is:
> 	errno = 0;
> 	res = strtol(arg, &ptr, base);
> 	if (ptr == arg || errno)
> 		return -1;
> 
> "RETURN VALUE
>        The strtol() function returns the result of the conversion,
unless  the
>        value  would  underflow  or overflow.  If an underflow occurs,
strtol()
>        returns LONG_MIN.  If an overflow occurs,  strtol() returns
LONG_MAX.
>        In  both  cases,  errno is set to ERANGE.  Precisely the same
holds for
>        strtoll()  (with  LLONG_MIN  and  LLONG_MAX  instead  of
LONG_MIN  and
>        LONG_MAX).

If you are that worried about numeric overflow (IIRC) you have
have to check the result for LONG_MIN/MAX (etc) before looking
at errno.

strtoul() is defined to support -ve values, and I think the
C rules for conversion between signed and unsigned ints
DTRT even for non 2's compliment systems.

Some of these bound checks are a waste of time.
The SUS doesn't require standard utilities to perform them.

	David

Re: [PATCH iproute2 1/2] utils: add s32 parser

[Hagen Paul Pfeifer]

* David Laight | 2011-11-25 17:34:21 [-0000]:

>If you are that worried about numeric overflow (IIRC) you have
>have to check the result for LONG_MIN/MAX (etc) before looking
>at errno.
>
>strtoul() is defined to support -ve values, and I think the
>C rules for conversion between signed and unsigned ints
>DTRT even for non 2's compliment systems.
>
>Some of these bound checks are a waste of time.
>The SUS doesn't require standard utilities to perform them.

David: are you able to fix all conversations functions in utils.c (and add
get_s32). If not I will repost get_s32 with strtol() with the same error check
mechanism as get_* (to be consistent).

Stephen, any other ideas?

Hagen

[PATCH iproute2] utils: add s32 parser

[Hagen Paul Pfeifer]

This should be enough "security", tc users have root capabilities
anyway.

Not sure if we should save errno and restore afterwards:

errno_save = errno;
errno = 0;
[...]
errno = errno_save;


Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
---

 include/utils.h |    1 +
 lib/utils.c     |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 47f8e07..496db68 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -85,6 +85,7 @@ extern int get_time_rtt(unsigned *val, const char *arg, int *raw);
 #define get_short get_s16
 extern int get_u64(__u64 *val, const char *arg, int base);
 extern int get_u32(__u32 *val, const char *arg, int base);
+extern int get_s32(__s32 *val, const char *arg, int base);
 extern int get_u16(__u16 *val, const char *arg, int base);
 extern int get_s16(__s16 *val, const char *arg, int base);
 extern int get_u8(__u8 *val, const char *arg, int base);
diff --git a/lib/utils.c b/lib/utils.c
index efaf377..9cd45e4 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -25,6 +25,7 @@
 #include <linux/pkt_sched.h>
 #include <time.h>
 #include <sys/time.h>
+#include <errno.h>
 
 
 #include "utils.h"
@@ -198,6 +199,24 @@ int get_u8(__u8 *val, const char *arg, int base)
 	return 0;
 }
 
+int get_s32(__s32 *val, const char *arg, int base)
+{
+	long res;
+	char *ptr;
+
+	errno = 0;
+
+	if (!arg || !*arg)
+		return -1;
+	res = strtol(arg, &ptr, base);
+	if (ptr == arg || *ptr ||
+	    ((res =  LONG_MIN || res == LONG_MAX) && errno == ERANGE) ||
+	    res > INT32_MAX || res < INT32_MIN)
+		return -1;
+	*val = res;
+	return 0;
+}
+
 int get_s16(__s16 *val, const char *arg, int base)
 {
 	long res;
-- 
1.7.7

Re: [PATCH iproute2] utils: add s32 parser

[Stephen Hemminger]

On Sat, 26 Nov 2011 00:00:26 +0100
Hagen Paul Pfeifer <hagen@jauu.net> wrote:

> Not sure if we should save errno and restore afterwards:
> 
> errno_save = errno;
> errno = 0;
> [...]
> errno = errno_save;

Not worth it. errno is only meaningful after a failed library or syscall.

[PATCH iproute2] utils: add s32 parser

[Hagen Paul Pfeifer]

Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>
---
 include/utils.h |    1 +
 lib/utils.c     |   19 +++++++++++++++++++
 2 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/include/utils.h b/include/utils.h
index 47f8e07..496db68 100644
--- a/include/utils.h
+++ b/include/utils.h
@@ -85,6 +85,7 @@ extern int get_time_rtt(unsigned *val, const char *arg, int *raw);
 #define get_short get_s16
 extern int get_u64(__u64 *val, const char *arg, int base);
 extern int get_u32(__u32 *val, const char *arg, int base);
+extern int get_s32(__s32 *val, const char *arg, int base);
 extern int get_u16(__u16 *val, const char *arg, int base);
 extern int get_s16(__s16 *val, const char *arg, int base);
 extern int get_u8(__u8 *val, const char *arg, int base);
diff --git a/lib/utils.c b/lib/utils.c
index efaf377..d80f79b 100644
--- a/lib/utils.c
+++ b/lib/utils.c
@@ -25,6 +25,7 @@
 #include <linux/pkt_sched.h>
 #include <time.h>
 #include <sys/time.h>
+#include <errno.h>
 
 
 #include "utils.h"
@@ -198,6 +199,24 @@ int get_u8(__u8 *val, const char *arg, int base)
 	return 0;
 }
 
+int get_s32(__s32 *val, const char *arg, int base)
+{
+	long res;
+	char *ptr;
+
+	errno = 0;
+
+	if (!arg || !*arg)
+		return -1;
+	res = strtol(arg, &ptr, base);
+	if (ptr == arg || *ptr ||
+	    ((res ==  LONG_MIN || res == LONG_MAX) && errno == ERANGE) ||
+	    res > INT32_MAX || res < INT32_MIN)
+		return -1;
+	*val = res;
+	return 0;
+}
+
 int get_s16(__s16 *val, const char *arg, int base)
 {
 	long res;
-- 
1.7.7

Re: [PATCH iproute2 1/2] utils: add s32 parser

[Stephen Hemminger]

On Thu, 24 Nov 2011 18:40:14 +0100
Hagen Paul Pfeifer <hagen@jauu.net> wrote:

> Signed-off-by: Hagen Paul Pfeifer <hagen@jauu.net>

All three patches applied to iproute2 git. Had to fix some merge conflict
and man page formatting issues.

Re: [PATCH iproute2 1/2] utils: add s32 parser

[Hagen Paul Pfeifer]

* Stephen Hemminger | 2012-01-19 14:41:38 [-0800]:

>All three patches applied to iproute2 git. Had to fix some merge conflict
>and man page formatting issues.

Thank you Stephen! I already rebased this patchset in my branch (mainly netem
manpage changes. To late ... ;-)