[1/5] usertools: use non-strict when json-loads in telemetry

  Use 'strict=False' in json-loads, it will ignore control characters
(e.g. '\n\t'), this patch is prepared for the support of telemetry dump
in the future.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 usertools/dpdk-telemetry.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
  

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Wednesday, 15 June 2022 09.39
> 
> Use 'strict=False' in json-loads, it will ignore control characters
> (e.g. '\n\t'), this patch is prepared for the support of telemetry dump
> in the future.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  usertools/dpdk-telemetry.py | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-telemetry.py
> index a81868a547..63f8004566 100755
> --- a/usertools/dpdk-telemetry.py
> +++ b/usertools/dpdk-telemetry.py
> @@ -27,7 +27,7 @@ def read_socket(sock, buf_len, echo=True):
>      """ Read data from socket and return it in JSON format """
>      reply = sock.recv(buf_len).decode()
>      try:
> -        ret = json.loads(reply)
> +        ret = json.loads(reply, strict=False)
>      except json.JSONDecodeError:
>          print("Error in reply: ", reply)
>          sock.close()
> --
> 2.33.0
> 

As I understand this patch, it accepts non-JSON data from the telemetry socket.

Isn't it is a major protocol violation if the telemetry socket produces output requiring this modification? Doing that would break other applications expecting strictly JSON formatted output from the telemetry socket.
  

On Wed, Jun 15, 2022 at 03:54:57PM +0200, Morten Brørup wrote:
> > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > Sent: Wednesday, 15 June 2022 09.39
> > 
> > Use 'strict=False' in json-loads, it will ignore control characters
> > (e.g. '\n\t'), this patch is prepared for the support of telemetry dump
> > in the future.
> > 
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> >  usertools/dpdk-telemetry.py | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-telemetry.py
> > index a81868a547..63f8004566 100755
> > --- a/usertools/dpdk-telemetry.py
> > +++ b/usertools/dpdk-telemetry.py
> > @@ -27,7 +27,7 @@ def read_socket(sock, buf_len, echo=True):
> >      """ Read data from socket and return it in JSON format """
> >      reply = sock.recv(buf_len).decode()
> >      try:
> > -        ret = json.loads(reply)
> > +        ret = json.loads(reply, strict=False)
> >      except json.JSONDecodeError:
> >          print("Error in reply: ", reply)
> >          sock.close()
> > --
> > 2.33.0
> > 
> 
> As I understand this patch, it accepts non-JSON data from the telemetry socket.
> 
> Isn't it is a major protocol violation if the telemetry socket produces output requiring this modification? Doing that would break other applications expecting strictly JSON formatted output from the telemetry socket.
> 

I would tend to agree.

As an alternative, I think you should add to the telemetry library an
"escape string" function which can then be used by the telemetry functions
to properly json encode the strings back from the dump functions before
sending them out.

/Bruce
  

+CC Ciara Power, Telemetry lib maintainer

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 15 June 2022 18.55
> 
> On Wed, Jun 15, 2022 at 03:54:57PM +0200, Morten Brørup wrote:
> > > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > > Sent: Wednesday, 15 June 2022 09.39
> > >
> > > Use 'strict=False' in json-loads, it will ignore control characters
> > > (e.g. '\n\t'), this patch is prepared for the support of telemetry
> dump
> > > in the future.
> > >
> > > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > > ---
> > >  usertools/dpdk-telemetry.py | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-
> telemetry.py
> > > index a81868a547..63f8004566 100755
> > > --- a/usertools/dpdk-telemetry.py
> > > +++ b/usertools/dpdk-telemetry.py
> > > @@ -27,7 +27,7 @@ def read_socket(sock, buf_len, echo=True):
> > >      """ Read data from socket and return it in JSON format """
> > >      reply = sock.recv(buf_len).decode()
> > >      try:
> > > -        ret = json.loads(reply)
> > > +        ret = json.loads(reply, strict=False)
> > >      except json.JSONDecodeError:
> > >          print("Error in reply: ", reply)
> > >          sock.close()
> > > --
> > > 2.33.0
> > >
> >
> > As I understand this patch, it accepts non-JSON data from the
> telemetry socket.
> >
> > Isn't it is a major protocol violation if the telemetry socket
> produces output requiring this modification? Doing that would break
> other applications expecting strictly JSON formatted output from the
> telemetry socket.
> >
> 
> I would tend to agree.
> 
> As an alternative, I think you should add to the telemetry library an
> "escape string" function which can then be used by the telemetry
> functions
> to properly json encode the strings back from the dump functions before
> sending them out.

Instead of adding a separate JSON encode function, the rte_tel_data_string() and rte_tel_data_add_array_string() functions should simply JSON encode the provided strings if required. Their descriptions do not mention any requirements to the strings provided, and being control plane functions, I would certainly expect them to JSON encode.

Warning: Although I consider such a change a bugfix, others might consider it an ABI breakage. :-(

@Ciara, what is your opinion about my suggestion here?

For minimal changes, RTE_TEL_MAX_STRING_LEN and RTE_TEL_MAX_SINGLE_STRING_LEN should keep their meaning, i.e. define the maximum length of the string after any JSON encoding.

And optionally, new rte_tel_data_[array_]string_raw() performance optimized functions could be provided for strings known not to require any encoding.
  

> From: Morten Brørup
> Sent: Wednesday, 15 June 2022 20.01
> 
> +CC Ciara Power, Telemetry lib maintainer
> 
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Wednesday, 15 June 2022 18.55
> >
> > On Wed, Jun 15, 2022 at 03:54:57PM +0200, Morten Brørup wrote:
> > > > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > > > Sent: Wednesday, 15 June 2022 09.39
> > > >
> > > > Use 'strict=False' in json-loads, it will ignore control
> characters
> > > > (e.g. '\n\t'), this patch is prepared for the support of
> telemetry
> > dump
> > > > in the future.
> > > >
> > > > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > > > ---
> > > >  usertools/dpdk-telemetry.py | 2 +-
> > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/usertools/dpdk-telemetry.py b/usertools/dpdk-
> > telemetry.py
> > > > index a81868a547..63f8004566 100755
> > > > --- a/usertools/dpdk-telemetry.py
> > > > +++ b/usertools/dpdk-telemetry.py
> > > > @@ -27,7 +27,7 @@ def read_socket(sock, buf_len, echo=True):
> > > >      """ Read data from socket and return it in JSON format """
> > > >      reply = sock.recv(buf_len).decode()
> > > >      try:
> > > > -        ret = json.loads(reply)
> > > > +        ret = json.loads(reply, strict=False)
> > > >      except json.JSONDecodeError:
> > > >          print("Error in reply: ", reply)
> > > >          sock.close()
> > > > --
> > > > 2.33.0
> > > >
> > >
> > > As I understand this patch, it accepts non-JSON data from the
> > telemetry socket.
> > >
> > > Isn't it is a major protocol violation if the telemetry socket
> > produces output requiring this modification? Doing that would break
> > other applications expecting strictly JSON formatted output from the
> > telemetry socket.
> > >
> >
> > I would tend to agree.
> >
> > As an alternative, I think you should add to the telemetry library an
> > "escape string" function which can then be used by the telemetry
> > functions
> > to properly json encode the strings back from the dump functions
> before
> > sending them out.
> 
> Instead of adding a separate JSON encode function, the
> rte_tel_data_string() and rte_tel_data_add_array_string() functions
> should simply JSON encode the provided strings if required. Their
> descriptions do not mention any requirements to the strings provided,
> and being control plane functions, I would certainly expect them to
> JSON encode.
> 
> Warning: Although I consider such a change a bugfix, others might
> consider it an ABI breakage. :-(
> 
> @Ciara, what is your opinion about my suggestion here?
> 
> For minimal changes, RTE_TEL_MAX_STRING_LEN and
> RTE_TEL_MAX_SINGLE_STRING_LEN should keep their meaning, i.e. define
> the maximum length of the string after any JSON encoding.
> 
> And optionally, new rte_tel_data_[array_]string_raw() performance
> optimized functions could be provided for strings known not to require
> any encoding.

Forget my suggestion above!!! It would mess up the telemetry database, which could be used for SNMP too, and thus should not be JSON formatted.

Any JSON encoding needs to happen in the presentation layer, which seems to reside in /lib/telemetry/telemetry.c, and it looks like it already does JSON encode strings, except the rte_tel_json_str() function and friends in /lib/telemetry/telemetry_json.h don't implement it.

So the bug is in the JSON string functions in /lib/telemetry/telemetry_json.h: Their descriptions say that they take a string and copy it into a buffer in JSON format, which I interpret as JSON encoding. But they don't JSON encode the string. I have filed a bug in Bugzilla: https://bugs.dpdk.org/show_bug.cgi?id=1037