Re: [PATCH iproute2] tc: add print options to fix json output

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 29 Oct 2020 21:20:55 +0000
"Sharma, Puneet" <pusharma@akamai.com> wrote:

> I did provide an example to better explain what patch is doing.
> 
> Sorry for long paste.
> 
> So, with current implementation output of command:
> $ tc -s -d -j filter show dev <eth_name> ingress
> 
> would contain:
> [{
>         "protocol": "ip",
>         "pref": 20000,
>         "kind": "basic",
>         "chain": 0
>     },{
>         "protocol": "ip",
>         "pref": 20000,
>         "kind": "basic",
>         "chain": 0,
>         "options": {handle 0x1
>   (
>     cmp(u8 at 9 layer 1 eq 6)
>     OR cmp(u8 at 9 layer 1 eq 17)
>   ) AND ipset(sg-test-ipv4 src)
> 
>             "actions": [{
>                     "order": 1,
>                     "kind": "gact",
>                     "control_action": {
>                         "type": "pass"
>                     },
>                     "prob": {
>                         "random_type": "none",
>                         "control_action": {
>                             "type": "pass"
>                         },
>                         "val": 0
>                     },
>                     "index": 1,
>                     "ref": 1,
>                     "bind": 1,
>                     "installed": 2633,
>                     "last_used": 2633,
>                     "stats": {
>                         "bytes": 0,
>                         "packets": 0,
>                         "drops": 0,
>                         "overlimits": 0,
>                         "requeues": 0,
>                         "backlog": 0,
>                         "qlen": 0
>                     }
>                 }]
>         }
>     }
> ]
> 
> Clearly this is an invalid JSON. Look at “options"
> 
> 
> With patch it would look like:
> [{
>         "protocol": "ip",
>         "pref": 20000,
>         "kind": "basic",
>         "chain": 0
>     },{
>         "protocol": "ip",
>         "pref": 20000,
>         "kind": "basic",
>         "chain": 0,
>         "options": {
>             "handle": 1,
>             "ematch": "(cmp(u8 at 9 layer 1 eq 6)OR cmp(u8 at 9 layer 1 eq 17)) AND ipset(sg-test-ipv4 src)",
>             "actions": [{
>                     "order": 1,
>                     "kind": "gact",
>                     "control_action": {
>                         "type": "pass"
>                     },
>                     "prob": {
>                         "random_type": "none",
>                         "control_action": {
>                             "type": "pass"
>                         },
>                         "val": 0
>                     },
>                     "index": 1,
>                     "ref": 1,
>                     "bind": 1,
>                     "installed": 297829723,
>                     "last_used": 297829723,
>                     "stats": {
>                         "bytes": 0,
>                         "packets": 0,
>                         "drops": 0,
>                         "overlimits": 0,
>                         "requeues": 0,
>                         "backlog": 0,
>                         "qlen": 0
>                     }
>                 }]
>         }
>     }
> ]
> 
> Now it’s handling the “handle” and “ematch” inside “options" depending on context.
> 
> Hope it’s more clear now.
> 
> Thanks,
> ~Puneet.
> 
> > On Oct 29, 2020, at 4:17 PM, Stephen Hemminger <stephen@networkplumber.org> wrote:
> > 
> > On Wed, 28 Oct 2020 14:35:54 -0400
> > Puneet Sharma <pusharma@akamai.com> wrote:
> >   
> >> Currently, json for basic rules output does not produce correct json
> >> syntax. The following fixes were done to correct it for extended
> >> matches for use with "basic" filters.
> >> 
> >> tc/f_basic.c: replace fprintf with print_uint to support json output.
> >> fixing this prints "handle" tag correctly in json output.
> >> 
> >> tc/m_ematch.c: replace various fprintf with correct print.
> >> add new "ematch" tag for json output which represents
> >> "tc filter add ... basic match '()'" string. Added print_raw_string
> >> to print raw string instead of key value for json.
> >> 
> >> lib/json_writer.c: add jsonw_raw_string to print raw text in json.
> >> 
> >> lib/json_print.c: add print_color_raw_string to print string
> >> depending on context.
> >> 
> >> example:
> >> $ tc -s -d -j filter show dev <eth_name> ingress
> >> Before:
> >> ...
> >> "options": {handle 0x2
> >>  (
> >>    cmp(u8 at 9 layer 1 eq 6)
> >>    OR cmp(u8 at 9 layer 1 eq 17)
> >>  ) AND ipset(test-ipv4 src)
> >> 
> >>            "actions": [{
> >> ...
> >> 
> >> After:
> >> [{
> >> ...
> >> "options": {
> >>    "handle": 1,
> >>    "ematch": "(cmp(u8 at 9 layer 1 eq 6)OR cmp(u8 at 9 layer 1 eq 17)) AND ipset(test-ipv4 src)",
> >> ...
> >> ]
> >> 
> >> Signed-off-by: Puneet Sharma <pusharma@akamai.com>
> >> ---  
> > 
> > What is the point of introducing raw string?
> > The JSON standard says that string fields must use proper escapes.
> > 
> > Please  don't emit invalid JSON. It will break consumption by other libraries.  
> 


I agree that the existing output is wrong. But your patch introduces

+void jsonw_raw_string(json_writer_t *self, const char *value);

Why?

You should just use jsonw_string() which already handles things like special
characters in the string. In theory, there could be quotes or other characters
in the ematch string.