From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mails.dpdk.org (mails.dpdk.org [217.70.189.124]) by inbox.dpdk.org (Postfix) with ESMTP id 45E8EA0547; Wed, 15 Jun 2022 22:09:30 +0200 (CEST) Received: from [217.70.189.124] (localhost [127.0.0.1]) by mails.dpdk.org (Postfix) with ESMTP id 3817D427EB; Wed, 15 Jun 2022 22:09:30 +0200 (CEST) Received: from smartserver.smartsharesystems.com (smartserver.smartsharesystems.com [77.243.40.215]) by mails.dpdk.org (Postfix) with ESMTP id 69378410D0 for ; Wed, 15 Jun 2022 22:09:28 +0200 (CEST) Content-class: urn:content-classes:message MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable Subject: RE: [PATCH 1/5] usertools: use non-strict when json-loads in telemetry X-MimeOLE: Produced By Microsoft Exchange V6.5 Date: Wed, 15 Jun 2022 22:09:27 +0200 Message-ID: <98CBD80474FA8B44BF855DF32C47DC35D87129@smartserver.smartshare.dk> X-MS-Has-Attach: X-MS-TNEF-Correlator: Thread-Topic: [PATCH 1/5] usertools: use non-strict when json-loads in telemetry Thread-Index: AdiA2MNooZl1WFEUSuWo0JroMdNuiwABj+GwAAP4IPA= References: <20220615073915.14041-1-fengchengwen@huawei.com> <20220615073915.14041-2-fengchengwen@huawei.com> <98CBD80474FA8B44BF855DF32C47DC35D87126@smartserver.smartshare.dk> From: =?iso-8859-1?Q?Morten_Br=F8rup?= To: "Bruce Richardson" , "Ciara Power" , "Chengwen Feng" Cc: , , , , , , , X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org > From: Morten Br=F8rup > Sent: Wednesday, 15 June 2022 20.01 >=20 > +CC Ciara Power, Telemetry lib maintainer >=20 > > 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=F8rup wrote: > > > > From: Chengwen Feng [mailto:fengchengwen@huawei.com] > > > > Sent: Wednesday, 15 June 2022 09.39 > > > > > > > > Use 'strict=3DFalse' 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 > > > > --- > > > > 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=3DTrue): > > > > """ Read data from socket and return it in JSON format """ > > > > reply =3D sock.recv(buf_len).decode() > > > > try: > > > > - ret =3D json.loads(reply) > > > > + ret =3D json.loads(reply, strict=3DFalse) > > > > 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. >=20 > 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. >=20 > Warning: Although I consider such a change a bugfix, others might > consider it an ABI breakage. :-( >=20 > @Ciara, what is your opinion about my suggestion here? >=20 > 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. >=20 > 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=3D1037