Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
On 8/2/2017 11:41 AM, Roopa Prabhu wrote: > On Mon, Jul 31, 2017 at 5:40 PM, Amritha Nambiar >wrote: >> The Mirred/redirect action is extended to accept a traffic >> class on the device in addition to the device's ifindex. >> >> Usage: mirred >> >> Example: >> # tc qdisc add dev eth0 ingress >> >> # tc filter add dev eth0 protocol ip parent : prio 1 flower\ >> dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\ >> indev eth0 action mirred ingress redirect dev eth0 tc 1 > > > Can we call the new parameter tclass or something else ?. > with this 'tc' appears twice in the command :) > Sounds right. I was already thinking of alternatives like 'tcqgroup', 'qgroup' and now we have 'tclass'.
Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
On Mon, Jul 31, 2017 at 5:40 PM, Amritha Nambiarwrote: > The Mirred/redirect action is extended to accept a traffic > class on the device in addition to the device's ifindex. > > Usage: mirred > > Example: > # tc qdisc add dev eth0 ingress > > # tc filter add dev eth0 protocol ip parent : prio 1 flower\ > dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\ > indev eth0 action mirred ingress redirect dev eth0 tc 1 Can we call the new parameter tclass or something else ?. with this 'tc' appears twice in the command :)
Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
On 8/1/2017 8:12 AM, David Laight wrote: > From: Stephen Hemminger >> Sent: 01 August 2017 04:52 >> On Mon, 31 Jul 2017 17:40:50 -0700 >> Amritha Nambiarwrote: >> The concept is fine, bu t the code looks different than the rest which >> is never a good sign. >> >> >>> + if ((argc > 0) && (matches(*argv, "tc") == 0)) { >> >> Extra () are unnecessary in compound conditional. >> >>> + tc = atoi(*argv); >> >> Prefer using strtoul since it has better error handling than atoi() >> >>> + argc--; >>> + argv++; >>> + } >> >> >> Use NEXT_ARG() construct like rest of the code. > > Why bother faffing about with argc at all? > The argument list terminates when *argv == NULL. > I'll submit the next version with these fixes. > David >
RE: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
From: Stephen Hemminger > Sent: 01 August 2017 04:52 > On Mon, 31 Jul 2017 17:40:50 -0700 > Amritha Nambiarwrote: > The concept is fine, bu t the code looks different than the rest which > is never a good sign. > > > > + if ((argc > 0) && (matches(*argv, "tc") == 0)) { > > Extra () are unnecessary in compound conditional. > > > + tc = atoi(*argv); > > Prefer using strtoul since it has better error handling than atoi() > > > + argc--; > > + argv++; > > + } > > > Use NEXT_ARG() construct like rest of the code. Why bother faffing about with argc at all? The argument list terminates when *argv == NULL. David
Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
Amritha Nambiarwrites: [...] > @@ -72,6 +73,8 @@ parse_direction(struct action_util *a, int *argc_p, char > ***argv_p, > struct tc_mirred p = {}; > struct rtattr *tail; > char d[16] = {}; > + __u32 flags = 0; > + __u8 tc; > > while (argc > 0) { > > @@ -142,6 +145,18 @@ parse_direction(struct action_util *a, int *argc_p, char > ***argv_p, > argc--; > argv++; > > + if ((argc > 0) && (matches(*argv, "tc") == 0)) { > + NEXT_ARG(); > + tc = atoi(*argv); Probably better to use strtol() instead, somebody wants to specify hex base, also it has stronger error checks. > + if (tc >= MIRRED_TC_MAP_MAX) { > + fprintf(stderr, "Invalid TC > index\n"); > + return -1; > + } > + flags |= MIRRED_F_TC_MAP; > + ok++; > + argc--; > + argv++; > + } > break; > > } > @@ -193,6 +208,9 @@ parse_direction(struct action_util *a, int *argc_p, char > ***argv_p, > tail = NLMSG_TAIL(n); > addattr_l(n, MAX_MSG, tca_id, NULL, 0); > addattr_l(n, MAX_MSG, TCA_MIRRED_PARMS, , sizeof(p)); > + if (flags & MIRRED_F_TC_MAP) > + addattr_l(n, MAX_MSG, TCA_MIRRED_TC_MAP, > + , sizeof(tc)); > tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail; > > *argc_p = argc; > @@ -248,6 +266,7 @@ print_mirred(struct action_util *au, FILE * f, struct > rtattr *arg) > struct tc_mirred *p; > struct rtattr *tb[TCA_MIRRED_MAX + 1]; > const char *dev; > + __u8 *tc; > > if (arg == NULL) > return -1; > @@ -273,6 +292,11 @@ print_mirred(struct action_util *au, FILE * f, struct > rtattr *arg) > fprintf(f, "mirred (%s to device %s)", mirred_n2a(p->eaction), dev); > print_action_control(f, " ", p->action, ""); > > + if (tb[TCA_MIRRED_TC_MAP]) { > + tc = RTA_DATA(tb[TCA_MIRRED_TC_MAP]); > + fprintf(f, " tc %d", *tc); 'tc' is declared as __u8 so format should be %u > + } > + > fprintf(f, "\n "); > fprintf(f, "\tindex %u ref %d bind %d", p->index, p->refcnt, > p->bindcnt);
Re: [PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
On Mon, 31 Jul 2017 17:40:50 -0700 Amritha Nambiarwrote: The concept is fine, bu t the code looks different than the rest which is never a good sign. > + if ((argc > 0) && (matches(*argv, "tc") == 0)) { Extra () are unnecessary in compound conditional. > + tc = atoi(*argv); Prefer using strtoul since it has better error handling than atoi() > + argc--; > + argv++; > + } Use NEXT_ARG() construct like rest of the code.
[PATCH RFC, iproute2] tc/mirred: Extend the mirred/redirect action to accept additional traffic class parameter
The Mirred/redirect action is extended to accept a traffic class on the device in addition to the device's ifindex. Usage: mirred Example: # tc qdisc add dev eth0 ingress # tc filter add dev eth0 protocol ip parent : prio 1 flower\ dst_ip 192.168.1.1/32 ip_proto udp dst_port 22\ indev eth0 action mirred ingress redirect dev eth0 tc 1 Signed-off-by: Amritha Nambiar--- tc/m_mirred.c | 26 +- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/include/linux/tc_act/tc_mirred.h b/include/linux/tc_act/tc_mirred.h index 3d7a2b3..9a3aa61 100644 --- a/include/linux/tc_act/tc_mirred.h +++ b/include/linux/tc_act/tc_mirred.h @@ -9,6 +9,9 @@ #define TCA_EGRESS_MIRROR 2 /* mirror packet to EGRESS */ #define TCA_INGRESS_REDIR 3 /* packet redirect to INGRESS*/ #define TCA_INGRESS_MIRROR 4 /* mirror packet to INGRESS */ + +#define MIRRED_F_TC_MAP0x1 +#define MIRRED_TC_MAP_MAX 0x10 struct tc_mirred { tc_gen; @@ -21,6 +24,7 @@ enum { TCA_MIRRED_TM, TCA_MIRRED_PARMS, TCA_MIRRED_PAD, + TCA_MIRRED_TC_MAP, __TCA_MIRRED_MAX }; #define TCA_MIRRED_MAX (__TCA_MIRRED_MAX - 1) diff --git a/tc/m_mirred.c b/tc/m_mirred.c index 2384bda..1a18c6b 100644 --- a/tc/m_mirred.c +++ b/tc/m_mirred.c @@ -29,12 +29,13 @@ static void explain(void) { - fprintf(stderr, "Usage: mirred [index INDEX] \n"); + fprintf(stderr, "Usage: mirred [index INDEX] [tc TCINDEX]\n"); fprintf(stderr, "where:\n"); fprintf(stderr, "\tDIRECTION := \n"); fprintf(stderr, "\tACTION := \n"); fprintf(stderr, "\tINDEX is the specific policy instance id\n"); fprintf(stderr, "\tDEVICENAME is the devicename\n"); + fprintf(stderr, "\tTCINDEX is the traffic class index\n"); } @@ -72,6 +73,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, struct tc_mirred p = {}; struct rtattr *tail; char d[16] = {}; + __u32 flags = 0; + __u8 tc; while (argc > 0) { @@ -142,6 +145,18 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, argc--; argv++; + if ((argc > 0) && (matches(*argv, "tc") == 0)) { + NEXT_ARG(); + tc = atoi(*argv); + if (tc >= MIRRED_TC_MAP_MAX) { + fprintf(stderr, "Invalid TC index\n"); + return -1; + } + flags |= MIRRED_F_TC_MAP; + ok++; + argc--; + argv++; + } break; } @@ -193,6 +208,9 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, tail = NLMSG_TAIL(n); addattr_l(n, MAX_MSG, tca_id, NULL, 0); addattr_l(n, MAX_MSG, TCA_MIRRED_PARMS, , sizeof(p)); + if (flags & MIRRED_F_TC_MAP) + addattr_l(n, MAX_MSG, TCA_MIRRED_TC_MAP, + , sizeof(tc)); tail->rta_len = (void *) NLMSG_TAIL(n) - (void *) tail; *argc_p = argc; @@ -248,6 +266,7 @@ print_mirred(struct action_util *au, FILE * f, struct rtattr *arg) struct tc_mirred *p; struct rtattr *tb[TCA_MIRRED_MAX + 1]; const char *dev; + __u8 *tc; if (arg == NULL) return -1; @@ -273,6 +292,11 @@ print_mirred(struct action_util *au, FILE * f, struct rtattr *arg) fprintf(f, "mirred (%s to device %s)", mirred_n2a(p->eaction), dev); print_action_control(f, " ", p->action, ""); + if (tb[TCA_MIRRED_TC_MAP]) { + tc = RTA_DATA(tb[TCA_MIRRED_TC_MAP]); + fprintf(f, " tc %d", *tc); + } + fprintf(f, "\n "); fprintf(f, "\tindex %u ref %d bind %d", p->index, p->refcnt, p->bindcnt);