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

["Morten Brørup" <mb@smartsharesystems.com>]

+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.