diff mbox series

[iproute2-next,master] bridge: fdb show: fix fdb entry state output for json context

Message ID 20200710005055.8439-1-julien@cumulusnetworks.com
State Changes Requested
Delegated to: stephen hemminger
Headers show
Series [iproute2-next,master] bridge: fdb show: fix fdb entry state output for json context | expand

Commit Message

Julien Fortin July 10, 2020, 12:50 a.m. UTC 
From: Julien Fortin <julien@cumulusnetworks.com>

bridge json fdb show is printing an incorrect / non-machine readable
value, when using -j (json output) we are expecting machine readable
data that shouldn't require special handling/parsing.

$ bridge -j fdb show | \
python -c \
'import sys,json;print(json.dumps(json.loads(sys.stdin.read()),indent=4))'
[
    {
        "master": "br0",
        "mac": "56:23:28:4f:4f:e5",
        "flags": [],
        "ifname": "vx0",
        "state": "state=0x80"  <<<<<<<<< with the patch: "state": "0x80"
    }
]

Fixes: c7c1a1ef51aea7c ("bridge: colorize output and use JSON print library")
Signed-off-by: Julien Fortin <julien@cumulusnetworks.com>
---
 bridge/fdb.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

Comments

Stephen Hemminger July 13, 2020, 2:54 p.m. UTC | #1 
On Fri, 10 Jul 2020 02:50:55 +0200
Julien Fortin <julien@cumulusnetworks.com> wrote:

> From: Julien Fortin <julien@cumulusnetworks.com>
> 
> bridge json fdb show is printing an incorrect / non-machine readable
> value, when using -j (json output) we are expecting machine readable
> data that shouldn't require special handling/parsing.
> 
> $ bridge -j fdb show | \
> python -c \
> 'import sys,json;print(json.dumps(json.loads(sys.stdin.read()),indent=4))'
> [
>     {
>         "master": "br0",
>         "mac": "56:23:28:4f:4f:e5",
>         "flags": [],
>         "ifname": "vx0",
>         "state": "state=0x80"  <<<<<<<<< with the patch: "state": "0x80"
>     }
> ]
> 
> Fixes: c7c1a1ef51aea7c ("bridge: colorize output and use JSON print library")
> Signed-off-by: Julien Fortin <julien@cumulusnetworks.com>
> ---
>  bridge/fdb.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/bridge/fdb.c b/bridge/fdb.c
> index d2247e80..198c51d1 100644
> --- a/bridge/fdb.c
> +++ b/bridge/fdb.c
> @@ -62,7 +62,10 @@ static const char *state_n2a(unsigned int s)
>  	if (s & NUD_REACHABLE)
>  		return "";
>  
> -	sprintf(buf, "state=%#x", s);
> +	if (is_json_context())
> +		sprintf(buf, "%#x", s);
> +	else
> +		sprintf(buf, "state=%#x", s);
>  	return buf;
>  }
>  

Printing in non JSON case was also wrong.
i.e.
              ...  state state=0x80
should be:
	      ... state 0x80

Let's do that.


The state=xxx value only shows up if the FDB entry has a value bridge command
doesn't understand. The bridge command needs to be able to display the new flag values.

Please fixup the two patches and resubmit to iproute2
Julien Fortin July 14, 2020, 11:37 p.m. UTC | #2 
On Mon, Jul 13, 2020 at 4:54 PM Stephen Hemminger
<stephen@networkplumber.org> wrote:
>
> On Fri, 10 Jul 2020 02:50:55 +0200
> Julien Fortin <julien@cumulusnetworks.com> wrote:
>
> > From: Julien Fortin <julien@cumulusnetworks.com>
> >
> > bridge json fdb show is printing an incorrect / non-machine readable
> > value, when using -j (json output) we are expecting machine readable
> > data that shouldn't require special handling/parsing.
> >
> > $ bridge -j fdb show | \
> > python -c \
> > 'import sys,json;print(json.dumps(json.loads(sys.stdin.read()),indent=4))'
> > [
> >     {
> >         "master": "br0",
> >         "mac": "56:23:28:4f:4f:e5",
> >         "flags": [],
> >         "ifname": "vx0",
> >         "state": "state=0x80"  <<<<<<<<< with the patch: "state": "0x80"
> >     }
> > ]
> >
> > Fixes: c7c1a1ef51aea7c ("bridge: colorize output and use JSON print library")
> > Signed-off-by: Julien Fortin <julien@cumulusnetworks.com>
> > ---
> >  bridge/fdb.c | 5 ++++-
> >  1 file changed, 4 insertions(+), 1 deletion(-)
> >
> > diff --git a/bridge/fdb.c b/bridge/fdb.c
> > index d2247e80..198c51d1 100644
> > --- a/bridge/fdb.c
> > +++ b/bridge/fdb.c
> > @@ -62,7 +62,10 @@ static const char *state_n2a(unsigned int s)
> >       if (s & NUD_REACHABLE)
> >               return "";
> >
> > -     sprintf(buf, "state=%#x", s);
> > +     if (is_json_context())
> > +             sprintf(buf, "%#x", s);
> > +     else
> > +             sprintf(buf, "state=%#x", s);
> >       return buf;
> >  }
> >
>
> Printing in non JSON case was also wrong.
> i.e.
>               ...  state state=0x80
> should be:
>               ... state 0x80
>
> Let's do that.
>
>
> The state=xxx value only shows up if the FDB entry has a value bridge command
> doesn't understand. The bridge command needs to be able to display the new flag values.
>
> Please fixup the two patches and resubmit to iproute2

I'll resubmit a patch for this specific issue, to handle both json and
non-json case correctly but i don't think it's necessary to resubmit
"bridge: fdb get: add missing json init (new_json_obj)" as it's a
different issue.

(re-sending without html)
Stephen Hemminger July 14, 2020, 11:47 p.m. UTC | #3 
On Wed, 15 Jul 2020 01:35:33 +0200
Julien Fortin <julien@cumulusnetworks.com> wrote:

> On Mon, Jul 13, 2020 at 4:54 PM Stephen Hemminger <
> stephen@networkplumber.org> wrote:  
> 
> > On Fri, 10 Jul 2020 02:50:55 +0200
> > Julien Fortin <julien@cumulusnetworks.com> wrote:
> >  
> > > From: Julien Fortin <julien@cumulusnetworks.com>
> > >
> > > bridge json fdb show is printing an incorrect / non-machine readable
> > > value, when using -j (json output) we are expecting machine readable
> > > data that shouldn't require special handling/parsing.
> > >
> > > $ bridge -j fdb show | \
> > > python -c \
> > > 'import  
> > sys,json;print(json.dumps(json.loads(sys.stdin.read()),indent=4))'  
> > > [
> > >     {
> > >         "master": "br0",
> > >         "mac": "56:23:28:4f:4f:e5",
> > >         "flags": [],
> > >         "ifname": "vx0",
> > >         "state": "state=0x80"  <<<<<<<<< with the patch: "state": "0x80"
> > >     }
> > > ]
> > >
> > > Fixes: c7c1a1ef51aea7c ("bridge: colorize output and use JSON print  
> > library")  
> > > Signed-off-by: Julien Fortin <julien@cumulusnetworks.com>
> > > ---
> > >  bridge/fdb.c | 5 ++++-
> > >  1 file changed, 4 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/bridge/fdb.c b/bridge/fdb.c
> > > index d2247e80..198c51d1 100644
> > > --- a/bridge/fdb.c
> > > +++ b/bridge/fdb.c
> > > @@ -62,7 +62,10 @@ static const char *state_n2a(unsigned int s)
> > >       if (s & NUD_REACHABLE)
> > >               return "";
> > >
> > > -     sprintf(buf, "state=%#x", s);
> > > +     if (is_json_context())
> > > +             sprintf(buf, "%#x", s);
> > > +     else
> > > +             sprintf(buf, "state=%#x", s);
> > >       return buf;
> > >  }
> > >  
> >
> > Printing in non JSON case was also wrong.
> > i.e.
> >               ...  state state=0x80
> > should be:
> >               ... state 0x80
> >
> > Let's do that.
> >
> >
> > The state=xxx value only shows up if the FDB entry has a value bridge
> > command
> > doesn't understand. The bridge command needs to be able to display the new
> > flag values.  
> 
> 
> > Please fixup the two patches and resubmit to iproute2
> >  
> 
> I'll resubmit a patch for this specific issue, to handle both json and
> non-json case correctly
> but i don't think it's necessary to resubmit "bridge: fdb get: add missing
> json init (new_json_obj)"
> as it's a different issue.

Normally, if you submit a patch series, then you need to resubmit
the whole series. If you submit two separate patches then each one can
be reviewed and accepted alone.
diff mbox series

Patch

diff --git a/bridge/fdb.c b/bridge/fdb.c
index d2247e80..198c51d1 100644
--- a/bridge/fdb.c
+++ b/bridge/fdb.c
@@ -62,7 +62,10 @@  static const char *state_n2a(unsigned int s)
 	if (s & NUD_REACHABLE)
 		return "";
 
-	sprintf(buf, "state=%#x", s);
+	if (is_json_context())
+		sprintf(buf, "%#x", s);
+	else
+		sprintf(buf, "state=%#x", s);
 	return buf;
 }