| Message ID | 20240123161115.69729-1-victor@mojatatu.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next] m_mirred: Allow mirred to block | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Tue, 23 Jan 2024 13:11:15 -0300
Victor Nogueira <victor@mojatatu.com> wrote:
> + print_string(PRINT_ANY, "to_dev", " to device %s)", dev);
Suggestion for future.
Use colorized device name here.
print_color_string(PRINT_ANY, COLOR_IFNAME, "to_dev", " to device %s)", dev);
On 23/01/2024 14:18, Stephen Hemminger wrote: > On Tue, 23 Jan 2024 13:11:15 -0300 > Victor Nogueira <victor@mojatatu.com> wrote: > >> + print_string(PRINT_ANY, "to_dev", " to device %s)", dev); > > Suggestion for future. > Use colorized device name here. > print_color_string(PRINT_ANY, COLOR_IFNAME, "to_dev", " to device %s)", dev); Thank you for the suggestion. Will send a patch after this one to fix this. cheers, Victor
On 1/23/24 9:11 AM, Victor Nogueira wrote: > --- > tc/m_mirred.c | 60 +++++++++++++++++++++++++++++++++++++++++---------- missing the man page update > 1 file changed, 49 insertions(+), 11 deletions(-) > > diff --git a/tc/m_mirred.c b/tc/m_mirred.c > index e5653e67f..db847b1a3 100644 > --- a/tc/m_mirred.c > +++ b/tc/m_mirred.c > @@ -162,15 +167,38 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, > TCA_INGRESS_REDIR; > p.action = TC_ACT_STOLEN; > ok++; > - } else if ((redir || mirror) && > - matches(*argv, "dev") == 0) { > - NEXT_ARG(); > - if (strlen(d)) > - duparg("dev", *argv); > - > - strncpy(d, *argv, sizeof(d)-1); > - argc--; > - argv++; > + } else if ((redir || mirror)) { > + if (matches(*argv, "blockid") == 0) { Not accepting any more uses of matches. > + if (strlen(d)) { > + fprintf(stderr, > + "Mustn't specify blockid and dev simultaneously\n"); > + return -1; > + } > + NEXT_ARG(); > + if (get_u32(&blockid, *argv, 0) || > + !blockid) { > + fprintf(stderr, > + "invalid block ID index value %s", > + *argv); > + return -1; > + } > + argc--; > + argv++; > + } > + if (argc && matches(*argv, "dev") == 0) { This one is legacy, but I really would like to see it as 'dev' (versus just 'd' or 'de'). Such a change is risky from a backwards compatibility, so I guess we are stuck with it. > + if (blockid) { > + fprintf(stderr, > + "Mustn't specify blockid and dev simultaneously\n"); > + return -1; > + } > + NEXT_ARG(); > + if (strlen(d)) > + duparg("dev", *argv); > + > + strncpy(d, *argv, sizeof(d)-1); > + argc--; > + argv++; > + } > > break; >
On 23/01/2024 15:17, David Ahern wrote: > On 1/23/24 9:11 AM, Victor Nogueira wrote: >> --- >> tc/m_mirred.c | 60 +++++++++++++++++++++++++++++++++++++++++---------- > > missing the man page update > >> [...] >> diff --git a/tc/m_mirred.c b/tc/m_mirred.c >> index e5653e67f..db847b1a3 100644 >> --- a/tc/m_mirred.c >> +++ b/tc/m_mirred.c >> @@ -162,15 +167,38 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, >> TCA_INGRESS_REDIR; >> p.action = TC_ACT_STOLEN; >> ok++; >> - } else if ((redir || mirror) && >> - matches(*argv, "dev") == 0) { >> - NEXT_ARG(); >> - if (strlen(d)) >> - duparg("dev", *argv); >> - >> - strncpy(d, *argv, sizeof(d)-1); >> - argc--; >> - argv++; >> + } else if ((redir || mirror)) { >> + if (matches(*argv, "blockid") == 0) { > > Not accepting any more uses of matches. > [...] Thank you for the catches. Sent a v2 fixing these issues. cheers, Victor
diff --git a/tc/m_mirred.c b/tc/m_mirred.c index e5653e67f..db847b1a3 100644 --- a/tc/m_mirred.c +++ b/tc/m_mirred.c @@ -29,7 +29,11 @@ explain(void) "\tDIRECTION := <ingress | egress>\n" "\tACTION := <mirror | redirect>\n" "\tINDEX is the specific policy instance id\n" - "\tDEVICENAME is the devicename\n"); + "\tTARGET := <BLOCK | DEVICE>\n" + "\tDEVICE := dev DEVICENAME\n" + "\tDEVICENAME is the devicename\n" + "\tBLOCK := blockid BLOCKID\n" + "\tBLOCKID := 32-bit unsigned block ID\n"); } static void @@ -94,6 +98,7 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, struct tc_mirred p = {}; struct rtattr *tail; char d[IFNAMSIZ] = {}; + __u32 blockid = 0; while (argc > 0) { @@ -162,15 +167,38 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, TCA_INGRESS_REDIR; p.action = TC_ACT_STOLEN; ok++; - } else if ((redir || mirror) && - matches(*argv, "dev") == 0) { - NEXT_ARG(); - if (strlen(d)) - duparg("dev", *argv); - - strncpy(d, *argv, sizeof(d)-1); - argc--; - argv++; + } else if ((redir || mirror)) { + if (matches(*argv, "blockid") == 0) { + if (strlen(d)) { + fprintf(stderr, + "Mustn't specify blockid and dev simultaneously\n"); + return -1; + } + NEXT_ARG(); + if (get_u32(&blockid, *argv, 0) || + !blockid) { + fprintf(stderr, + "invalid block ID index value %s", + *argv); + return -1; + } + argc--; + argv++; + } + if (argc && matches(*argv, "dev") == 0) { + if (blockid) { + fprintf(stderr, + "Mustn't specify blockid and dev simultaneously\n"); + return -1; + } + NEXT_ARG(); + if (strlen(d)) + duparg("dev", *argv); + + strncpy(d, *argv, sizeof(d)-1); + argc--; + argv++; + } break; @@ -220,6 +248,8 @@ parse_direction(struct action_util *a, int *argc_p, char ***argv_p, tail = addattr_nest(n, MAX_MSG, tca_id); addattr_l(n, MAX_MSG, TCA_MIRRED_PARMS, &p, sizeof(p)); + if (blockid) + addattr32(n, MAX_MSG, TCA_MIRRED_BLOCKID, blockid); addattr_nest_end(n, tail); *argc_p = argc; @@ -299,7 +329,15 @@ print_mirred(struct action_util *au, FILE *f, struct rtattr *arg) mirred_action(p->eaction)); print_string(PRINT_JSON, "direction", NULL, mirred_direction(p->eaction)); - print_string(PRINT_ANY, "to_dev", " to device %s)", dev); + if (tb[TCA_MIRRED_BLOCKID]) { + const __u32 *blockid = RTA_DATA(tb[TCA_MIRRED_BLOCKID]); + + print_uint(PRINT_ANY, "to_blockid", " to blockid %u)", + *blockid); + } else { + print_string(PRINT_ANY, "to_dev", " to device %s)", dev); + } + print_action_control(f, " ", p->action, ""); print_nl();