| Message ID | 19073d9bc5a2977a5a366caf5e06b392e4b63e54.1587575157.git.petrm@mellanox.com |
|---|---|
| State | Superseded |
| Delegated to: | David Ahern |
| Headers | show |
| Series | [iproute2-next] tc: pedit: Support JSON dumping | expand |
On Wed, 22 Apr 2020 20:06:15 +0300 Petr Machata <petrm@mellanox.com> wrote: > + print_string(PRINT_FP, NULL, ": %s", > + cmd ? "add" : "val"); > + print_string(PRINT_JSON, "cmd", NULL, > + cmd ? "add" : "set"); Having different outputs for JSON and file here. Is that necessary? JSON output is new, and could just mirror existing usage.
Stephen Hemminger <stephen@networkplumber.org> writes: > On Wed, 22 Apr 2020 20:06:15 +0300 > Petr Machata <petrm@mellanox.com> wrote: > >> + print_string(PRINT_FP, NULL, ": %s", >> + cmd ? "add" : "val"); >> + print_string(PRINT_JSON, "cmd", NULL, >> + cmd ? "add" : "set"); > > Having different outputs for JSON and file here. Is that necessary? > JSON output is new, and could just mirror existing usage. This code outputs this bit: { "htype": "udp", "offset": 0, "cmd": "set", <---- "val": "3039", "mask": "ffff0000" }, There are currently two commands, set and add. The words used to configure these actions are set and add as well. The way these commands are dumped should be the same, too. The only reason why "set" is reported as "val" in file is that set used to be the implied action. JSON doesn't have to be backward compatible, so it should present the expected words.
On 4/23/20 3:59 AM, Petr Machata wrote: > > Stephen Hemminger <stephen@networkplumber.org> writes: > >> On Wed, 22 Apr 2020 20:06:15 +0300 >> Petr Machata <petrm@mellanox.com> wrote: >> >>> + print_string(PRINT_FP, NULL, ": %s", >>> + cmd ? "add" : "val"); >>> + print_string(PRINT_JSON, "cmd", NULL, >>> + cmd ? "add" : "set"); >> >> Having different outputs for JSON and file here. Is that necessary? >> JSON output is new, and could just mirror existing usage. > > This code outputs this bit: > > { > "htype": "udp", > "offset": 0, > "cmd": "set", <---- > "val": "3039", > "mask": "ffff0000" > }, > > There are currently two commands, set and add. The words used to > configure these actions are set and add as well. The way these commands > are dumped should be the same, too. The only reason why "set" is > reported as "val" in file is that set used to be the implied action. > > JSON doesn't have to be backward compatible, so it should present the > expected words. > Stephen: do you agree?
On Sun, 26 Apr 2020 12:23:04 -0600 David Ahern <dsahern@gmail.com> wrote: > On 4/23/20 3:59 AM, Petr Machata wrote: > > > > Stephen Hemminger <stephen@networkplumber.org> writes: > > > >> On Wed, 22 Apr 2020 20:06:15 +0300 > >> Petr Machata <petrm@mellanox.com> wrote: > >> > >>> + print_string(PRINT_FP, NULL, ": %s", > >>> + cmd ? "add" : "val"); > >>> + print_string(PRINT_JSON, "cmd", NULL, > >>> + cmd ? "add" : "set"); > >> > >> Having different outputs for JSON and file here. Is that necessary? > >> JSON output is new, and could just mirror existing usage. > > > > This code outputs this bit: > > > > { > > "htype": "udp", > > "offset": 0, > > "cmd": "set", <---- > > "val": "3039", > > "mask": "ffff0000" > > }, > > > > There are currently two commands, set and add. The words used to > > configure these actions are set and add as well. The way these commands > > are dumped should be the same, too. The only reason why "set" is > > reported as "val" in file is that set used to be the implied action. > > > > JSON doesn't have to be backward compatible, so it should present the > > expected words. > > > > Stephen: do you agree? Sure that is fine, maybe a comment would help?
Stephen Hemminger <stephen@networkplumber.org> writes: > On Sun, 26 Apr 2020 12:23:04 -0600 > David Ahern <dsahern@gmail.com> wrote: > >> On 4/23/20 3:59 AM, Petr Machata wrote: >> > >> > Stephen Hemminger <stephen@networkplumber.org> writes: >> > >> >> On Wed, 22 Apr 2020 20:06:15 +0300 >> >> Petr Machata <petrm@mellanox.com> wrote: >> >> >> >>> + print_string(PRINT_FP, NULL, ": %s", >> >>> + cmd ? "add" : "val"); >> >>> + print_string(PRINT_JSON, "cmd", NULL, >> >>> + cmd ? "add" : "set"); >> >> >> >> Having different outputs for JSON and file here. Is that necessary? >> >> JSON output is new, and could just mirror existing usage. >> > >> > This code outputs this bit: >> > >> > { >> > "htype": "udp", >> > "offset": 0, >> > "cmd": "set", <---- >> > "val": "3039", >> > "mask": "ffff0000" >> > }, >> > >> > There are currently two commands, set and add. The words used to >> > configure these actions are set and add as well. The way these commands >> > are dumped should be the same, too. The only reason why "set" is >> > reported as "val" in file is that set used to be the implied action. >> > >> > JSON doesn't have to be backward compatible, so it should present the >> > expected words. >> > >> >> Stephen: do you agree? > > Sure that is fine, maybe a comment would help? Something like this? /* In FP, report the "set" command as "val" to keep * backward compatibility. */ print_string(PRINT_FP, NULL, ": %s", cmd ? "add" : "val"); print_string(PRINT_JSON, "cmd", NULL, cmd ? "add" : "set");
Petr Machata <petrm@mellanox.com> writes: > Stephen Hemminger <stephen@networkplumber.org> writes: > >> On Sun, 26 Apr 2020 12:23:04 -0600 >> David Ahern <dsahern@gmail.com> wrote: >> >>> On 4/23/20 3:59 AM, Petr Machata wrote: >>> > >>> > Stephen Hemminger <stephen@networkplumber.org> writes: >>> > >>> >> On Wed, 22 Apr 2020 20:06:15 +0300 >>> >> Petr Machata <petrm@mellanox.com> wrote: >>> >> >>> >>> + print_string(PRINT_FP, NULL, ": %s", >>> >>> + cmd ? "add" : "val"); >>> >>> + print_string(PRINT_JSON, "cmd", NULL, >>> >>> + cmd ? "add" : "set"); >>> >> >>> >> Having different outputs for JSON and file here. Is that necessary? >>> >> JSON output is new, and could just mirror existing usage. >>> > >>> > This code outputs this bit: >>> > >>> > { >>> > "htype": "udp", >>> > "offset": 0, >>> > "cmd": "set", <---- >>> > "val": "3039", >>> > "mask": "ffff0000" >>> > }, >>> > >>> > There are currently two commands, set and add. The words used to >>> > configure these actions are set and add as well. The way these commands >>> > are dumped should be the same, too. The only reason why "set" is >>> > reported as "val" in file is that set used to be the implied action. >>> > >>> > JSON doesn't have to be backward compatible, so it should present the >>> > expected words. >>> > >>> >>> Stephen: do you agree? >> >> Sure that is fine, maybe a comment would help? > > Something like this? > > /* In FP, report the "set" command as "val" to keep > * backward compatibility. > */ > print_string(PRINT_FP, NULL, ": %s", > cmd ? "add" : "val"); > print_string(PRINT_JSON, "cmd", NULL, > cmd ? "add" : "set"); I just sent it as a v2 of the patch, we can discuss there.
diff --git a/tc/m_pedit.c b/tc/m_pedit.c index fccfd17c..d3aa08de 100644 --- a/tc/m_pedit.c +++ b/tc/m_pedit.c @@ -714,20 +714,28 @@ static const char * const pedit_htype_str[] = { [TCA_PEDIT_KEY_EX_HDR_TYPE_UDP] = "udp", }; -static void print_pedit_location(FILE *f, - enum pedit_header_type htype, __u32 off) +static int print_pedit_location(FILE *f, + enum pedit_header_type htype, __u32 off) { - if (htype == TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK) { - fprintf(f, "%d", (unsigned int)off); - return; + char *buf = NULL; + int rc; + + if (htype != TCA_PEDIT_KEY_EX_HDR_TYPE_NETWORK) { + if (htype < ARRAY_SIZE(pedit_htype_str)) + rc = asprintf(&buf, "%s", pedit_htype_str[htype]); + else + rc = asprintf(&buf, "unknown(%d)", htype); + if (rc < 0) + return rc; + print_string(PRINT_ANY, "htype", "%s", buf); + print_int(PRINT_ANY, "offset", "%+d", off); + } else { + print_string(PRINT_JSON, "htype", NULL, "network"); + print_int(PRINT_ANY, "offset", "%d", off); } - if (htype < ARRAY_SIZE(pedit_htype_str)) - fprintf(f, "%s", pedit_htype_str[htype]); - else - fprintf(f, "unknown(%d)", htype); - - fprintf(f, "%c%d", (int)off >= 0 ? '+' : '-', abs((int)off)); + free(buf); + return 0; } static int print_pedit(struct action_util *au, FILE *f, struct rtattr *arg) @@ -735,6 +743,7 @@ static int print_pedit(struct action_util *au, FILE *f, struct rtattr *arg) struct tc_pedit_sel *sel; struct rtattr *tb[TCA_PEDIT_MAX + 1]; struct m_pedit_key_ex *keys_ex = NULL; + int err; if (arg == NULL) return -1; @@ -774,11 +783,12 @@ static int print_pedit(struct action_util *au, FILE *f, struct rtattr *arg) } } - fprintf(f, " pedit "); + print_string(PRINT_ANY, "kind", " %s ", "pedit"); print_action_control(f, "action ", sel->action, " "); - fprintf(f,"keys %d\n ", sel->nkeys); - fprintf(f, "\t index %u ref %d bind %d", sel->index, sel->refcnt, - sel->bindcnt); + print_uint(PRINT_ANY, "nkeys", "keys %d\n", sel->nkeys); + print_uint(PRINT_ANY, "index", " \t index %u", sel->index); + print_int(PRINT_ANY, "ref", " ref %d", sel->refcnt); + print_int(PRINT_ANY, "bind", " bind %d", sel->bindcnt); if (show_stats) { if (tb[TCA_PEDIT_TM]) { @@ -787,6 +797,7 @@ static int print_pedit(struct action_util *au, FILE *f, struct rtattr *arg) print_tm(f, tm); } } + open_json_array(PRINT_JSON, "keys"); if (sel->nkeys) { int i; struct tc_pedit_key *key = sel->keys; @@ -804,21 +815,28 @@ static int print_pedit(struct action_util *au, FILE *f, struct rtattr *arg) key_ex++; } - fprintf(f, "\n\t key #%d", i); + open_json_object(NULL); + print_uint(PRINT_FP, NULL, "\n\t key #%d at ", i); - fprintf(f, " at "); + err = print_pedit_location(f, htype, key->off); + if (err) + return err; - print_pedit_location(f, htype, key->off); - - fprintf(f, ": %s %08x mask %08x", - cmd ? "add" : "val", - (unsigned int)ntohl(key->val), - (unsigned int)ntohl(key->mask)); + print_string(PRINT_FP, NULL, ": %s", + cmd ? "add" : "val"); + print_string(PRINT_JSON, "cmd", NULL, + cmd ? "add" : "set"); + print_hex(PRINT_ANY, "val", " %08x", + (unsigned int)ntohl(key->val)); + print_hex(PRINT_ANY, "mask", " mask %08x", + (unsigned int)ntohl(key->mask)); + close_json_object(); } } else { fprintf(f, "\npedit %x keys %d is not LEGIT", sel->index, sel->nkeys); } + close_json_array(PRINT_JSON, " "); print_nl();
The action pedit does not currently support dumping to JSON. Convert print_pedit() to the print_* family of functions so that dumping is correct both in plain and JSON mode. In plain mode, the output is character for character the same as it was before. In JSON mode, this is an example dump: $ tc filter add dev dummy0 ingress prio 125 flower \ action pedit ex munge udp dport set 12345 \ munge ip ttl add 1 \ munge offset 10 u8 clear $ tc -j filter show dev dummy0 ingress | jq [ { "protocol": "all", "pref": 125, "kind": "flower", "chain": 0 }, { "protocol": "all", "pref": 125, "kind": "flower", "chain": 0, "options": { "handle": 1, "keys": {}, "not_in_hw": true, "actions": [ { "order": 1, "kind": "pedit", "control_action": { "type": "pass" }, "nkeys": 3, "index": 1, "ref": 1, "bind": 1, "keys": [ { "htype": "udp", "offset": 0, "cmd": "set", "val": "3039", "mask": "ffff0000" }, { "htype": "ipv4", "offset": 8, "cmd": "add", "val": "1000000", "mask": "ffffff" }, { "htype": "network", "offset": 8, "cmd": "set", "val": "0", "mask": "ffff00ff" } ] } ] } } ] Signed-off-by: Petr Machata <petrm@mellanox.com> --- tc/m_pedit.c | 64 +++++++++++++++++++++++++++++++++------------------- 1 file changed, 41 insertions(+), 23 deletions(-)