[PATCH 0/5] support telemetry dump dev

[PATCH 0/5] support telemetry dump dev

[Chengwen Feng]

This patchset contains five patch which support telemetry dump
dmadev/eventdev/rawdev/ethdev.

Chengwen Feng (5):
  usertools: use non-strict when json-loads in telemetry
  dmadev: support telemetry dump dmadev
  eventdev: support telemetry dump eventdev
  rawdev: support telemetry dump rawdev
  ethdev: support telemetry private dump

 lib/dmadev/rte_dmadev.c     | 39 +++++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c     | 42 ++++++++++++++++++++++++++++++++++++
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
 lib/rawdev/rte_rawdev.c     | 42 ++++++++++++++++++++++++++++++++++++
 usertools/dpdk-telemetry.py |  2 +-
 5 files changed, 167 insertions(+), 1 deletion(-)

-- 
2.33.0

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

[Chengwen Feng]

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

[PATCH 2/5] dmadev: support telemetry dump dmadev

[Chengwen Feng]

This patch supports telemetry dump dmadev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/dmadev/rte_dmadev.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 174d4c40ae..72c3989f60 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -985,6 +985,43 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_dma_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(dmadev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/dmadev/list", dmadev_handle_dev_list,
@@ -993,4 +1030,6 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
+			"Returns dump information for a dmadev. Parameters: int dev_id");
 }
-- 
2.33.0

[PATCH 3/5] eventdev: support telemetry dump eventdev

[Chengwen Feng]

This patch supports telemetry dump eventdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/eventdev/rte_eventdev.c b/lib/eventdev/rte_eventdev.c
index 1dc4f966be..5b1bd2b132 100644
--- a/lib/eventdev/rte_eventdev.c
+++ b/lib/eventdev/rte_eventdev.c
@@ -1791,6 +1791,47 @@ handle_queue_xstats(const char *cmd __rte_unused,
 	return eventdev_build_telemetry_data(dev_id, mode, port_queue_id, d);
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	/* Get dev ID from parameter string */
+	dev_id = strtoul(params, &end_param, 10);
+	if (*end_param != '\0')
+		RTE_EDEV_LOG_DEBUG(
+			"Extra parameters passed to eventdev telemetry command, ignoring");
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_event_dev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(eventdev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/eventdev/dev_list", handle_dev_list,
@@ -1807,6 +1848,8 @@ RTE_INIT(eventdev_init_telemetry)
 	rte_telemetry_register_cmd("/eventdev/queue_xstats",
 			handle_queue_xstats,
 			"Returns stats for an eventdev queue. Params: DevID,QueueID");
+	rte_telemetry_register_cmd("/eventdev/dev_dump", handle_dev_dump,
+			"Returns dump information for an eventdev. Parameter: DevID");
 	rte_telemetry_register_cmd("/eventdev/queue_links", handle_queue_links,
 			"Returns links for an eventdev port. Params: DevID,QueueID");
 }
-- 
2.33.0

[PATCH 4/5] rawdev: support telemetry dump rawdev

[Chengwen Feng]

This patch supports telemetry dump rawdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/rawdev/rte_rawdev.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/lib/rawdev/rte_rawdev.c b/lib/rawdev/rte_rawdev.c
index 2f0a4f132e..a4cdd418f8 100644
--- a/lib/rawdev/rte_rawdev.c
+++ b/lib/rawdev/rte_rawdev.c
@@ -613,6 +613,46 @@ handle_dev_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_RDEV_LOG(NOTICE,
+			"Extra parameters passed to rawdev telemetry command, ignoring");
+	if (!rte_rawdev_pmd_is_valid_dev(dev_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_rawdev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 RTE_LOG_REGISTER_DEFAULT(librawdev_logtype, INFO);
 
 RTE_INIT(librawdev_init_telemetry)
@@ -621,4 +661,6 @@ RTE_INIT(librawdev_init_telemetry)
 			"Returns list of available rawdev ports. Takes no parameters");
 	rte_telemetry_register_cmd("/rawdev/xstats", handle_dev_xstats,
 			"Returns the xstats for a rawdev port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/rawdev/dump", handle_dev_dump,
+			"Returns dump information for a rawdev port. Parameters: int port_id");
 }
-- 
2.33.0

[PATCH 5/5] ethdev: support telemetry private dump

[Chengwen Feng]

This patch supports telemetry private dump a ethdev port.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/ethdev/rte_ethdev.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 90e50eb02b..f79fb45ba2 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -5653,6 +5653,46 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
+			const char *params,
+			struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int port_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_eth_dev_priv_dump(port_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 static int
 eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 		const char *params,
@@ -5936,6 +5976,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
+			"Returns dump private information for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/link_status",
 			eth_dev_handle_port_link_status,
 			"Returns the link status for a port. Parameters: int port_id");
-- 
2.33.0

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

[Morten Brørup]

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

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

[Bruce Richardson]

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

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

[Morten Brørup]

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

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

[Morten Brørup]

> 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

RE: [PATCH 0/5] support telemetry dump dev

[Morten Brørup]

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Wednesday, 15 June 2022 09.39
> 
> This patchset contains five patch which support telemetry dump
> dmadev/eventdev/rawdev/ethdev.
> 
> Chengwen Feng (5):
>   usertools: use non-strict when json-loads in telemetry
>   dmadev: support telemetry dump dmadev
>   eventdev: support telemetry dump eventdev
>   rawdev: support telemetry dump rawdev
>   ethdev: support telemetry private dump
> 
>  lib/dmadev/rte_dmadev.c     | 39 +++++++++++++++++++++++++++++++++
>  lib/ethdev/rte_ethdev.c     | 42 ++++++++++++++++++++++++++++++++++++
>  lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
>  lib/rawdev/rte_rawdev.c     | 42 ++++++++++++++++++++++++++++++++++++
>  usertools/dpdk-telemetry.py |  2 +-
>  5 files changed, 167 insertions(+), 1 deletion(-)
> 
> --
> 2.33.0
> 

While hoping for someone to fix the Telemetry library bug regarding the missing JSON encoding of strings (https://bugs.dpdk.org/show_bug.cgi?id=1037), thus making the patch regarding usertools/dpdk-telemetry.py superfluous,

Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH 0/5] support telemetry dump dev

[Bruce Richardson]

On Wed, Jun 15, 2022 at 10:15:27PM +0200, Morten Brørup wrote:
> > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > Sent: Wednesday, 15 June 2022 09.39
> > 
> > This patchset contains five patch which support telemetry dump
> > dmadev/eventdev/rawdev/ethdev.
> > 
> > Chengwen Feng (5):
> >   usertools: use non-strict when json-loads in telemetry
> >   dmadev: support telemetry dump dmadev
> >   eventdev: support telemetry dump eventdev
> >   rawdev: support telemetry dump rawdev
> >   ethdev: support telemetry private dump
> > 
> >  lib/dmadev/rte_dmadev.c     | 39 +++++++++++++++++++++++++++++++++
> >  lib/ethdev/rte_ethdev.c     | 42 ++++++++++++++++++++++++++++++++++++
> >  lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
> >  lib/rawdev/rte_rawdev.c     | 42 ++++++++++++++++++++++++++++++++++++
> >  usertools/dpdk-telemetry.py |  2 +-
> >  5 files changed, 167 insertions(+), 1 deletion(-)
> > 
> > --
> > 2.33.0
> > 
> 
> While hoping for someone to fix the Telemetry library bug regarding the missing JSON encoding of strings (https://bugs.dpdk.org/show_bug.cgi?id=1037), thus making the patch regarding usertools/dpdk-telemetry.py superfluous,
> 
> Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 

I disagree with taking this series without fixing the underlying issue. I
don't think we should be sending invalid json data so doing proper
escaping of \n\t etc. characters on encode is the best solution.

There is also some merit in adding in support for tagging strings that
don't contain invalid characters, if perf is affected in some cases. That
said, in the testing I have done in the past:
* for jobs like getting the nic stats or nic xstats, the vast majority of
  the cycles spent in the telemetry function is doing PCI reads for the
  stats
* for simpler jobs e.g. getting list of ethdevs, the vast majority of the
  time is actually being spent in the kernel socket handling

Based on what I've seen (and unfortunately I didn't note down the exact
figures for above), its likely that even if performance of the interface is
a concern we can probably add in some basic string encoding without really
affecting things much.

/Bruce

RE: [PATCH 0/5] support telemetry dump dev

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 16 June 2022 10.19
> 
> On Wed, Jun 15, 2022 at 10:15:27PM +0200, Morten Brørup wrote:
> > > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > > Sent: Wednesday, 15 June 2022 09.39
> > >
> > > This patchset contains five patch which support telemetry dump
> > > dmadev/eventdev/rawdev/ethdev.
> > >
> > > Chengwen Feng (5):
> > >   usertools: use non-strict when json-loads in telemetry
> > >   dmadev: support telemetry dump dmadev
> > >   eventdev: support telemetry dump eventdev
> > >   rawdev: support telemetry dump rawdev
> > >   ethdev: support telemetry private dump
> > >
> > >  lib/dmadev/rte_dmadev.c     | 39 +++++++++++++++++++++++++++++++++
> > >  lib/ethdev/rte_ethdev.c     | 42
> ++++++++++++++++++++++++++++++++++++
> > >  lib/eventdev/rte_eventdev.c | 43
> +++++++++++++++++++++++++++++++++++++
> > >  lib/rawdev/rte_rawdev.c     | 42
> ++++++++++++++++++++++++++++++++++++
> > >  usertools/dpdk-telemetry.py |  2 +-
> > >  5 files changed, 167 insertions(+), 1 deletion(-)
> > >
> > > --
> > > 2.33.0
> > >
> >
> > While hoping for someone to fix the Telemetry library bug regarding
> the missing JSON encoding of strings
> (https://bugs.dpdk.org/show_bug.cgi?id=1037), thus making the patch
> regarding usertools/dpdk-telemetry.py superfluous,
> >
> > Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>
> >
> 
> I disagree with taking this series without fixing the underlying issue.
> I
> don't think we should be sending invalid json data so doing proper
> escaping of \n\t etc. characters on encode is the best solution.

OK. I'll accept upgrading from hope to requirement. :-)

> 
> There is also some merit in adding in support for tagging strings that
> don't contain invalid characters, if perf is affected in some cases.

You seem to be suggesting that we, in addition to RTE_TEL_STRING_VAL add a new telemetry value type for "strings not requiring any encoding by any protocols, JSON, HTML, SNMP, SOAP, InfluxDB, or whatever the future might bring", for performance reasons?

This would require that the telemetry providers call either rte_tel_data_plain_string() instead of rte_tel_data_string(), depending on the contents of the string, and damn well make sure they don't call rte_tel_data_plain_string() with strings containing special characters.

Alternatively, the rte_tel_data_string() function would need to scan for special characters and mark the string type accordingly.


I don't think it is a good idea modifying the lower layer of the telemetry library to support this.

I prefer to keep the choice of encoding or simple copying (assuming that simply copying provides a performance benefit) in the protocol specific encoding functions, such as rte_tel_json_str().

The performance cost of making the decision (to encode or not) in the protocol layer, i.e. in rte_tel_json_str(), is not higher than it is in the lower layer, i.e. in rte_tel_data_string(). And some protocols, e.g. SNMP, do allow a lot of special characters in strings without encoding.

> That
> said, in the testing I have done in the past:
> * for jobs like getting the nic stats or nic xstats, the vast majority
> of
>   the cycles spent in the telemetry function is doing PCI reads for the
>   stats
> * for simpler jobs e.g. getting list of ethdevs, the vast majority of
> the
>   time is actually being spent in the kernel socket handling
> 
> Based on what I've seen (and unfortunately I didn't note down the exact
> figures for above), its likely that even if performance of the
> interface is
> a concern we can probably add in some basic string encoding without
> really
> affecting things much.

Agree. Especially if we keep the implementation simple. E.g. we can reuse size limit from the unencoded for the encoded string.

> 
> /Bruce

[PATCH v2 0/5] support telemetry dump

[Chengwen Feng]

This patchset contains five patch which support telemetry dump
dmadev/eventdev/rawdev/ethdev.

---
v2: 
* Address Morten and Bruce's comments.

Chengwen Feng (5):
  telemetry: escape special char when tel string
  dmadev: support telemetry dump dmadev
  eventdev: support telemetry dump eventdev
  rawdev: support telemetry dump rawdev
  ethdev: support telemetry private dump

 lib/dmadev/rte_dmadev.c     | 39 +++++++++++++++
 lib/ethdev/rte_ethdev.c     | 43 +++++++++++++++++
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++
 lib/rawdev/rte_rawdev.c     | 42 ++++++++++++++++
 lib/telemetry/telemetry.c   | 96 +++++++++++++++++++++++++++++++++++--
 5 files changed, 260 insertions(+), 3 deletions(-)

-- 
2.33.0

[PATCH v2 1/5] telemetry: escape special char when tel string

[Chengwen Feng]

This patch supports escape special characters (including: \",\\,/,\b,
/f,/n,/r,/t) when telemetry string.
This patch is used to support telemetry xxx-dump commands which the
string may include special characters.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
---
 lib/telemetry/telemetry.c | 96 +++++++++++++++++++++++++++++++++++++--
 1 file changed, 93 insertions(+), 3 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index c6fd03a5ab..0f762f633e 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 	return used;
 }
 
+static bool
+json_is_special_char(char ch)
+{
+	static unsigned char is_spec[256] = { 0 };
+	static bool init_once;
+
+	if (!init_once) {
+		is_spec['\"'] = 1;
+		is_spec['\\'] = 1;
+		is_spec['/'] = 1;
+		is_spec['\b'] = 1;
+		is_spec['\f'] = 1;
+		is_spec['\n'] = 1;
+		is_spec['\r'] = 1;
+		is_spec['\t'] = 1;
+		init_once = true;
+	}
+
+	return (bool)is_spec[(unsigned char)ch];
+}
+
+static size_t
+json_escape_special_char(char *buf, const char ch)
+{
+	size_t used = 0;
+
+	switch (ch) {
+	case '\"':
+		buf[used++] = '\\';
+		buf[used++] = '\"';
+		break;
+	case '\\':
+		buf[used++] = '\\';
+		buf[used++] = '\\';
+		break;
+	case '/':
+		buf[used++] = '\\';
+		buf[used++] = '/';
+		break;
+	case '\b':
+		buf[used++] = '\\';
+		buf[used++] = 'b';
+		break;
+	case '\f':
+		buf[used++] = '\\';
+		buf[used++] = 'f';
+		break;
+	case '\n':
+		buf[used++] = '\\';
+		buf[used++] = 'n';
+		break;
+	case '\r':
+		buf[used++] = '\\';
+		buf[used++] = 'r';
+		break;
+	case '\t':
+		buf[used++] = '\\';
+		buf[used++] = 't';
+		break;
+	default:
+		break;
+	}
+
+	return used;
+}
+
+static size_t
+json_format_string(char *buf, size_t len, const char *str)
+{
+	size_t used = 0;
+
+	while (*str) {
+		if (unlikely(len < used + 2)) {
+			TMTY_LOG(WARNING, "Insufficient buffer when json format string\n");
+			break;
+		}
+
+		if (json_is_special_char(*str))
+			used += json_escape_special_char(buf + used, *str);
+		else
+			buf[used++] = *str;
+
+		str++;
+	}
+
+	return used;
+}
+
 static void
 output_json(const char *cmd, const struct rte_tel_data *d, int s)
 {
@@ -232,9 +320,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				MAX_CMD_LEN, cmd ? cmd : "none");
 		break;
 	case RTE_TEL_STRING:
-		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":\"%.*s\"}",
-				MAX_CMD_LEN, cmd,
-				RTE_TEL_MAX_SINGLE_STRING_LEN, d->data.str);
+		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":\"",
+				MAX_CMD_LEN, cmd);
+		used += json_format_string(out_buf + used,
+				sizeof(out_buf) - used - 3, d->data.str);
+		used += snprintf(out_buf + used, sizeof(out_buf) - used, "\"}");
 		break;
 	case RTE_TEL_DICT:
 		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-- 
2.33.0

[PATCH v2 2/5] dmadev: support telemetry dump dmadev

[Chengwen Feng]

This patch supports telemetry dump dmadev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/dmadev/rte_dmadev.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 174d4c40ae..72c3989f60 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -985,6 +985,43 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_dma_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(dmadev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/dmadev/list", dmadev_handle_dev_list,
@@ -993,4 +1030,6 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
+			"Returns dump information for a dmadev. Parameters: int dev_id");
 }
-- 
2.33.0

[PATCH v2 3/5] eventdev: support telemetry dump eventdev

[Chengwen Feng]

This patch supports telemetry dump eventdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/eventdev/rte_eventdev.c b/lib/eventdev/rte_eventdev.c
index 1dc4f966be..5b1bd2b132 100644
--- a/lib/eventdev/rte_eventdev.c
+++ b/lib/eventdev/rte_eventdev.c
@@ -1791,6 +1791,47 @@ handle_queue_xstats(const char *cmd __rte_unused,
 	return eventdev_build_telemetry_data(dev_id, mode, port_queue_id, d);
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	/* Get dev ID from parameter string */
+	dev_id = strtoul(params, &end_param, 10);
+	if (*end_param != '\0')
+		RTE_EDEV_LOG_DEBUG(
+			"Extra parameters passed to eventdev telemetry command, ignoring");
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_event_dev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(eventdev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/eventdev/dev_list", handle_dev_list,
@@ -1807,6 +1848,8 @@ RTE_INIT(eventdev_init_telemetry)
 	rte_telemetry_register_cmd("/eventdev/queue_xstats",
 			handle_queue_xstats,
 			"Returns stats for an eventdev queue. Params: DevID,QueueID");
+	rte_telemetry_register_cmd("/eventdev/dev_dump", handle_dev_dump,
+			"Returns dump information for an eventdev. Parameter: DevID");
 	rte_telemetry_register_cmd("/eventdev/queue_links", handle_queue_links,
 			"Returns links for an eventdev port. Params: DevID,QueueID");
 }
-- 
2.33.0

[PATCH v2 4/5] rawdev: support telemetry dump rawdev

[Chengwen Feng]

This patch supports telemetry dump rawdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/rawdev/rte_rawdev.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/lib/rawdev/rte_rawdev.c b/lib/rawdev/rte_rawdev.c
index 2f0a4f132e..a4cdd418f8 100644
--- a/lib/rawdev/rte_rawdev.c
+++ b/lib/rawdev/rte_rawdev.c
@@ -613,6 +613,46 @@ handle_dev_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_RDEV_LOG(NOTICE,
+			"Extra parameters passed to rawdev telemetry command, ignoring");
+	if (!rte_rawdev_pmd_is_valid_dev(dev_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_rawdev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 RTE_LOG_REGISTER_DEFAULT(librawdev_logtype, INFO);
 
 RTE_INIT(librawdev_init_telemetry)
@@ -621,4 +661,6 @@ RTE_INIT(librawdev_init_telemetry)
 			"Returns list of available rawdev ports. Takes no parameters");
 	rte_telemetry_register_cmd("/rawdev/xstats", handle_dev_xstats,
 			"Returns the xstats for a rawdev port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/rawdev/dump", handle_dev_dump,
+			"Returns dump information for a rawdev port. Parameters: int port_id");
 }
-- 
2.33.0

[PATCH v2 5/5] ethdev: support telemetry private dump

[Chengwen Feng]

This patch supports telemetry private dump a ethdev port.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ethdev/rte_ethdev.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 90e50eb02b..89ebafd680 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -7,6 +7,7 @@
 #include <inttypes.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/queue.h>
@@ -5653,6 +5654,46 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
+			const char *params,
+			struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int port_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_eth_dev_priv_dump(port_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 static int
 eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 		const char *params,
@@ -5936,6 +5977,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
+			"Returns dump private information for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/link_status",
 			eth_dev_handle_port_link_status,
 			"Returns the link status for a port. Parameters: int port_id");
-- 
2.33.0

RE: [PATCH v2 1/5] telemetry: escape special char when tel string

[Morten Brørup]

> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> Sent: Friday, 17 June 2022 11.46
> 
> This patch supports escape special characters (including: \",\\,/,\b,
> /f,/n,/r,/t) when telemetry string.
> This patch is used to support telemetry xxx-dump commands which the
> string may include special characters.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/telemetry/telemetry.c | 96 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index c6fd03a5ab..0f762f633e 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data *d,
> char *out_buf, size_t buf_len)
>  	return used;
>  }
> 
> +static bool
> +json_is_special_char(char ch)
> +{
> +	static unsigned char is_spec[256] = { 0 };
> +	static bool init_once;
> +
> +	if (!init_once) {
> +		is_spec['\"'] = 1;
> +		is_spec['\\'] = 1;
> +		is_spec['/'] = 1;
> +		is_spec['\b'] = 1;
> +		is_spec['\f'] = 1;
> +		is_spec['\n'] = 1;
> +		is_spec['\r'] = 1;
> +		is_spec['\t'] = 1;
> +		init_once = true;
> +	}
> +
> +	return (bool)is_spec[(unsigned char)ch];
> +}

Here's a suggestion for simplifying the code:

Remove json_is_special_char(), and update json_escape_special_char() and json_format_string() as follows:

> +
> +static size_t
> +json_escape_special_char(char *buf, const char ch)

Consider making this function inline.

> +{
> +	size_t used = 0;
> +
> +	switch (ch) {
> +	case '\"':
> +		buf[used++] = '\\';
> +		buf[used++] = '\"';
> +		break;
> +	case '\\':
> +		buf[used++] = '\\';
> +		buf[used++] = '\\';
> +		break;
> +	case '/':
> +		buf[used++] = '\\';
> +		buf[used++] = '/';
> +		break;
> +	case '\b':
> +		buf[used++] = '\\';
> +		buf[used++] = 'b';
> +		break;
> +	case '\f':
> +		buf[used++] = '\\';
> +		buf[used++] = 'f';
> +		break;
> +	case '\n':
> +		buf[used++] = '\\';
> +		buf[used++] = 'n';
> +		break;
> +	case '\r':
> +		buf[used++] = '\\';
> +		buf[used++] = 'r';
> +		break;
> +	case '\t':
> +		buf[used++] = '\\';
> +		buf[used++] = 't';
> +		break;
> +	default:

Handle non-escaped characters in the default case here instead:

+		buf[used++] = ch;

> +		break;
> +	}
> +
> +	return used;
> +}
> +
> +static size_t
> +json_format_string(char *buf, size_t len, const char *str)
> +{
> +	size_t used = 0;
> +
> +	while (*str) {
> +		if (unlikely(len < used + 2)) {
> +			TMTY_LOG(WARNING, "Insufficient buffer when json
> format string\n");
> +			break;
> +		}
> +

-- replace:
> +		if (json_is_special_char(*str))
> +			used += json_escape_special_char(buf + used, *str);
> +		else
> +			buf[used++] = *str;
> +
> +		str++;
-- by:
+		used += json_escape_special_char(buf + used, *str++);
--

End of suggestion. Feel free to use it or not. :-)

> +	}
> +
> +	return used;
> +}
> +
>  static void
>  output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  {
> @@ -232,9 +320,11 @@ output_json(const char *cmd, const struct
> rte_tel_data *d, int s)
>  				MAX_CMD_LEN, cmd ? cmd : "none");
>  		break;
>  	case RTE_TEL_STRING:
> -		used = snprintf(out_buf, sizeof(out_buf),
> "{\"%.*s\":\"%.*s\"}",
> -				MAX_CMD_LEN, cmd,
> -				RTE_TEL_MAX_SINGLE_STRING_LEN, d->data.str);
> +		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":\"",
> +				MAX_CMD_LEN, cmd);
> +		used += json_format_string(out_buf + used,
> +				sizeof(out_buf) - used - 3, d->data.str);
> +		used += snprintf(out_buf + used, sizeof(out_buf) - used,
> "\"}");

I looked for missing 0-termination in json_format_string(), but that is not required due to the immediately following snprintf().

>  		break;
>  	case RTE_TEL_DICT:
>  		prefix_used = snprintf(out_buf, sizeof(out_buf),
> "{\"%.*s\":",
> --
> 2.33.0
> 

If you want to make it generic, these four cases in output_json() also need to JSON encode the strings:

case RTE_TEL_DICT:
	case RTE_TEL_STRING_VAL: ...
	case RTE_TEL_CONTAINER: ... (strings only)

case RTE_TEL_ARRAY_STRING:
	if (d->type == RTE_TEL_ARRAY_STRING) ...
	else if (d->type == RTE_TEL_ARRAY_CONTAINER) ... (strings only)


However, JSON encoding of strings inside arrays and containers is not required for the dump purposes addressed by this patch series, so I consider this patch complete without it. No need to add "feature creep" to this series.


Reviewed-by: Morten Brørup <mb@smartsharesystems.com>

Re: [PATCH v2 1/5] telemetry: escape special char when tel string

[Bruce Richardson]

On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
> > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > Sent: Friday, 17 June 2022 11.46
> > 
> > This patch supports escape special characters (including: \",\\,/,\b,
> > /f,/n,/r,/t) when telemetry string.
> > This patch is used to support telemetry xxx-dump commands which the
> > string may include special characters.
> > 
> > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > ---
> >  lib/telemetry/telemetry.c | 96 +++++++++++++++++++++++++++++++++++++--
> >  1 file changed, 93 insertions(+), 3 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> > index c6fd03a5ab..0f762f633e 100644
> > --- a/lib/telemetry/telemetry.c
> > +++ b/lib/telemetry/telemetry.c
> > @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data *d,
> > char *out_buf, size_t buf_len)
> >  	return used;
> >  }
> > 
> > +static bool
> > +json_is_special_char(char ch)
> > +{
> > +	static unsigned char is_spec[256] = { 0 };
> > +	static bool init_once;
> > +
> > +	if (!init_once) {
> > +		is_spec['\"'] = 1;
> > +		is_spec['\\'] = 1;
> > +		is_spec['/'] = 1;
> > +		is_spec['\b'] = 1;
> > +		is_spec['\f'] = 1;
> > +		is_spec['\n'] = 1;
> > +		is_spec['\r'] = 1;
> > +		is_spec['\t'] = 1;
> > +		init_once = true;
> > +	}
> > +
> > +	return (bool)is_spec[(unsigned char)ch];
> > +}

According to the json spec at [1], the characters that need to be escaped
are:
a) any characters <0x20
b) inverted commas/quote character \"
c) the "reverse solidus character", better known to you and I as the
back-slash.

Therefore, I think this table generation could be simplified, but also
expanded using this. For completeness we should also see about handling all
control characters if they are encountered.

[1] https://www.rfc-editor.org/rfc/rfc8259.txt

/Bruce

Re: [PATCH v2 1/5] telemetry: escape special char when tel string

[Bruce Richardson]

On Fri, Jun 17, 2022 at 05:46:20PM +0800, Chengwen Feng wrote:
> This patch supports escape special characters (including: \",\\,/,\b,
> /f,/n,/r,/t) when telemetry string.
> This patch is used to support telemetry xxx-dump commands which the
> string may include special characters.
> 
> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> ---
>  lib/telemetry/telemetry.c | 96 +++++++++++++++++++++++++++++++++++++--
>  1 file changed, 93 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index c6fd03a5ab..0f762f633e 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>  	return used;
>  }
>  
> +static bool
> +json_is_special_char(char ch)
> +{
> +	static unsigned char is_spec[256] = { 0 };
> +	static bool init_once;
> +
> +	if (!init_once) {
> +		is_spec['\"'] = 1;
> +		is_spec['\\'] = 1;
> +		is_spec['/'] = 1;
> +		is_spec['\b'] = 1;
> +		is_spec['\f'] = 1;
> +		is_spec['\n'] = 1;
> +		is_spec['\r'] = 1;
> +		is_spec['\t'] = 1;
> +		init_once = true;
> +	}
> +
> +	return (bool)is_spec[(unsigned char)ch];
> +}
> +
> +static size_t
> +json_escape_special_char(char *buf, const char ch)
> +{
> +	size_t used = 0;
> +
> +	switch (ch) {
> +	case '\"':
> +		buf[used++] = '\\';
> +		buf[used++] = '\"';
> +		break;
> +	case '\\':
> +		buf[used++] = '\\';
> +		buf[used++] = '\\';
> +		break;
> +	case '/':
> +		buf[used++] = '\\';
> +		buf[used++] = '/';
> +		break;
> +	case '\b':
> +		buf[used++] = '\\';
> +		buf[used++] = 'b';
> +		break;
> +	case '\f':
> +		buf[used++] = '\\';
> +		buf[used++] = 'f';
> +		break;
> +	case '\n':
> +		buf[used++] = '\\';
> +		buf[used++] = 'n';
> +		break;
> +	case '\r':
> +		buf[used++] = '\\';
> +		buf[used++] = 'r';
> +		break;
> +	case '\t':
> +		buf[used++] = '\\';
> +		buf[used++] = 't';
> +		break;
> +	default:
> +		break;
> +	}
> +
> +	return used;
> +}
> +
> +static size_t
> +json_format_string(char *buf, size_t len, const char *str)
> +{
> +	size_t used = 0;
> +
> +	while (*str) {
> +		if (unlikely(len < used + 2)) {
> +			TMTY_LOG(WARNING, "Insufficient buffer when json format string\n");
> +			break;
> +		}
> +
> +		if (json_is_special_char(*str))
> +			used += json_escape_special_char(buf + used, *str);
> +		else
> +			buf[used++] = *str;
> +
> +		str++;
> +	}
> +
> +	return used;
> +}
> +
>  static void
>  output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  {
> @@ -232,9 +320,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>  				MAX_CMD_LEN, cmd ? cmd : "none");
>  		break;
>  	case RTE_TEL_STRING:
> -		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":\"%.*s\"}",
> -				MAX_CMD_LEN, cmd,
> -				RTE_TEL_MAX_SINGLE_STRING_LEN, d->data.str);
> +		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":\"",
> +				MAX_CMD_LEN, cmd);
> +		used += json_format_string(out_buf + used,
> +				sizeof(out_buf) - used - 3, d->data.str);
> +		used += snprintf(out_buf + used, sizeof(out_buf) - used, "\"}");
>  		break;
>  	case RTE_TEL_DICT:
>  		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> -- 

I think it might be worthwhile to write a general json_str_printf function
to do the snprintf and the escaping in one go. Then that might be more able
to be used in other places where we output strings.

/Bruce

Re: [PATCH v2 1/5] telemetry: escape special char when tel string

[Stephen Hemminger]

On Fri, 17 Jun 2022 12:25:04 +0100
Bruce Richardson <bruce.richardson@intel.com> wrote:

> On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
> > > From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > > Sent: Friday, 17 June 2022 11.46
> > > 
> > > This patch supports escape special characters (including: \",\\,/,\b,
> > > /f,/n,/r,/t) when telemetry string.
> > > This patch is used to support telemetry xxx-dump commands which the
> > > string may include special characters.
> > > 
> > > Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > > ---
> > >  lib/telemetry/telemetry.c | 96 +++++++++++++++++++++++++++++++++++++--
> > >  1 file changed, 93 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> > > index c6fd03a5ab..0f762f633e 100644
> > > --- a/lib/telemetry/telemetry.c
> > > +++ b/lib/telemetry/telemetry.c
> > > @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data *d,
> > > char *out_buf, size_t buf_len)
> > >  	return used;
> > >  }
> > > 
> > > +static bool
> > > +json_is_special_char(char ch)
> > > +{
> > > +	static unsigned char is_spec[256] = { 0 };
> > > +	static bool init_once;
> > > +
> > > +	if (!init_once) {
> > > +		is_spec['\"'] = 1;
> > > +		is_spec['\\'] = 1;
> > > +		is_spec['/'] = 1;
> > > +		is_spec['\b'] = 1;
> > > +		is_spec['\f'] = 1;
> > > +		is_spec['\n'] = 1;
> > > +		is_spec['\r'] = 1;
> > > +		is_spec['\t'] = 1;
> > > +		init_once = true;
> > > +	}
> > > +
> > > +	return (bool)is_spec[(unsigned char)ch];
> > > +}  
> 
> According to the json spec at [1], the characters that need to be escaped
> are:
> a) any characters <0x20
> b) inverted commas/quote character \"
> c) the "reverse solidus character", better known to you and I as the
> back-slash.
> 
> Therefore, I think this table generation could be simplified, but also
> expanded using this. For completeness we should also see about handling all
> control characters if they are encountered.
> 
> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> 
> /Bruce

Since it is trivial could be initializer?

static const uint8_t is_spec[256] = {
   [0 ... 0x20] = 1,
   ['\"' ] = 1,
   ['\\' ] = 1,
   ['/'] = 1,

etc

Or we could change the telemetry API to disallow control characters?

Re: [PATCH v2 1/5] telemetry: escape special char when tel string

[fengchengwen]

On 2022/6/18 1:05, Stephen Hemminger wrote:
> On Fri, 17 Jun 2022 12:25:04 +0100
> Bruce Richardson <bruce.richardson@intel.com> wrote:
> 
>> On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
>>>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
>>>> Sent: Friday, 17 June 2022 11.46
>>>>
>>>> This patch supports escape special characters (including: \",\\,/,\b,
>>>> /f,/n,/r,/t) when telemetry string.
>>>> This patch is used to support telemetry xxx-dump commands which the
>>>> string may include special characters.
>>>>
>>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
>>>> ---
>>>>  lib/telemetry/telemetry.c | 96 +++++++++++++++++++++++++++++++++++++--
>>>>  1 file changed, 93 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
>>>> index c6fd03a5ab..0f762f633e 100644
>>>> --- a/lib/telemetry/telemetry.c
>>>> +++ b/lib/telemetry/telemetry.c
>>>> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data *d,
>>>> char *out_buf, size_t buf_len)
>>>>  	return used;
>>>>  }
>>>>
>>>> +static bool
>>>> +json_is_special_char(char ch)
>>>> +{
>>>> +	static unsigned char is_spec[256] = { 0 };
>>>> +	static bool init_once;
>>>> +
>>>> +	if (!init_once) {
>>>> +		is_spec['\"'] = 1;
>>>> +		is_spec['\\'] = 1;
>>>> +		is_spec['/'] = 1;
>>>> +		is_spec['\b'] = 1;
>>>> +		is_spec['\f'] = 1;
>>>> +		is_spec['\n'] = 1;
>>>> +		is_spec['\r'] = 1;
>>>> +		is_spec['\t'] = 1;
>>>> +		init_once = true;
>>>> +	}
>>>> +
>>>> +	return (bool)is_spec[(unsigned char)ch];
>>>> +}  
>>
>> According to the json spec at [1], the characters that need to be escaped
>> are:
>> a) any characters <0x20
>> b) inverted commas/quote character \"
>> c) the "reverse solidus character", better known to you and I as the
>> back-slash.
>>
>> Therefore, I think this table generation could be simplified, but also
>> expanded using this. For completeness we should also see about handling all
>> control characters if they are encountered.
>>
>> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
>>
>> /Bruce
> 
> Since it is trivial could be initializer?
> 
> static const uint8_t is_spec[256] = {
>    [0 ... 0x20] = 1,
>    ['\"' ] = 1,
>    ['\\' ] = 1,
>    ['/'] = 1,
> 
> etc
> 
> Or we could change the telemetry API to disallow control characters?

I was thinking about converting 0~0x20, but I don't think there's a scenario.

I prefer change the telemetry API to disallow control characters, and this may not
be a violation of the ABI, if yes, the dpdk-telemetry.py will returns an error.

So I think we could add declaring and checking functions to make sure telemetry string
do not allow control characters.

> 
> 
> .
> 

RE: [PATCH v2 1/5] telemetry: escape special char when tel string

[Morten Brørup]

+CC: Ciara Power, Telemetry library maintainer

> From: fengchengwen [mailto:fengchengwen@huawei.com]
> Sent: Saturday, 18 June 2022 05.52
> 
> On 2022/6/18 1:05, Stephen Hemminger wrote:
> > On Fri, 17 Jun 2022 12:25:04 +0100
> > Bruce Richardson <bruce.richardson@intel.com> wrote:
> >
> >> On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
> >>>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> >>>> Sent: Friday, 17 June 2022 11.46
> >>>>
> >>>> This patch supports escape special characters (including:
> \",\\,/,\b,
> >>>> /f,/n,/r,/t) when telemetry string.
> >>>> This patch is used to support telemetry xxx-dump commands which
> the
> >>>> string may include special characters.
> >>>>
> >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> >>>> ---
> >>>>  lib/telemetry/telemetry.c | 96
> +++++++++++++++++++++++++++++++++++++--
> >>>>  1 file changed, 93 insertions(+), 3 deletions(-)
> >>>>
> >>>> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> >>>> index c6fd03a5ab..0f762f633e 100644
> >>>> --- a/lib/telemetry/telemetry.c
> >>>> +++ b/lib/telemetry/telemetry.c
> >>>> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data
> *d,
> >>>> char *out_buf, size_t buf_len)
> >>>>  	return used;
> >>>>  }
> >>>>
> >>>> +static bool
> >>>> +json_is_special_char(char ch)
> >>>> +{
> >>>> +	static unsigned char is_spec[256] = { 0 };
> >>>> +	static bool init_once;
> >>>> +
> >>>> +	if (!init_once) {
> >>>> +		is_spec['\"'] = 1;
> >>>> +		is_spec['\\'] = 1;
> >>>> +		is_spec['/'] = 1;
> >>>> +		is_spec['\b'] = 1;
> >>>> +		is_spec['\f'] = 1;
> >>>> +		is_spec['\n'] = 1;
> >>>> +		is_spec['\r'] = 1;
> >>>> +		is_spec['\t'] = 1;
> >>>> +		init_once = true;
> >>>> +	}
> >>>> +
> >>>> +	return (bool)is_spec[(unsigned char)ch];
> >>>> +}
> >>
> >> According to the json spec at [1], the characters that need to be
> escaped
> >> are:
> >> a) any characters <0x20
> >> b) inverted commas/quote character \"
> >> c) the "reverse solidus character", better known to you and I as the
> >> back-slash.
> >>
> >> Therefore, I think this table generation could be simplified, but
> also
> >> expanded using this. For completeness we should also see about
> handling all
> >> control characters if they are encountered.
> >>
> >> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> >>
> >> /Bruce
> >
> > Since it is trivial could be initializer?
> >
> > static const uint8_t is_spec[256] = {
> >    [0 ... 0x20] = 1,
> >    ['\"' ] = 1,
> >    ['\\' ] = 1,
> >    ['/'] = 1,
> >
> > etc
> >
> > Or we could change the telemetry API to disallow control characters?
> 
> I was thinking about converting 0~0x20, but I don't think there's a
> scenario.
> 
> I prefer change the telemetry API to disallow control characters, and
> this may not
> be a violation of the ABI, if yes, the dpdk-telemetry.py will returns
> an error.

I agree with Chengwen Feng. The telemetry data type is STRING, not BLOB.

So we need to define exactly what the STRING type contains.

I hope we can all agree that control characters should be disallowed.

The more complicated question is: Do we want to use the ASCII character set only, or do we want to use UTF-8 encoded Unicode?

Personally, think UTF-8 encoded Unicode is more future proof, and would vote for that.

But I wouldn't reject limiting it to ASCII, and perhaps in the future introduce another data type for UTF-8 strings.

UTF-8 is the modern choice, but it is incompatible with old stuff, e.g. many SNMP MIBs.

> 
> So I think we could add declaring and checking functions to make sure
> telemetry string
> do not allow control characters.

Input validation (when storing a string in the telemetry database) has a performance cost, so it could be a compile time debug option, like the memory cookies and mbuf integrity checks. Just a thought.

RE: [PATCH v2 1/5] telemetry: escape special char when tel string

[Power, Ciara]

Hi folks,

> -----Original Message-----
> From: Morten Brørup <mb@smartsharesystems.com>
> Sent: Saturday 18 June 2022 10:59
> To: fengchengwen <fengchengwen@huawei.com>; Stephen Hemminger
> <stephen@networkplumber.org>; Richardson, Bruce
> <bruce.richardson@intel.com>
> Cc: thomas@monjalon.net; ferruh.yigit@xilinx.com; Laatz, Kevin
> <kevin.laatz@intel.com>; andrew.rybchenko@oktetlabs.ru;
> jerinj@marvell.com; sachin.saxena@oss.nxp.com;
> hemant.agrawal@nxp.com; dev@dpdk.org; Power, Ciara
> <ciara.power@intel.com>
> Subject: RE: [PATCH v2 1/5] telemetry: escape special char when tel string
> 
> +CC: Ciara Power, Telemetry library maintainer
> 
> > From: fengchengwen [mailto:fengchengwen@huawei.com]
> > Sent: Saturday, 18 June 2022 05.52
> >
> > On 2022/6/18 1:05, Stephen Hemminger wrote:
> > > On Fri, 17 Jun 2022 12:25:04 +0100
> > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > >
> > >> On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
> > >>>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > >>>> Sent: Friday, 17 June 2022 11.46
> > >>>>
> > >>>> This patch supports escape special characters (including:
> > \",\\,/,\b,
> > >>>> /f,/n,/r,/t) when telemetry string.
> > >>>> This patch is used to support telemetry xxx-dump commands which
> > the
> > >>>> string may include special characters.
> > >>>>
> > >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > >>>> ---
> > >>>>  lib/telemetry/telemetry.c | 96
> > +++++++++++++++++++++++++++++++++++++--
> > >>>>  1 file changed, 93 insertions(+), 3 deletions(-)
> > >>>>
> > >>>> diff --git a/lib/telemetry/telemetry.c
> > >>>> b/lib/telemetry/telemetry.c index c6fd03a5ab..0f762f633e 100644
> > >>>> --- a/lib/telemetry/telemetry.c
> > >>>> +++ b/lib/telemetry/telemetry.c
> > >>>> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data
> > *d,
> > >>>> char *out_buf, size_t buf_len)
> > >>>>  	return used;
> > >>>>  }
> > >>>>
> > >>>> +static bool
> > >>>> +json_is_special_char(char ch)
> > >>>> +{
> > >>>> +	static unsigned char is_spec[256] = { 0 };
> > >>>> +	static bool init_once;
> > >>>> +
> > >>>> +	if (!init_once) {
> > >>>> +		is_spec['\"'] = 1;
> > >>>> +		is_spec['\\'] = 1;
> > >>>> +		is_spec['/'] = 1;
> > >>>> +		is_spec['\b'] = 1;
> > >>>> +		is_spec['\f'] = 1;
> > >>>> +		is_spec['\n'] = 1;
> > >>>> +		is_spec['\r'] = 1;
> > >>>> +		is_spec['\t'] = 1;
> > >>>> +		init_once = true;
> > >>>> +	}
> > >>>> +
> > >>>> +	return (bool)is_spec[(unsigned char)ch]; }
> > >>
> > >> According to the json spec at [1], the characters that need to be
> > escaped
> > >> are:
> > >> a) any characters <0x20
> > >> b) inverted commas/quote character \"
> > >> c) the "reverse solidus character", better known to you and I as
> > >> the back-slash.
> > >>
> > >> Therefore, I think this table generation could be simplified, but
> > also
> > >> expanded using this. For completeness we should also see about
> > handling all
> > >> control characters if they are encountered.
> > >>
> > >> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > >>
> > >> /Bruce
> > >
> > > Since it is trivial could be initializer?
> > >
> > > static const uint8_t is_spec[256] = {
> > >    [0 ... 0x20] = 1,
> > >    ['\"' ] = 1,
> > >    ['\\' ] = 1,
> > >    ['/'] = 1,
> > >
> > > etc
> > >
> > > Or we could change the telemetry API to disallow control characters?
> >
> > I was thinking about converting 0~0x20, but I don't think there's a
> > scenario.
> >
> > I prefer change the telemetry API to disallow control characters, and
> > this may not be a violation of the ABI, if yes, the dpdk-telemetry.py
> > will returns an error.
> 
> I agree with Chengwen Feng. The telemetry data type is STRING, not BLOB.
> 
> So we need to define exactly what the STRING type contains.
> 
> I hope we can all agree that control characters should be disallowed.
> 
> The more complicated question is: Do we want to use the ASCII character set
> only, or do we want to use UTF-8 encoded Unicode?
> 
> Personally, think UTF-8 encoded Unicode is more future proof, and would
> vote for that.
> 
> But I wouldn't reject limiting it to ASCII, and perhaps in the future introduce
> another data type for UTF-8 strings.
> 
> UTF-8 is the modern choice, but it is incompatible with old stuff, e.g. many
> SNMP MIBs.
> 
[CP] 

Just from looking at the spec [1] , I would say UTF-8, as it seems to suggest its use for JSON (section 8.1).

[1] https://www.rfc-editor.org/rfc/rfc8259.txt

> >
> > So I think we could add declaring and checking functions to make sure
> > telemetry string do not allow control characters.
[CP] 

I am not sure why we don't want these at all - I thought we do want some of them, like tab (\u0009) for example.

<snip>

In general, I think Bruce's suggestion of using a customised printf function instead of snprintf would be a good way forward, to scan the chars as they are being copied in.

Thanks,
Ciara

Re: [PATCH v2 1/5] telemetry: escape special char when tel string

[Bruce Richardson]

On Wed, Jun 22, 2022 at 08:57:43AM +0100, Power, Ciara wrote:
> Hi folks,
> 
> > -----Original Message-----
> > From: Morten Brørup <mb@smartsharesystems.com>
> > Sent: Saturday 18 June 2022 10:59
> > To: fengchengwen <fengchengwen@huawei.com>; Stephen Hemminger
> > <stephen@networkplumber.org>; Richardson, Bruce
> > <bruce.richardson@intel.com>
> > Cc: thomas@monjalon.net; ferruh.yigit@xilinx.com; Laatz, Kevin
> > <kevin.laatz@intel.com>; andrew.rybchenko@oktetlabs.ru;
> > jerinj@marvell.com; sachin.saxena@oss.nxp.com;
> > hemant.agrawal@nxp.com; dev@dpdk.org; Power, Ciara
> > <ciara.power@intel.com>
> > Subject: RE: [PATCH v2 1/5] telemetry: escape special char when tel string
> >
> > +CC: Ciara Power, Telemetry library maintainer
> >
> > > From: fengchengwen [mailto:fengchengwen@huawei.com]
> > > Sent: Saturday, 18 June 2022 05.52
> > >
> > > On 2022/6/18 1:05, Stephen Hemminger wrote:
> > > > On Fri, 17 Jun 2022 12:25:04 +0100
> > > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > >
> > > >> On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
> > > >>>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > > >>>> Sent: Friday, 17 June 2022 11.46
> > > >>>>
> > > >>>> This patch supports escape special characters (including:
> > > \",\\,/,\b,
> > > >>>> /f,/n,/r,/t) when telemetry string.
> > > >>>> This patch is used to support telemetry xxx-dump commands which
> > > the
> > > >>>> string may include special characters.
> > > >>>>
> > > >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > > >>>> ---
> > > >>>>  lib/telemetry/telemetry.c | 96
> > > +++++++++++++++++++++++++++++++++++++--
> > > >>>>  1 file changed, 93 insertions(+), 3 deletions(-)
> > > >>>>
> > > >>>> diff --git a/lib/telemetry/telemetry.c
> > > >>>> b/lib/telemetry/telemetry.c index c6fd03a5ab..0f762f633e 100644
> > > >>>> --- a/lib/telemetry/telemetry.c
> > > >>>> +++ b/lib/telemetry/telemetry.c
> > > >>>> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data
> > > *d,
> > > >>>> char *out_buf, size_t buf_len)
> > > >>>>        return used;
> > > >>>>  }
> > > >>>>
> > > >>>> +static bool
> > > >>>> +json_is_special_char(char ch)
> > > >>>> +{
> > > >>>> +      static unsigned char is_spec[256] = { 0 };
> > > >>>> +      static bool init_once;
> > > >>>> +
> > > >>>> +      if (!init_once) {
> > > >>>> +              is_spec['\"'] = 1;
> > > >>>> +              is_spec['\\'] = 1;
> > > >>>> +              is_spec['/'] = 1;
> > > >>>> +              is_spec['\b'] = 1;
> > > >>>> +              is_spec['\f'] = 1;
> > > >>>> +              is_spec['\n'] = 1;
> > > >>>> +              is_spec['\r'] = 1;
> > > >>>> +              is_spec['\t'] = 1;
> > > >>>> +              init_once = true;
> > > >>>> +      }
> > > >>>> +
> > > >>>> +      return (bool)is_spec[(unsigned char)ch]; }
> > > >>
> > > >> According to the json spec at [1], the characters that need to be
> > > escaped
> > > >> are:
> > > >> a) any characters <0x20
> > > >> b) inverted commas/quote character \"
> > > >> c) the "reverse solidus character", better known to you and I as
> > > >> the back-slash.
> > > >>
> > > >> Therefore, I think this table generation could be simplified, but
> > > also
> > > >> expanded using this. For completeness we should also see about
> > > handling all
> > > >> control characters if they are encountered.
> > > >>
> > > >> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > > >>
> > > >> /Bruce
> > > >
> > > > Since it is trivial could be initializer?
> > > >
> > > > static const uint8_t is_spec[256] = {
> > > >    [0 ... 0x20] = 1,
> > > >    ['\"' ] = 1,
> > > >    ['\\' ] = 1,
> > > >    ['/'] = 1,
> > > >
> > > > etc
> > > >
> > > > Or we could change the telemetry API to disallow control characters?
> > >
> > > I was thinking about converting 0~0x20, but I don't think there's a
> > > scenario.
> > >
> > > I prefer change the telemetry API to disallow control characters, and
> > > this may not be a violation of the ABI, if yes, the dpdk-telemetry.py
> > > will returns an error.
> >
> > I agree with Chengwen Feng. The telemetry data type is STRING, not BLOB.
> >
> > So we need to define exactly what the STRING type contains.
> >
> > I hope we can all agree that control characters should be disallowed.
> >
> > The more complicated question is: Do we want to use the ASCII character set
> > only, or do we want to use UTF-8 encoded Unicode?
> >
> > Personally, think UTF-8 encoded Unicode is more future proof, and would
> > vote for that.
> >
> > But I wouldn't reject limiting it to ASCII, and perhaps in the future introduce
> > another data type for UTF-8 strings.
> >
> > UTF-8 is the modern choice, but it is incompatible with old stuff, e.g. many
> > SNMP MIBs.
> >
> [CP]
> 
> Just from looking at the spec [1] , I would say UTF-8, as it seems to suggest its use for JSON (section 8.1).
> 
> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> 
> > >
> > > So I think we could add declaring and checking functions to make sure
> > > telemetry string do not allow control characters.
> [CP]
> 
> I am not sure why we don't want these at all - I thought we do want some of them, like tab (\u0009) for example.
> 
> <snip>
> 
> In general, I think Bruce's suggestion of using a customised printf function instead of snprintf would be a good way forward, to scan the chars as they are being copied in.
>

I'm hoping to have some time to try and prototype this myself soon, and
send out a draft patch to this mailing list for consideration.

/Bruce 

Re: [PATCH v2 1/5] telemetry: escape special char when tel string

[Bruce Richardson]

On Wed, Jun 22, 2022 at 10:19:48AM +0100, Bruce Richardson wrote:
> On Wed, Jun 22, 2022 at 08:57:43AM +0100, Power, Ciara wrote:
> > Hi folks,
> > 
> > > -----Original Message-----
> > > From: Morten Brørup <mb@smartsharesystems.com>
> > > Sent: Saturday 18 June 2022 10:59
> > > To: fengchengwen <fengchengwen@huawei.com>; Stephen Hemminger
> > > <stephen@networkplumber.org>; Richardson, Bruce
> > > <bruce.richardson@intel.com>
> > > Cc: thomas@monjalon.net; ferruh.yigit@xilinx.com; Laatz, Kevin
> > > <kevin.laatz@intel.com>; andrew.rybchenko@oktetlabs.ru;
> > > jerinj@marvell.com; sachin.saxena@oss.nxp.com;
> > > hemant.agrawal@nxp.com; dev@dpdk.org; Power, Ciara
> > > <ciara.power@intel.com>
> > > Subject: RE: [PATCH v2 1/5] telemetry: escape special char when tel string
> > >
> > > +CC: Ciara Power, Telemetry library maintainer
> > >
> > > > From: fengchengwen [mailto:fengchengwen@huawei.com]
> > > > Sent: Saturday, 18 June 2022 05.52
> > > >
> > > > On 2022/6/18 1:05, Stephen Hemminger wrote:
> > > > > On Fri, 17 Jun 2022 12:25:04 +0100
> > > > > Bruce Richardson <bruce.richardson@intel.com> wrote:
> > > > >
> > > > >> On Fri, Jun 17, 2022 at 01:16:08PM +0200, Morten Brørup wrote:
> > > > >>>> From: Chengwen Feng [mailto:fengchengwen@huawei.com]
> > > > >>>> Sent: Friday, 17 June 2022 11.46
> > > > >>>>
> > > > >>>> This patch supports escape special characters (including:
> > > > \",\\,/,\b,
> > > > >>>> /f,/n,/r,/t) when telemetry string.
> > > > >>>> This patch is used to support telemetry xxx-dump commands which
> > > > the
> > > > >>>> string may include special characters.
> > > > >>>>
> > > > >>>> Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
> > > > >>>> ---
> > > > >>>>  lib/telemetry/telemetry.c | 96
> > > > +++++++++++++++++++++++++++++++++++++--
> > > > >>>>  1 file changed, 93 insertions(+), 3 deletions(-)
> > > > >>>>
> > > > >>>> diff --git a/lib/telemetry/telemetry.c
> > > > >>>> b/lib/telemetry/telemetry.c index c6fd03a5ab..0f762f633e 100644
> > > > >>>> --- a/lib/telemetry/telemetry.c
> > > > >>>> +++ b/lib/telemetry/telemetry.c
> > > > >>>> @@ -215,6 +215,94 @@ container_to_json(const struct rte_tel_data
> > > > *d,
> > > > >>>> char *out_buf, size_t buf_len)
> > > > >>>>        return used;
> > > > >>>>  }
> > > > >>>>
> > > > >>>> +static bool
> > > > >>>> +json_is_special_char(char ch)
> > > > >>>> +{
> > > > >>>> +      static unsigned char is_spec[256] = { 0 };
> > > > >>>> +      static bool init_once;
> > > > >>>> +
> > > > >>>> +      if (!init_once) {
> > > > >>>> +              is_spec['\"'] = 1;
> > > > >>>> +              is_spec['\\'] = 1;
> > > > >>>> +              is_spec['/'] = 1;
> > > > >>>> +              is_spec['\b'] = 1;
> > > > >>>> +              is_spec['\f'] = 1;
> > > > >>>> +              is_spec['\n'] = 1;
> > > > >>>> +              is_spec['\r'] = 1;
> > > > >>>> +              is_spec['\t'] = 1;
> > > > >>>> +              init_once = true;
> > > > >>>> +      }
> > > > >>>> +
> > > > >>>> +      return (bool)is_spec[(unsigned char)ch]; }
> > > > >>
> > > > >> According to the json spec at [1], the characters that need to be
> > > > escaped
> > > > >> are:
> > > > >> a) any characters <0x20
> > > > >> b) inverted commas/quote character \"
> > > > >> c) the "reverse solidus character", better known to you and I as
> > > > >> the back-slash.
> > > > >>
> > > > >> Therefore, I think this table generation could be simplified, but
> > > > also
> > > > >> expanded using this. For completeness we should also see about
> > > > handling all
> > > > >> control characters if they are encountered.
> > > > >>
> > > > >> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > > > >>
> > > > >> /Bruce
> > > > >
> > > > > Since it is trivial could be initializer?
> > > > >
> > > > > static const uint8_t is_spec[256] = {
> > > > >    [0 ... 0x20] = 1,
> > > > >    ['\"' ] = 1,
> > > > >    ['\\' ] = 1,
> > > > >    ['/'] = 1,
> > > > >
> > > > > etc
> > > > >
> > > > > Or we could change the telemetry API to disallow control characters?
> > > >
> > > > I was thinking about converting 0~0x20, but I don't think there's a
> > > > scenario.
> > > >
> > > > I prefer change the telemetry API to disallow control characters, and
> > > > this may not be a violation of the ABI, if yes, the dpdk-telemetry.py
> > > > will returns an error.
> > >
> > > I agree with Chengwen Feng. The telemetry data type is STRING, not BLOB.
> > >
> > > So we need to define exactly what the STRING type contains.
> > >
> > > I hope we can all agree that control characters should be disallowed.
> > >
> > > The more complicated question is: Do we want to use the ASCII character set
> > > only, or do we want to use UTF-8 encoded Unicode?
> > >
> > > Personally, think UTF-8 encoded Unicode is more future proof, and would
> > > vote for that.
> > >
> > > But I wouldn't reject limiting it to ASCII, and perhaps in the future introduce
> > > another data type for UTF-8 strings.
> > >
> > > UTF-8 is the modern choice, but it is incompatible with old stuff, e.g. many
> > > SNMP MIBs.
> > >
> > [CP]
> > 
> > Just from looking at the spec [1] , I would say UTF-8, as it seems to suggest its use for JSON (section 8.1).
> > 
> > [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > 
> > > >
> > > > So I think we could add declaring and checking functions to make sure
> > > > telemetry string do not allow control characters.
> > [CP]
> > 
> > I am not sure why we don't want these at all - I thought we do want some of them, like tab (\u0009) for example.
> > 
> > <snip>
> > 
> > In general, I think Bruce's suggestion of using a customised printf function instead of snprintf would be a good way forward, to scan the chars as they are being copied in.
> >
> 
> I'm hoping to have some time to try and prototype this myself soon, and
> send out a draft patch to this mailing list for consideration.
> 
Here is an RFC of my current prototype of this:

http://patches.dpdk.org/project/dpdk/list/?series=23739

Feedback welcome.

Regards,
/Bruce

[PATCH v3 0/4] support telemetry dump dev

[Chengwen Feng]

This patchset contains four patch which support telemetry dump
dmadev/eventdev/rawdev/ethdev.

---
v3:
* rebase on [PATCH v3 00/13] telemetry JSON escaping and other enhancements
v2:
* Address Morten and Bruce's comments.

Depends-on: series-116128 ("[PATCH v3 00/13] telemetry JSON escaping and other enhancements")

Chengwen Feng (4):
  dmadev: support telemetry dump dmadev
  eventdev: support telemetry dump eventdev
  rawdev: support telemetry dump rawdev
  ethdev: support telemetry private dump

 lib/dmadev/rte_dmadev.c     | 39 +++++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c     | 43 +++++++++++++++++++++++++++++++++++++
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
 lib/rawdev/rte_rawdev.c     | 42 ++++++++++++++++++++++++++++++++++++
 4 files changed, 167 insertions(+)

-- 
2.17.1

[PATCH v3 1/4] dmadev: support telemetry dump dmadev

[Chengwen Feng]

This patch supports telemetry dump dmadev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/dmadev/rte_dmadev.c | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 174d4c40ae..72c3989f60 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -985,6 +985,43 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+dmadev_handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_dma_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(dmadev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/dmadev/list", dmadev_handle_dev_list,
@@ -993,4 +1030,6 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
+			"Returns dump information for a dmadev. Parameters: int dev_id");
 }
-- 
2.17.1

[PATCH v3 2/4] eventdev: support telemetry dump eventdev

[Chengwen Feng]

This patch supports telemetry dump eventdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/eventdev/rte_eventdev.c b/lib/eventdev/rte_eventdev.c
index 1dc4f966be..5b1bd2b132 100644
--- a/lib/eventdev/rte_eventdev.c
+++ b/lib/eventdev/rte_eventdev.c
@@ -1791,6 +1791,47 @@ handle_queue_xstats(const char *cmd __rte_unused,
 	return eventdev_build_telemetry_data(dev_id, mode, port_queue_id, d);
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	/* Get dev ID from parameter string */
+	dev_id = strtoul(params, &end_param, 10);
+	if (*end_param != '\0')
+		RTE_EDEV_LOG_DEBUG(
+			"Extra parameters passed to eventdev telemetry command, ignoring");
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_event_dev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(eventdev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/eventdev/dev_list", handle_dev_list,
@@ -1807,6 +1848,8 @@ RTE_INIT(eventdev_init_telemetry)
 	rte_telemetry_register_cmd("/eventdev/queue_xstats",
 			handle_queue_xstats,
 			"Returns stats for an eventdev queue. Params: DevID,QueueID");
+	rte_telemetry_register_cmd("/eventdev/dev_dump", handle_dev_dump,
+			"Returns dump information for an eventdev. Parameter: DevID");
 	rte_telemetry_register_cmd("/eventdev/queue_links", handle_queue_links,
 			"Returns links for an eventdev port. Params: DevID,QueueID");
 }
-- 
2.17.1

[PATCH v3 3/4] rawdev: support telemetry dump rawdev

[Chengwen Feng]

This patch supports telemetry dump rawdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/rawdev/rte_rawdev.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/lib/rawdev/rte_rawdev.c b/lib/rawdev/rte_rawdev.c
index 2f0a4f132e..a4cdd418f8 100644
--- a/lib/rawdev/rte_rawdev.c
+++ b/lib/rawdev/rte_rawdev.c
@@ -613,6 +613,46 @@ handle_dev_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_RDEV_LOG(NOTICE,
+			"Extra parameters passed to rawdev telemetry command, ignoring");
+	if (!rte_rawdev_pmd_is_valid_dev(dev_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_rawdev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 RTE_LOG_REGISTER_DEFAULT(librawdev_logtype, INFO);
 
 RTE_INIT(librawdev_init_telemetry)
@@ -621,4 +661,6 @@ RTE_INIT(librawdev_init_telemetry)
 			"Returns list of available rawdev ports. Takes no parameters");
 	rte_telemetry_register_cmd("/rawdev/xstats", handle_dev_xstats,
 			"Returns the xstats for a rawdev port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/rawdev/dump", handle_dev_dump,
+			"Returns dump information for a rawdev port. Parameters: int port_id");
 }
-- 
2.17.1

[PATCH v3 4/4] ethdev: support telemetry private dump

[Chengwen Feng]

This patch supports telemetry private dump a ethdev port.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ethdev/rte_ethdev.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 1979dc0850..c34a9f0c4a 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -7,6 +7,7 @@
 #include <inttypes.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/queue.h>
@@ -5644,6 +5645,46 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
+			const char *params,
+			struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int port_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_eth_dev_priv_dump(port_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 static int
 eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 		const char *params,
@@ -5927,6 +5968,8 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
+			"Returns dump private information for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/link_status",
 			eth_dev_handle_port_link_status,
 			"Returns the link status for a port. Parameters: int port_id");
-- 
2.17.1

[PATCH v4 0/4] support telemetry dump dev

[Chengwen Feng]

This patchset contains four patch which support telemetry dump
dmadev/eventdev/rawdev/ethdev.

---
v4:
* fix compile fail with windows platform due to no fmemopen API.
v3:
* rebase on [PATCH v3 00/13] telemetry JSON escaping and other enhancements
v2:
* Address Morten and Bruce's comments.

Depends-on: series-116128 ("[PATCH v3 00/13] telemetry JSON escaping and other enhancements")

Chengwen Feng (4):
  dmadev: support telemetry dump dmadev
  eventdev: support telemetry dump eventdev
  rawdev: support telemetry dump rawdev
  ethdev: support telemetry private dump

 lib/dmadev/rte_dmadev.c     | 43 +++++++++++++++++++++++++++++++++
 lib/ethdev/rte_ethdev.c     | 47 +++++++++++++++++++++++++++++++++++++
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++
 lib/rawdev/rte_rawdev.c     | 42 +++++++++++++++++++++++++++++++++
 4 files changed, 175 insertions(+)

-- 
2.17.1

[PATCH v4 1/4] dmadev: support telemetry dump dmadev

[Chengwen Feng]

This patch supports telemetry dump dmadev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/dmadev/rte_dmadev.c | 43 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/dmadev/rte_dmadev.c b/lib/dmadev/rte_dmadev.c
index 174d4c40ae..ea1cb815b4 100644
--- a/lib/dmadev/rte_dmadev.c
+++ b/lib/dmadev/rte_dmadev.c
@@ -985,6 +985,45 @@ dmadev_handle_dev_stats(const char *cmd __rte_unused,
 	return 0;
 }
 
+#ifndef RTE_EXEC_ENV_WINDOWS
+static int
+dmadev_handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_DMA_LOG(WARNING, "Extra parameters passed to dmadev telemetry command, ignoring");
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_dma_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+#endif /* !RTE_EXEC_ENV_WINDOWS */
+
 RTE_INIT(dmadev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/dmadev/list", dmadev_handle_dev_list,
@@ -993,4 +1032,8 @@ RTE_INIT(dmadev_init_telemetry)
 			"Returns information for a dmadev. Parameters: int dev_id");
 	rte_telemetry_register_cmd("/dmadev/stats", dmadev_handle_dev_stats,
 			"Returns the stats for a dmadev vchannel. Parameters: int dev_id, vchan_id (Optional if only one vchannel)");
+#ifndef RTE_EXEC_ENV_WINDOWS
+	rte_telemetry_register_cmd("/dmadev/dump", dmadev_handle_dev_dump,
+			"Returns dump information for a dmadev. Parameters: int dev_id");
+#endif
 }
-- 
2.17.1

[PATCH v4 2/4] eventdev: support telemetry dump eventdev

[Chengwen Feng]

This patch supports telemetry dump eventdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/eventdev/rte_eventdev.c | 43 +++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

diff --git a/lib/eventdev/rte_eventdev.c b/lib/eventdev/rte_eventdev.c
index 1dc4f966be..5b1bd2b132 100644
--- a/lib/eventdev/rte_eventdev.c
+++ b/lib/eventdev/rte_eventdev.c
@@ -1791,6 +1791,47 @@ handle_queue_xstats(const char *cmd __rte_unused,
 	return eventdev_build_telemetry_data(dev_id, mode, port_queue_id, d);
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -1;
+
+	/* Get dev ID from parameter string */
+	dev_id = strtoul(params, &end_param, 10);
+	if (*end_param != '\0')
+		RTE_EDEV_LOG_DEBUG(
+			"Extra parameters passed to eventdev telemetry command, ignoring");
+
+	RTE_EVENTDEV_VALID_DEVID_OR_ERR_RET(dev_id, -EINVAL);
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_event_dev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return ret;
+}
+
 RTE_INIT(eventdev_init_telemetry)
 {
 	rte_telemetry_register_cmd("/eventdev/dev_list", handle_dev_list,
@@ -1807,6 +1848,8 @@ RTE_INIT(eventdev_init_telemetry)
 	rte_telemetry_register_cmd("/eventdev/queue_xstats",
 			handle_queue_xstats,
 			"Returns stats for an eventdev queue. Params: DevID,QueueID");
+	rte_telemetry_register_cmd("/eventdev/dev_dump", handle_dev_dump,
+			"Returns dump information for an eventdev. Parameter: DevID");
 	rte_telemetry_register_cmd("/eventdev/queue_links", handle_queue_links,
 			"Returns links for an eventdev port. Params: DevID,QueueID");
 }
-- 
2.17.1

[PATCH v4 3/4] rawdev: support telemetry dump rawdev

[Chengwen Feng]

This patch supports telemetry dump rawdev.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/rawdev/rte_rawdev.c | 42 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 42 insertions(+)

diff --git a/lib/rawdev/rte_rawdev.c b/lib/rawdev/rte_rawdev.c
index 2f0a4f132e..a4cdd418f8 100644
--- a/lib/rawdev/rte_rawdev.c
+++ b/lib/rawdev/rte_rawdev.c
@@ -613,6 +613,46 @@ handle_dev_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+static int
+handle_dev_dump(const char *cmd __rte_unused,
+		const char *params,
+		struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int dev_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	dev_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_RDEV_LOG(NOTICE,
+			"Extra parameters passed to rawdev telemetry command, ignoring");
+	if (!rte_rawdev_pmd_is_valid_dev(dev_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_rawdev_dump(dev_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+
 RTE_LOG_REGISTER_DEFAULT(librawdev_logtype, INFO);
 
 RTE_INIT(librawdev_init_telemetry)
@@ -621,4 +661,6 @@ RTE_INIT(librawdev_init_telemetry)
 			"Returns list of available rawdev ports. Takes no parameters");
 	rte_telemetry_register_cmd("/rawdev/xstats", handle_dev_xstats,
 			"Returns the xstats for a rawdev port. Parameters: int port_id");
+	rte_telemetry_register_cmd("/rawdev/dump", handle_dev_dump,
+			"Returns dump information for a rawdev port. Parameters: int port_id");
 }
-- 
2.17.1

[PATCH v4 4/4] ethdev: support telemetry private dump

[Chengwen Feng]

This patch supports telemetry private dump a ethdev port.

Signed-off-by: Chengwen Feng <fengchengwen@huawei.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/ethdev/rte_ethdev.c | 47 +++++++++++++++++++++++++++++++++++++++++
 1 file changed, 47 insertions(+)

diff --git a/lib/ethdev/rte_ethdev.c b/lib/ethdev/rte_ethdev.c
index 1979dc0850..a19b1215be 100644
--- a/lib/ethdev/rte_ethdev.c
+++ b/lib/ethdev/rte_ethdev.c
@@ -7,6 +7,7 @@
 #include <inttypes.h>
 #include <stdbool.h>
 #include <stdint.h>
+#include <stdio.h>
 #include <stdlib.h>
 #include <string.h>
 #include <sys/queue.h>
@@ -5644,6 +5645,48 @@ eth_dev_handle_port_xstats(const char *cmd __rte_unused,
 	return 0;
 }
 
+#ifndef RTE_EXEC_ENV_WINDOWS
+static int
+eth_dev_handle_port_dump_priv(const char *cmd __rte_unused,
+			const char *params,
+			struct rte_tel_data *d)
+{
+	char *buf, *end_param;
+	int port_id, ret;
+	FILE *f;
+
+	if (params == NULL || strlen(params) == 0 || !isdigit(*params))
+		return -EINVAL;
+
+	port_id = strtoul(params, &end_param, 0);
+	if (*end_param != '\0')
+		RTE_ETHDEV_LOG(NOTICE,
+			"Extra parameters passed to ethdev telemetry command, ignoring");
+	if (!rte_eth_dev_is_valid_port(port_id))
+		return -EINVAL;
+
+	buf = calloc(sizeof(char), RTE_TEL_MAX_SINGLE_STRING_LEN);
+	if (buf == NULL)
+		return -ENOMEM;
+
+	f = fmemopen(buf, RTE_TEL_MAX_SINGLE_STRING_LEN - 1, "w+");
+	if (f == NULL) {
+		free(buf);
+		return -EINVAL;
+	}
+
+	ret = rte_eth_dev_priv_dump(port_id, f);
+	fclose(f);
+	if (ret == 0) {
+		rte_tel_data_start_dict(d);
+		rte_tel_data_string(d, buf);
+	}
+
+	free(buf);
+	return 0;
+}
+#endif /* !RTE_EXEC_ENV_WINDOWS */
+
 static int
 eth_dev_handle_port_link_status(const char *cmd __rte_unused,
 		const char *params,
@@ -5927,6 +5970,10 @@ RTE_INIT(ethdev_init_telemetry)
 			"Returns the common stats for a port. Parameters: int port_id");
 	rte_telemetry_register_cmd("/ethdev/xstats", eth_dev_handle_port_xstats,
 			"Returns the extended stats for a port. Parameters: int port_id");
+#ifndef RTE_EXEC_ENV_WINDOWS
+	rte_telemetry_register_cmd("/ethdev/dump_priv", eth_dev_handle_port_dump_priv,
+			"Returns dump private information for a port. Parameters: int port_id");
+#endif
 	rte_telemetry_register_cmd("/ethdev/link_status",
 			eth_dev_handle_port_link_status,
 			"Returns the link status for a port. Parameters: int port_id");
-- 
2.17.1

Re: [PATCH v4 0/4] support telemetry dump dev

[David Marchand]

On Tue, Sep 13, 2022 at 9:20 AM Chengwen Feng <fengchengwen@huawei.com> wrote:
>
> This patchset contains four patch which support telemetry dump
> dmadev/eventdev/rawdev/ethdev.
>
> ---
> v4:
> * fix compile fail with windows platform due to no fmemopen API.
> v3:
> * rebase on [PATCH v3 00/13] telemetry JSON escaping and other enhancements
> v2:
> * Address Morten and Bruce's comments.
>
> Depends-on: series-116128 ("[PATCH v3 00/13] telemetry JSON escaping and other enhancements")
>
> Chengwen Feng (4):
>   dmadev: support telemetry dump dmadev
>   eventdev: support telemetry dump eventdev
>   rawdev: support telemetry dump rawdev
>   ethdev: support telemetry private dump

Series applied, thanks.


-- 
David Marchand