From: "Morten Brørup" <mb@smartsharesystems.com>
To: "Bruce Richardson" <bruce.richardson@intel.com>,
"Ciara Power" <ciara.power@intel.com>,
"Chengwen Feng" <fengchengwen@huawei.com>
Cc: <thomas@monjalon.net>, <ferruh.yigit@xilinx.com>,
<kevin.laatz@intel.com>, <andrew.rybchenko@oktetlabs.ru>,
<jerinj@marvell.com>, <sachin.saxena@oss.nxp.com>,
<hemant.agrawal@nxp.com>, <dev@dpdk.org>
Subject: RE: [PATCH 1/5] usertools: use non-strict when json-loads in telemetry
Date: Wed, 15 Jun 2022 22:09:27 +0200 [thread overview]
Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87129@smartserver.smartshare.dk> (raw)
In-Reply-To: <YqoO36UqeAomXSSL@bricha3-MOBL.ger.corp.intel.com>
> 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
next prev parent reply other threads:[~2022-06-15 20:09 UTC|newest]
Thread overview: 39+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-06-15 7:39 [PATCH 0/5] support telemetry dump dev Chengwen Feng
2022-06-15 7:39 ` [PATCH 1/5] usertools: use non-strict when json-loads in telemetry Chengwen Feng
2022-06-15 13:54 ` Morten Brørup
2022-06-15 16:54 ` Bruce Richardson
2022-06-15 18:01 ` Morten Brørup
2022-06-15 20:09 ` Morten Brørup [this message]
2022-06-15 7:39 ` [PATCH 2/5] dmadev: support telemetry dump dmadev Chengwen Feng
2022-06-15 7:39 ` [PATCH 3/5] eventdev: support telemetry dump eventdev Chengwen Feng
2022-06-15 7:39 ` [PATCH 4/5] rawdev: support telemetry dump rawdev Chengwen Feng
2022-06-15 7:39 ` [PATCH 5/5] ethdev: support telemetry private dump Chengwen Feng
2022-06-15 20:15 ` [PATCH 0/5] support telemetry dump dev Morten Brørup
2022-06-16 8:19 ` Bruce Richardson
2022-06-16 9:00 ` Morten Brørup
2022-06-17 9:46 ` [PATCH v2 0/5] support telemetry dump Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 1/5] telemetry: escape special char when tel string Chengwen Feng
2022-06-17 11:16 ` Morten Brørup
2022-06-17 11:25 ` Bruce Richardson
2022-06-17 17:05 ` Stephen Hemminger
2022-06-18 3:52 ` fengchengwen
2022-06-18 9:59 ` Morten Brørup
2022-06-22 7:57 ` Power, Ciara
2022-06-22 9:19 ` Bruce Richardson
2022-06-23 16:45 ` Bruce Richardson
2022-06-17 11:27 ` Bruce Richardson
2022-06-17 9:46 ` [PATCH v2 2/5] dmadev: support telemetry dump dmadev Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 3/5] eventdev: support telemetry dump eventdev Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 4/5] rawdev: support telemetry dump rawdev Chengwen Feng
2022-06-17 9:46 ` [PATCH v2 5/5] ethdev: support telemetry private dump Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 0/4] support telemetry dump dev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 1/4] dmadev: support telemetry dump dmadev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 2/4] eventdev: support telemetry dump eventdev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 3/4] rawdev: support telemetry dump rawdev Chengwen Feng
2022-09-13 2:44 ` [PATCH v3 4/4] ethdev: support telemetry private dump Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 0/4] support telemetry dump dev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 1/4] dmadev: support telemetry dump dmadev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 2/4] eventdev: support telemetry dump eventdev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 3/4] rawdev: support telemetry dump rawdev Chengwen Feng
2022-09-13 7:13 ` [PATCH v4 4/4] ethdev: support telemetry private dump Chengwen Feng
2022-10-03 7:28 ` [PATCH v4 0/4] support telemetry dump dev David Marchand
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=98CBD80474FA8B44BF855DF32C47DC35D87129@smartserver.smartshare.dk \
--to=mb@smartsharesystems.com \
--cc=andrew.rybchenko@oktetlabs.ru \
--cc=bruce.richardson@intel.com \
--cc=ciara.power@intel.com \
--cc=dev@dpdk.org \
--cc=fengchengwen@huawei.com \
--cc=ferruh.yigit@xilinx.com \
--cc=hemant.agrawal@nxp.com \
--cc=jerinj@marvell.com \
--cc=kevin.laatz@intel.com \
--cc=sachin.saxena@oss.nxp.com \
--cc=thomas@monjalon.net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).