[RFC PATCH 0/6] add json string escaping to telemetry

[RFC PATCH 0/6] add json string escaping to telemetry

[Bruce Richardson]

This RFC shows one possible approach for escaping strings for the json
output of telemetry library. For now this RFC supports escaping strings
for the cases of returning a single string, or returning an array of
strings. Not done is escaping of strings in objs/dicts [see more below
on TODO]

As well as telemetry lib changes, this patchset includes unit tests for
the above and also little bit of cleanup to the json tests.

TODO:
Beyond what is here in this RFC:

1. we need to decide what to do about name/value pairs. Personally, I
   think we should add the restriction to the "rte_tel_data_add_obj_*" APIs
   to only allow a defined subset of characters in names: e.g. alphanumeric
   chars, underscore and dash. That means that we only need to escape
   the data part in the case of string returns.

2. once agreed, need to implement a patch to escape strings in
   dicts/objs

3. need to add a patch to escape the input command if it contains
   invalid chars

4. some small refactoring of the main telemetry.c json-encoding function
   may be possible.

Bruce Richardson (6):
  test/telemetry_json: print success or failure per subtest
  telemetry: fix escaping of invalid json characters
  telemetry: use json string function for string outputs
  test/telemetry_json: add test for string character escaping
  telemetry: add escaping of strings in arrays
  test/telemetry-json: add test case for escaping strings in arrays

 app/test/test_telemetry_json.c | 74 +++++++++++++++++++++++++++++-----
 lib/telemetry/telemetry.c      | 11 +++--
 lib/telemetry/telemetry_json.h | 62 ++++++++++++++++++++++++++--
 3 files changed, 132 insertions(+), 15 deletions(-)

--
2.34.1

[RFC PATCH 1/6] test/telemetry_json: print success or failure per subtest

[Bruce Richardson]

rather than just printing out success or failure at the end of the test
only, print out "OK" or "ERROR" for each individual test case within the
overall test. As part of this, ensure each case returns 0 on success and
any other value on failure.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 790181d316..748b7cfe5a 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -102,8 +102,10 @@ test_large_array_element(void)
 
 	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, str);
 	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != 0)
+		return -1;
 
-	return strlen(buf) != 0;
+	return strncmp(expected, buf, sizeof(buf));
 }
 
 static int
@@ -117,20 +119,33 @@ test_large_obj_element(void)
 
 	used = rte_tel_json_add_obj_u64(buf, sizeof(buf), used, str, 0);
 	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != 0)
+		return -1;
 
-	return strlen(buf) != 0;
+	return strncmp(expected, buf, sizeof(buf));
 }
 
+typedef int (*test_fn)(void);
+
 static int
 test_telemetry_json(void)
 {
-	if (test_basic_array() < 0 ||
-			test_basic_obj() < 0 ||
-			test_overflow_array() < 0 ||
-			test_overflow_obj() < 0 ||
-			test_large_array_element() < 0 ||
-			test_large_obj_element() < 0)
-		return -1;
+	unsigned int i;
+	test_fn fns[] = {
+			test_basic_array,
+			test_basic_obj,
+			test_overflow_array,
+			test_overflow_obj,
+			test_large_array_element,
+			test_large_obj_element,
+	};
+	for (i = 0; i < RTE_DIM(fns); i++)
+		if (fns[i]() == 0)
+			printf("OK\n");
+		else {
+			printf("ERROR\n");
+			return -1;
+		}
 	return 0;
 }
 
-- 
2.34.1

[RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

For string values returned from telemetry, escape any values that cannot
normally appear in a json string. According to the json spec[1], the
characters than need to be handled are control chars (char value < 0x20)
and '"' and '\' characters.

To handle this, we replace the snprintf call with a separate string
copying and encapsulation routine which checks each character as it
copies it to the final array.

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

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
 1 file changed, 47 insertions(+), 1 deletion(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index db70690274..13df5d07e3 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char *format, ...)
 	return 0; /* nothing written or modified */
 }
 
+static const char control_chars[0x20] = {
+		['\n'] = 'n',
+		['\r'] = 'r',
+		['\t'] = 't',
+};
+
+/**
+ * @internal
+ * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
+ * except that it does proper escaping as necessary.
+ * Drops any invalid characters we don't support
+ */
+static inline int
+__json_format_str(char *buf, const int len, const char *str)
+{
+	char tmp[len];
+	int tmpidx = 0;
+
+	tmp[tmpidx++] = '"';
+	while (*str != '\0') {
+		if (*str < (int)RTE_DIM(control_chars)) {
+			int idx = *str;  /* compilers don't like char type as index */
+			if (control_chars[idx] != 0) {
+				tmp[tmpidx++] = '\\';
+				tmp[tmpidx++] = control_chars[idx];
+			}
+		} else if (*str == '"' || *str == '\\') {
+			tmp[tmpidx++] = '\\';
+			tmp[tmpidx++] = *str;
+		} else
+			tmp[tmpidx++] = *str;
+		/* we always need space for closing quote and null character.
+		 * Ensuring at least two free characters also means we can always take an
+		 * escaped character like "\n" without overflowing
+		 */
+		if (tmpidx > len - 2)
+			return 0;
+		str++;
+	}
+	tmp[tmpidx++] = '"';
+	tmp[tmpidx] = '\0';
+
+	strcpy(buf, tmp);
+	return tmpidx;
+}
+
 /* Copies an empty array into the provided buffer. */
 static inline int
 rte_tel_json_empty_array(char *buf, const int len, const int used)
@@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
 static inline int
 rte_tel_json_str(char *buf, const int len, const int used, const char *str)
 {
-	return used + __json_snprintf(buf + used, len - used, "\"%s\"", str);
+	return used + __json_format_str(buf + used, len - used, str);
 }
 
 /* Appends a string into the JSON array in the provided buffer. */
-- 
2.34.1

RE: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 23 June 2022 18.43
> 
> For string values returned from telemetry, escape any values that
> cannot
> normally appear in a json string. According to the json spec[1], the
> characters than need to be handled are control chars (char value <
> 0x20)
> and '"' and '\' characters.

Correct. Other chars are optional to escape.

> 
> To handle this, we replace the snprintf call with a separate string
> copying and encapsulation routine which checks each character as it
> copies it to the final array.
> 
> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
>  1 file changed, 47 insertions(+), 1 deletion(-)
> 
> diff --git a/lib/telemetry/telemetry_json.h
> b/lib/telemetry/telemetry_json.h
> index db70690274..13df5d07e3 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char
> *format, ...)
>  	return 0; /* nothing written or modified */
>  }
> 
> +static const char control_chars[0x20] = {
> +		['\n'] = 'n',
> +		['\r'] = 'r',
> +		['\t'] = 't',
> +};
> +
> +/**
> + * @internal
> + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> + * except that it does proper escaping as necessary.
> + * Drops any invalid characters we don't support
> + */
> +static inline int
> +__json_format_str(char *buf, const int len, const char *str)
> +{
> +	char tmp[len];
> +	int tmpidx = 0;
> +
> +	tmp[tmpidx++] = '"';
> +	while (*str != '\0') {
> +		if (*str < (int)RTE_DIM(control_chars)) {

I would prefer the more explicit 0x20, directly copied from the RFC. RTE_DIM(control_chars) hints that it could change.

> +			int idx = *str;  /* compilers don't like char type as
> index */
> +			if (control_chars[idx] != 0) {
> +				tmp[tmpidx++] = '\\';
> +				tmp[tmpidx++] = control_chars[idx];
> +			}

Consider support for other control characters:
+			else {
+				tmp[tmpidx++] = '\\';
+				tmp[tmpidx++] = 'u';
+				tmp[tmpidx++] = '0';
+				tmp[tmpidx++] = '0';
+				tmp[tmpidx++] = hexchar(idx >> 4);
+				tmp[tmpidx++] = hexchar(idx & 0xf);
+			}

Or just drop them, as you mention in the function's description.

> +		} else if (*str == '"' || *str == '\\') {
> +			tmp[tmpidx++] = '\\';
> +			tmp[tmpidx++] = *str;
> +		} else
> +			tmp[tmpidx++] = *str;
> +		/* we always need space for closing quote and null
> character.
> +		 * Ensuring at least two free characters also means we can
> always take an
> +		 * escaped character like "\n" without overflowing
> +		 */
> +		if (tmpidx > len - 2)

If supporting the \u00XX encoding, you need to reserve more than 2 characters here and in related code.

> +			return 0;
> +		str++;
> +	}
> +	tmp[tmpidx++] = '"';
> +	tmp[tmpidx] = '\0';
> +
> +	strcpy(buf, tmp);
> +	return tmpidx;
> +}
> +
>  /* Copies an empty array into the provided buffer. */
>  static inline int
>  rte_tel_json_empty_array(char *buf, const int len, const int used)
> @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len,
> const int used)
>  static inline int
>  rte_tel_json_str(char *buf, const int len, const int used, const char
> *str)
>  {
> -	return used + __json_snprintf(buf + used, len - used, "\"%s\"",
> str);
> +	return used + __json_format_str(buf + used, len - used, str);
>  }
> 
>  /* Appends a string into the JSON array in the provided buffer. */
> --
> 2.34.1
> 

Re: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Stephen Hemminger]

On Thu, 23 Jun 2022 20:34:07 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> > 
> > For string values returned from telemetry, escape any values that
> > cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value <
> > 0x20)
> > and '"' and '\' characters.  
> 
> Correct. Other chars are optional to escape.

For json_writer (which I wrote for iproute2 and could have been used here).
The switch handles: \t \n \r \f \b \\ " ' as special cases.

RE: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Morten Brørup]

> From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> Sent: Thursday, 23 June 2022 20.40
> 
> On Thu, 23 Jun 2022 20:34:07 +0200
> Morten Brørup <mb@smartsharesystems.com> wrote:
> 
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Thursday, 23 June 2022 18.43
> > >
> > > For string values returned from telemetry, escape any values that
> > > cannot
> > > normally appear in a json string. According to the json spec[1],
> the
> > > characters than need to be handled are control chars (char value <
> > > 0x20)
> > > and '"' and '\' characters.
> >
> > Correct. Other chars are optional to escape.
> 
> For json_writer (which I wrote for iproute2 and could have been used
> here).
> The switch handles: \t \n \r \f \b \\ " ' as special cases.

RFC 8259 chapter 7 says:

   All Unicode characters may be placed within the
   quotation marks, except for the characters that MUST be escaped:
   quotation mark, reverse solidus, and the control characters (U+0000
   through U+001F).

I have no preference for either, as long as '/' and other non-control characters are not (unnecessarily) escaped.

Using tested and maintained code like json_writer could be beneficial. If you hold the copyright, there should be no license issues.

Re: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > Sent: Thursday, 23 June 2022 20.40
> > 
> > On Thu, 23 Jun 2022 20:34:07 +0200
> > Morten Brørup <mb@smartsharesystems.com> wrote:
> > 
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Thursday, 23 June 2022 18.43
> > > >
> > > > For string values returned from telemetry, escape any values that
> > > > cannot
> > > > normally appear in a json string. According to the json spec[1],
> > the
> > > > characters than need to be handled are control chars (char value <
> > > > 0x20)
> > > > and '"' and '\' characters.
> > >
> > > Correct. Other chars are optional to escape.
> > 
> > For json_writer (which I wrote for iproute2 and could have been used
> > here).
> > The switch handles: \t \n \r \f \b \\ " ' as special cases.
> 
> RFC 8259 chapter 7 says:
> 
>    All Unicode characters may be placed within the
>    quotation marks, except for the characters that MUST be escaped:
>    quotation mark, reverse solidus, and the control characters (U+0000
>    through U+001F).
> 
> I have no preference for either, as long as '/' and other non-control characters are not (unnecessarily) escaped.
> 
> Using tested and maintained code like json_writer could be beneficial. If you hold the copyright, there should be no license issues.
> 

I will take a look at json_writer.

Re: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

On Fri, Jun 24, 2022 at 09:00:38AM +0100, Bruce Richardson wrote:
> On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > Sent: Thursday, 23 June 2022 20.40
> > > 
> > > On Thu, 23 Jun 2022 20:34:07 +0200
> > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > 
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Thursday, 23 June 2022 18.43
> > > > >
> > > > > For string values returned from telemetry, escape any values that
> > > > > cannot
> > > > > normally appear in a json string. According to the json spec[1],
> > > the
> > > > > characters than need to be handled are control chars (char value <
> > > > > 0x20)
> > > > > and '"' and '\' characters.
> > > >
> > > > Correct. Other chars are optional to escape.
> > > 
> > > For json_writer (which I wrote for iproute2 and could have been used
> > > here).
> > > The switch handles: \t \n \r \f \b \\ " ' as special cases.
> > 
> > RFC 8259 chapter 7 says:
> > 
> >    All Unicode characters may be placed within the
> >    quotation marks, except for the characters that MUST be escaped:
> >    quotation mark, reverse solidus, and the control characters (U+0000
> >    through U+001F).
> > 
> > I have no preference for either, as long as '/' and other non-control characters are not (unnecessarily) escaped.
> > 
> > Using tested and maintained code like json_writer could be beneficial. If you hold the copyright, there should be no license issues.
> > 
> 
> I will take a look at json_writer.

Took a quick look at json_writer, and it's certainly an option. The main
gap compared to what we have in our current implementation is that
json_writer is designed around a stream for output rather than an output
buffer. Now while we can use fmemopen to make our buffer act as a stream
for writing, and the write apis should prevent it overflowing, we still hit
the issue of the result of truncation not being valid json. The current
implementation tries to handle truncation more gracefully in that any
fields which don't fit just don't get added.

I'll think about it a bit more, and see if there is a way that it can be
made to work more cleanly.

/Bruce

PS: just changing the output from a string to a stream on the output socket
I don't believe is an option either, as the socket type used for telemetry
is a SOCK_SEQPACKET where message boundaries are preserved, and a single
read will return the entire telemetry reply.

RE: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 24 June 2022 13.17
> 
> On Fri, Jun 24, 2022 at 09:00:38AM +0100, Bruce Richardson wrote:
> > On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:
> > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > Sent: Thursday, 23 June 2022 20.40
> > > >
> > > > On Thu, 23 Jun 2022 20:34:07 +0200
> > > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > >
> > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > > Sent: Thursday, 23 June 2022 18.43
> > > > > >
> > > > > > For string values returned from telemetry, escape any values
> that
> > > > > > cannot
> > > > > > normally appear in a json string. According to the json
> spec[1],
> > > > the
> > > > > > characters than need to be handled are control chars (char
> value <
> > > > > > 0x20)
> > > > > > and '"' and '\' characters.
> > > > >
> > > > > Correct. Other chars are optional to escape.
> > > >
> > > > For json_writer (which I wrote for iproute2 and could have been
> used
> > > > here).
> > > > The switch handles: \t \n \r \f \b \\ " ' as special cases.
> > >
> > > RFC 8259 chapter 7 says:
> > >
> > >    All Unicode characters may be placed within the
> > >    quotation marks, except for the characters that MUST be escaped:
> > >    quotation mark, reverse solidus, and the control characters
> (U+0000
> > >    through U+001F).
> > >
> > > I have no preference for either, as long as '/' and other non-
> control characters are not (unnecessarily) escaped.
> > >
> > > Using tested and maintained code like json_writer could be
> beneficial. If you hold the copyright, there should be no license
> issues.
> > >
> >
> > I will take a look at json_writer.
> 
> Took a quick look at json_writer, and it's certainly an option. The
> main
> gap compared to what we have in our current implementation is that
> json_writer is designed around a stream for output rather than an
> output
> buffer. Now while we can use fmemopen to make our buffer act as a
> stream
> for writing, and the write apis should prevent it overflowing, we still
> hit
> the issue of the result of truncation not being valid json. The current
> implementation tries to handle truncation more gracefully in that any
> fields which don't fit just don't get added.
> 
> I'll think about it a bit more, and see if there is a way that it can
> be
> made to work more cleanly.

It sounds like json_writer provides a more advanced API, adding a lot of overhead for wrapping it into the Telemetry library. Since we only need a very simple encoder, perhaps copy-paste-modify is more viable. Or just proceed with your RFC code.

Regardless, the API and underlying code probably needs extra scrutiny, so it doesn't become an attack vector into the control plane of a DPDK application.

> 
> /Bruce
> 
> PS: just changing the output from a string to a stream on the output
> socket
> I don't believe is an option either, as the socket type used for
> telemetry
> is a SOCK_SEQPACKET where message boundaries are preserved, and a
> single
> read will return the entire telemetry reply.

Re: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Stephen Hemminger]

On Fri, 24 Jun 2022 13:29:46 +0200
Morten Brørup <mb@smartsharesystems.com> wrote:

> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 24 June 2022 13.17
> > 
> > On Fri, Jun 24, 2022 at 09:00:38AM +0100, Bruce Richardson wrote:  
> > > On Thu, Jun 23, 2022 at 08:48:21PM +0200, Morten Brørup wrote:  
> > > > > From: Stephen Hemminger [mailto:stephen@networkplumber.org]
> > > > > Sent: Thursday, 23 June 2022 20.40
> > > > >
> > > > > On Thu, 23 Jun 2022 20:34:07 +0200
> > > > > Morten Brørup <mb@smartsharesystems.com> wrote:
> > > > >  
> > > > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > > > Sent: Thursday, 23 June 2022 18.43
> > > > > > >
> > > > > > > For string values returned from telemetry, escape any values  
> > that  
> > > > > > > cannot
> > > > > > > normally appear in a json string. According to the json  
> > spec[1],  
> > > > > the  
> > > > > > > characters than need to be handled are control chars (char  
> > value <  
> > > > > > > 0x20)
> > > > > > > and '"' and '\' characters.  
> > > > > >
> > > > > > Correct. Other chars are optional to escape.  
> > > > >
> > > > > For json_writer (which I wrote for iproute2 and could have been  
> > used  
> > > > > here).
> > > > > The switch handles: \t \n \r \f \b \\ " ' as special cases.  
> > > >
> > > > RFC 8259 chapter 7 says:
> > > >
> > > >    All Unicode characters may be placed within the
> > > >    quotation marks, except for the characters that MUST be escaped:
> > > >    quotation mark, reverse solidus, and the control characters  
> > (U+0000  
> > > >    through U+001F).
> > > >
> > > > I have no preference for either, as long as '/' and other non-  
> > control characters are not (unnecessarily) escaped.  
> > > >
> > > > Using tested and maintained code like json_writer could be  
> > beneficial. If you hold the copyright, there should be no license
> > issues.  
> > > >  
> > >
> > > I will take a look at json_writer.  
> > 
> > Took a quick look at json_writer, and it's certainly an option. The
> > main
> > gap compared to what we have in our current implementation is that
> > json_writer is designed around a stream for output rather than an
> > output
> > buffer. Now while we can use fmemopen to make our buffer act as a
> > stream
> > for writing, and the write apis should prevent it overflowing, we still
> > hit
> > the issue of the result of truncation not being valid json. The current
> > implementation tries to handle truncation more gracefully in that any
> > fields which don't fit just don't get added.
> > 
> > I'll think about it a bit more, and see if there is a way that it can
> > be
> > made to work more cleanly.  
> 
> It sounds like json_writer provides a more advanced API, adding a lot of overhead for wrapping it into the Telemetry library. Since we only need a very simple encoder, perhaps copy-paste-modify is more viable. Or just proceed with your RFC code.
> 
> Regardless, the API and underlying code probably needs extra scrutiny, so it doesn't become an attack vector into the control plane of a DPDK application.

I wrote it based on the model used by some Java library.
Other JSON libraries were more concerned with parsing JSON.

Re: [RFC PATCH 2/6] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

On Thu, Jun 23, 2022 at 08:34:07PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> > 
> > For string values returned from telemetry, escape any values that
> > cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value <
> > 0x20)
> > and '"' and '\' characters.
> 
> Correct. Other chars are optional to escape.
> 
> > 
> > To handle this, we replace the snprintf call with a separate string
> > copying and encapsulation routine which checks each character as it
> > copies it to the final array.
> > 
> > [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
> >  1 file changed, 47 insertions(+), 1 deletion(-)
> > 
> > diff --git a/lib/telemetry/telemetry_json.h
> > b/lib/telemetry/telemetry_json.h
> > index db70690274..13df5d07e3 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char
> > *format, ...)
> >  	return 0; /* nothing written or modified */
> >  }
> > 
> > +static const char control_chars[0x20] = {
> > +		['\n'] = 'n',
> > +		['\r'] = 'r',
> > +		['\t'] = 't',
> > +};
> > +
> > +/**
> > + * @internal
> > + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> > + * except that it does proper escaping as necessary.
> > + * Drops any invalid characters we don't support
> > + */
> > +static inline int
> > +__json_format_str(char *buf, const int len, const char *str)
> > +{
> > +	char tmp[len];
> > +	int tmpidx = 0;
> > +
> > +	tmp[tmpidx++] = '"';
> > +	while (*str != '\0') {
> > +		if (*str < (int)RTE_DIM(control_chars)) {
> 
> I would prefer the more explicit 0x20, directly copied from the RFC. RTE_DIM(control_chars) hints that it could change.
>
Sure. Just trying to avoid magic constants, but in this case it does make
sense. Alternatively, I considered using space char as the sentinel value,
as first non-control-char allowed.
 
> > +			int idx = *str;  /* compilers don't like char type as
> > index */
> > +			if (control_chars[idx] != 0) {
> > +				tmp[tmpidx++] = '\\';
> > +				tmp[tmpidx++] = control_chars[idx];
> > +			}
> 
> Consider support for other control characters:
> +			else {
> +				tmp[tmpidx++] = '\\';
> +				tmp[tmpidx++] = 'u';
> +				tmp[tmpidx++] = '0';
> +				tmp[tmpidx++] = '0';
> +				tmp[tmpidx++] = hexchar(idx >> 4);
> +				tmp[tmpidx++] = hexchar(idx & 0xf);
> +			}
> 
> Or just drop them, as you mention in the function's description.
> 

Yeah, I'd appreciate general feedback on that. Adding support is nice, but
just not sure if we really need it or not.

> > +		} else if (*str == '"' || *str == '\\') {
> > +			tmp[tmpidx++] = '\\';
> > +			tmp[tmpidx++] = *str;
> > +		} else
> > +			tmp[tmpidx++] = *str;
> > +		/* we always need space for closing quote and null
> > character.
> > +		 * Ensuring at least two free characters also means we can
> > always take an
> > +		 * escaped character like "\n" without overflowing
> > +		 */
> > +		if (tmpidx > len - 2)
> 
> If supporting the \u00XX encoding, you need to reserve more than 2 characters here and in related code.
> 
Yep. I avoided supporting it for simplicity for now.

> > +			return 0;
> > +		str++;
> > +	}
> > +	tmp[tmpidx++] = '"';
> > +	tmp[tmpidx] = '\0';
> > +
> > +	strcpy(buf, tmp);
> > +	return tmpidx;
> > +}
> > +
> >  /* Copies an empty array into the provided buffer. */
> >  static inline int
> >  rte_tel_json_empty_array(char *buf, const int len, const int used)
> > @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len,
> > const int used)
> >  static inline int
> >  rte_tel_json_str(char *buf, const int len, const int used, const char
> > *str)
> >  {
> > -	return used + __json_snprintf(buf + used, len - used, "\"%s\"",
> > str);
> > +	return used + __json_format_str(buf + used, len - used, str);
> >  }
> > 
> >  /* Appends a string into the JSON array in the provided buffer. */
> > --
> > 2.34.1
> > 
> 

[RFC PATCH 3/6] telemetry: use json string function for string outputs

[Bruce Richardson]

When returning just a string in response to a query, use the proper json
string printing function to escape characters rather than just snprintf.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry.c | 11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index c6fd03a5ab..7188b1905c 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -232,9 +232,14 @@ 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);
+		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
+				MAX_CMD_LEN, cmd);
+		cb_data_buf = &out_buf[prefix_used];
+		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
+
+		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
+		used += prefix_used;
+		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
 	case RTE_TEL_DICT:
 		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-- 
2.34.1

[RFC PATCH 4/6] test/telemetry_json: add test for string character escaping

[Bruce Richardson]

Add unit test to validate that when creating a string response in json,
that characters such as \n or quotes are properly escaped.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 748b7cfe5a..955c2e5b1b 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -125,6 +125,22 @@ test_large_obj_element(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_string_char_escaping(void)
+{
+	static const char str[] = "A string across\ntwo lines and \"with quotes\"!";
+	const char *expected = "\"A string across\\ntwo lines and \\\"with quotes\\\"!\"";
+	char buf[sizeof(str) + 10];
+	int used = 0;
+
+	used = rte_tel_json_str(buf, sizeof(buf), used, str);
+	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -138,6 +154,7 @@ test_telemetry_json(void)
 			test_overflow_obj,
 			test_large_array_element,
 			test_large_obj_element,
+			test_string_char_escaping,
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[RFC PATCH 5/6] telemetry: add escaping of strings in arrays

[Bruce Richardson]

When strings are added to an array variable, we need to properly escape
the invalid json characters in the strings.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 13df5d07e3..c4442a0bf0 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -52,17 +52,22 @@ static const char control_chars[0x20] = {
 
 /**
  * @internal
- * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
- * except that it does proper escaping as necessary.
+ * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
+ * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
+ * time constants not needing escaping.
  * Drops any invalid characters we don't support
  */
 static inline int
-__json_format_str(char *buf, const int len, const char *str)
+__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
 {
 	char tmp[len];
 	int tmpidx = 0;
 
-	tmp[tmpidx++] = '"';
+	while (*prefix != '\0' && tmpidx < len)
+		tmp[tmpidx++] = *prefix++;
+	if (tmpidx >= len)
+		return 0;
+
 	while (*str != '\0') {
 		if (*str < (int)RTE_DIM(control_chars)) {
 			int idx = *str;  /* compilers don't like char type as index */
@@ -75,7 +80,7 @@ __json_format_str(char *buf, const int len, const char *str)
 			tmp[tmpidx++] = *str;
 		} else
 			tmp[tmpidx++] = *str;
-		/* we always need space for closing quote and null character.
+		/* we always need space for (at minimum) closing quote and null character.
 		 * Ensuring at least two free characters also means we can always take an
 		 * escaped character like "\n" without overflowing
 		 */
@@ -83,7 +88,12 @@ __json_format_str(char *buf, const int len, const char *str)
 			return 0;
 		str++;
 	}
-	tmp[tmpidx++] = '"';
+
+	while (*suffix != '\0' && tmpidx < len)
+		tmp[tmpidx++] = *suffix++;
+	if (tmpidx >= len)
+		return 0;
+
 	tmp[tmpidx] = '\0';
 
 	strcpy(buf, tmp);
@@ -108,7 +118,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
 static inline int
 rte_tel_json_str(char *buf, const int len, const int used, const char *str)
 {
-	return used + __json_format_str(buf + used, len - used, str);
+	return used + __json_format_str(buf + used, len - used, "\"", str, "\"");
 }
 
 /* Appends a string into the JSON array in the provided buffer. */
@@ -118,9 +128,9 @@ rte_tel_json_add_array_string(char *buf, const int len, const int used,
 {
 	int ret, end = used - 1; /* strip off final delimiter */
 	if (used <= 2) /* assume empty, since minimum is '[]' */
-		return __json_snprintf(buf, len, "[\"%s\"]", str);
+		return __json_format_str(buf, len, "[\"", str, "\"]");
 
-	ret = __json_snprintf(buf + end, len - end, ",\"%s\"]", str);
+	ret = __json_format_str(buf + end, len - end, ",\"", str, "\"]");
 	return ret == 0 ? used : end + ret;
 }
 
-- 
2.34.1

[RFC PATCH 6/6] test/telemetry-json: add test case for escaping strings in arrays

[Bruce Richardson]

Add test-case to validate that when adding strings to arrays, the
strings are properly escaped to remove any invalid characters.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 955c2e5b1b..642ae9d6e1 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -141,6 +141,29 @@ test_string_char_escaping(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_array_char_escaping(void)
+{
+	/* "meaning of life", with tab between first two words, \n at end, and "life" in quotes,
+	 * followed by "all the fish" in quotes */
+	const char *expected = "[\"meaning\\tof \\\"life\\\"\\n\",\"\\\"all the fish\\\"\"]";
+	char buf[1024];
+	int used = 0;
+
+	printf("%s: ", __func__);
+	used = rte_tel_json_empty_array(buf, sizeof(buf), used);
+	if (used != 2 || strcmp(buf, "[]"))
+		return -1;
+
+	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, "meaning\tof \"life\"\n");
+	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, "\"all the fish\"");
+
+	printf("buf = '%s', expected = '%s'\n", buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -155,6 +178,7 @@ test_telemetry_json(void)
 			test_large_array_element,
 			test_large_obj_element,
 			test_string_char_escaping,
+			test_array_char_escaping,
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

RE: [RFC PATCH 0/6] add json string escaping to telemetry

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 23 June 2022 18.43
> 
> This RFC shows one possible approach for escaping strings for the json
> output of telemetry library. For now this RFC supports escaping strings
> for the cases of returning a single string, or returning an array of
> strings. Not done is escaping of strings in objs/dicts [see more below
> on TODO]

Very good initiative.

> 
> As well as telemetry lib changes, this patchset includes unit tests for
> the above and also little bit of cleanup to the json tests.
> 
> TODO:
> Beyond what is here in this RFC:
> 
> 1. we need to decide what to do about name/value pairs. Personally, I
>    think we should add the restriction to the "rte_tel_data_add_obj_*"
> APIs
>    to only allow a defined subset of characters in names: e.g.
> alphanumeric
>    chars, underscore and dash. That means that we only need to escape
>    the data part in the case of string returns.

I agree about only allowing a subset of characters in names, so JSON (and other) encoding is not required.

However, I think we should be less restrictive, and also allow characters commonly used for separation, indexing and wildcard, such as '/', '[', ']', and '*', '?' or '%'.

Obviously, we should disallow characters requiring escaping in not just JSON, but also other foreseeable encodings and protocols. So please bring your crystal ball to the discussion. ;-)

> 2. once agreed, need to implement a patch to escape strings in
>    dicts/objs

Yes.

> 
> 3. need to add a patch to escape the input command if it contains
>    invalid chars

What do you mean here? You mean unescape JSON encoded input (arriving on the JSON telemetry socket) to a proper binary string?

> 4. some small refactoring of the main telemetry.c json-encoding
> function
>    may be possible.

Perhaps.

Re: [RFC PATCH 0/6] add json string escaping to telemetry

[Bruce Richardson]

On Thu, Jun 23, 2022 at 09:04:31PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> > 
> > This RFC shows one possible approach for escaping strings for the json
> > output of telemetry library. For now this RFC supports escaping strings
> > for the cases of returning a single string, or returning an array of
> > strings. Not done is escaping of strings in objs/dicts [see more below
> > on TODO]
> 
> Very good initiative.
> 
> > 
> > As well as telemetry lib changes, this patchset includes unit tests for
> > the above and also little bit of cleanup to the json tests.
> > 
> > TODO:
> > Beyond what is here in this RFC:
> > 
> > 1. we need to decide what to do about name/value pairs. Personally, I
> >    think we should add the restriction to the "rte_tel_data_add_obj_*"
> > APIs
> >    to only allow a defined subset of characters in names: e.g.
> > alphanumeric
> >    chars, underscore and dash. That means that we only need to escape
> >    the data part in the case of string returns.
> 
> I agree about only allowing a subset of characters in names, so JSON (and other) encoding is not required.
> 
> However, I think we should be less restrictive, and also allow characters commonly used for separation, indexing and wildcard, such as '/', '[', ']', and '*', '?' or '%'.
> 
> Obviously, we should disallow characters requiring escaping in not just JSON, but also other foreseeable encodings and protocols. So please bring your crystal ball to the discussion. ;-)
> 
Exactly why I am looking for feedback - and why I'm looking to have an
explicit allowed list of characters rather than trying to just block the
known-bad in json ones.

For your suggestions: +1 to separators and indexing, i.e. '[', ']' and '/',
though I would probably also add ',' and maybe '.' (unless it's likely to
cause issues with some protocol we are likely to want to use).
For the wildcarding, I find it hard to see why we would want those?

The other advantage of using an allowlist of characters is that it makes it
possible to expand over time, compared to a blocklist which always runs the
risk of breaking something if you expand it. Therefore I suggest we keep
the list as small as we need right now, and expand it only as we need.

> > 2. once agreed, need to implement a patch to escape strings in
> >    dicts/objs
> 
> Yes.
> 
> > 
> > 3. need to add a patch to escape the input command if it contains
> >    invalid chars
> 
> What do you mean here? You mean unescape JSON encoded input (arriving on the JSON telemetry socket) to a proper binary string?
> 

The thing with the telemetry socket interface right now is that the input
requests are not-json. The reasons for that is that they be kept as simple
as possible, and to avoid needing a full json parser inside DPDK.
Therefore, the input sent by the user could contain invalid characters for
json output so we need to:
1. Guarantee that no command registered with the telemetry library contains
   invalid json characters (though why someone would do so, I don't know!)
2. When we return the command back in the reply, properly escape any
   invalid characters in the error case.

#1 is very important for sanity checking, but now that I think about it #2
is probably optional, since if any user does start sending invalid garbage
input that breaks their json parser on return, they are only hurting
themselves and not affecting anything else on the system.

> > 4. some small refactoring of the main telemetry.c json-encoding
> > function may be possible.
> 
> Perhaps.
> 
I saw some options for cleanup when I was working on the code, so including
this as a note-to-self as much as anything else for feedback. :-)

/Bruce

RE: [RFC PATCH 0/6] add json string escaping to telemetry

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 24 June 2022 10.14
> 
> On Thu, Jun 23, 2022 at 09:04:31PM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Thursday, 23 June 2022 18.43
> > >
> > > This RFC shows one possible approach for escaping strings for the
> json
> > > output of telemetry library. For now this RFC supports escaping
> strings
> > > for the cases of returning a single string, or returning an array
> of
> > > strings. Not done is escaping of strings in objs/dicts [see more
> below
> > > on TODO]
> >
> > Very good initiative.
> >
> > >
> > > As well as telemetry lib changes, this patchset includes unit tests
> for
> > > the above and also little bit of cleanup to the json tests.
> > >
> > > TODO:
> > > Beyond what is here in this RFC:
> > >
> > > 1. we need to decide what to do about name/value pairs. Personally,
> I
> > >    think we should add the restriction to the
> "rte_tel_data_add_obj_*"
> > > APIs
> > >    to only allow a defined subset of characters in names: e.g.
> > > alphanumeric
> > >    chars, underscore and dash. That means that we only need to
> escape
> > >    the data part in the case of string returns.
> >
> > I agree about only allowing a subset of characters in names, so JSON
> (and other) encoding is not required.
> >
> > However, I think we should be less restrictive, and also allow
> characters commonly used for separation, indexing and wildcard, such as
> '/', '[', ']', and '*', '?' or '%'.
> >
> > Obviously, we should disallow characters requiring escaping in not
> just JSON, but also other foreseeable encodings and protocols. So
> please bring your crystal ball to the discussion. ;-)
> >
> Exactly why I am looking for feedback - and why I'm looking to have an
> explicit allowed list of characters rather than trying to just block
> the
> known-bad in json ones.
> 
> For your suggestions: +1 to separators and indexing, i.e. '[', ']' and
> '/',
> though I would probably also add ',' and maybe '.' (unless it's likely
> to
> cause issues with some protocol we are likely to want to use).

After having slept on it, I think we should also allow characters that could appear in IP and MAC addresses, i.e. '.' and ':' (and '/' for subnetting).

> For the wildcarding, I find it hard to see why we would want those?

Initially, I thought a wildcard might be useful as a placeholder in templates.

But it might also be useful for partial IP or MAC addresses. E.g.:
- The SmartShare Systems OUI could be represented by the MAC address "00:1F:B4:??:??:??".
- A default gateway address in a template configuration could be "192.168.*.1".

On the other hand, wildcard characters could be disallowed or require escaping in other (non-JSON) protocols.

So I'm just being a bit creative here, throwing out ideas in our search for the right balance in the restrictions.

> 
> The other advantage of using an allowlist of characters is that it
> makes it
> possible to expand over time, compared to a blocklist which always runs
> the
> risk of breaking something if you expand it. Therefore I suggest we
> keep
> the list as small as we need right now, and expand it only as we need.

+1

> 
> > > 2. once agreed, need to implement a patch to escape strings in
> > >    dicts/objs
> >
> > Yes.
> >
> > >
> > > 3. need to add a patch to escape the input command if it contains
> > >    invalid chars
> >
> > What do you mean here? You mean unescape JSON encoded input (arriving
> on the JSON telemetry socket) to a proper binary string?
> >
> 
> The thing with the telemetry socket interface right now is that the
> input
> requests are not-json. The reasons for that is that they be kept as
> simple
> as possible, and to avoid needing a full json parser inside DPDK.
> Therefore, the input sent by the user could contain invalid characters
> for
> json output so we need to:
> 1. Guarantee that no command registered with the telemetry library
> contains
>    invalid json characters (though why someone would do so, I don't
> know!)
> 2. When we return the command back in the reply, properly escape any
>    invalid characters in the error case.
> 
> #1 is very important for sanity checking, but now that I think about it
> #2
> is probably optional, since if any user does start sending invalid
> garbage
> input that breaks their json parser on return, they are only hurting
> themselves and not affecting anything else on the system.
> 
> > > 4. some small refactoring of the main telemetry.c json-encoding
> > > function may be possible.
> >
> > Perhaps.
> >
> I saw some options for cleanup when I was working on the code, so
> including
> this as a note-to-self as much as anything else for feedback. :-)
> 
> /Bruce

Re: [RFC PATCH 0/6] add json string escaping to telemetry

[Bruce Richardson]

On Fri, Jun 24, 2022 at 11:12:05AM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Friday, 24 June 2022 10.14
> > 
> > On Thu, Jun 23, 2022 at 09:04:31PM +0200, Morten Brørup wrote:
> > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > Sent: Thursday, 23 June 2022 18.43
> > > >
> > > > This RFC shows one possible approach for escaping strings for the
> > json
> > > > output of telemetry library. For now this RFC supports escaping
> > strings
> > > > for the cases of returning a single string, or returning an array
> > of
> > > > strings. Not done is escaping of strings in objs/dicts [see more
> > below
> > > > on TODO]
> > >
> > > Very good initiative.
> > >
> > > >
> > > > As well as telemetry lib changes, this patchset includes unit tests
> > for
> > > > the above and also little bit of cleanup to the json tests.
> > > >
> > > > TODO:
> > > > Beyond what is here in this RFC:
> > > >
> > > > 1. we need to decide what to do about name/value pairs. Personally,
> > I
> > > >    think we should add the restriction to the
> > "rte_tel_data_add_obj_*"
> > > > APIs
> > > >    to only allow a defined subset of characters in names: e.g.
> > > > alphanumeric
> > > >    chars, underscore and dash. That means that we only need to
> > escape
> > > >    the data part in the case of string returns.
> > >
> > > I agree about only allowing a subset of characters in names, so JSON
> > (and other) encoding is not required.
> > >
> > > However, I think we should be less restrictive, and also allow
> > characters commonly used for separation, indexing and wildcard, such as
> > '/', '[', ']', and '*', '?' or '%'.
> > >
> > > Obviously, we should disallow characters requiring escaping in not
> > just JSON, but also other foreseeable encodings and protocols. So
> > please bring your crystal ball to the discussion. ;-)
> > >
> > Exactly why I am looking for feedback - and why I'm looking to have an
> > explicit allowed list of characters rather than trying to just block
> > the
> > known-bad in json ones.
> > 
> > For your suggestions: +1 to separators and indexing, i.e. '[', ']' and
> > '/',
> > though I would probably also add ',' and maybe '.' (unless it's likely
> > to
> > cause issues with some protocol we are likely to want to use).
> 
> After having slept on it, I think we should also allow characters that could appear in IP and MAC addresses, i.e. '.' and ':' (and '/' for subnetting).
> 
> > For the wildcarding, I find it hard to see why we would want those?
> 
> Initially, I thought a wildcard might be useful as a placeholder in templates.
> 
> But it might also be useful for partial IP or MAC addresses. E.g.:
> - The SmartShare Systems OUI could be represented by the MAC address "00:1F:B4:??:??:??".
> - A default gateway address in a template configuration could be "192.168.*.1".
> 
> On the other hand, wildcard characters could be disallowed or require escaping in other (non-JSON) protocols.
> 
> So I'm just being a bit creative here, throwing out ideas in our search for the right balance in the restrictions.
> 

I could see those characters certainly being needed in data values, but do
you foresee them being required in the names of fields?

> > 
> > The other advantage of using an allowlist of characters is that it
> > makes it
> > possible to expand over time, compared to a blocklist which always runs
> > the
> > risk of breaking something if you expand it. Therefore I suggest we
> > keep
> > the list as small as we need right now, and expand it only as we need.
> 
> +1
>

From previous on-list discussion, I take it that SNMP is a possible target
protocol you might have in mind. Any other protocols you can think of and
what restrictions (if any) would SNMP or those other protocols add?

/Bruce 

RE: [RFC PATCH 0/6] add json string escaping to telemetry

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Friday, 24 June 2022 11.17
> 
> On Fri, Jun 24, 2022 at 11:12:05AM +0200, Morten Brørup wrote:
> > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > Sent: Friday, 24 June 2022 10.14
> > >
> > > On Thu, Jun 23, 2022 at 09:04:31PM +0200, Morten Brørup wrote:
> > > > > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > > > > Sent: Thursday, 23 June 2022 18.43
> > > > >
> > > > > This RFC shows one possible approach for escaping strings for
> the
> > > json
> > > > > output of telemetry library. For now this RFC supports escaping
> > > strings
> > > > > for the cases of returning a single string, or returning an
> array
> > > of
> > > > > strings. Not done is escaping of strings in objs/dicts [see
> more
> > > below
> > > > > on TODO]
> > > >
> > > > Very good initiative.
> > > >
> > > > >
> > > > > As well as telemetry lib changes, this patchset includes unit
> tests
> > > for
> > > > > the above and also little bit of cleanup to the json tests.
> > > > >
> > > > > TODO:
> > > > > Beyond what is here in this RFC:
> > > > >
> > > > > 1. we need to decide what to do about name/value pairs.
> Personally,
> > > I
> > > > >    think we should add the restriction to the
> > > "rte_tel_data_add_obj_*"
> > > > > APIs
> > > > >    to only allow a defined subset of characters in names: e.g.
> > > > > alphanumeric
> > > > >    chars, underscore and dash. That means that we only need to
> > > escape
> > > > >    the data part in the case of string returns.
> > > >
> > > > I agree about only allowing a subset of characters in names, so
> JSON
> > > (and other) encoding is not required.
> > > >
> > > > However, I think we should be less restrictive, and also allow
> > > characters commonly used for separation, indexing and wildcard,
> such as
> > > '/', '[', ']', and '*', '?' or '%'.
> > > >
> > > > Obviously, we should disallow characters requiring escaping in
> not
> > > just JSON, but also other foreseeable encodings and protocols. So
> > > please bring your crystal ball to the discussion. ;-)
> > > >
> > > Exactly why I am looking for feedback - and why I'm looking to have
> an
> > > explicit allowed list of characters rather than trying to just
> block
> > > the
> > > known-bad in json ones.
> > >
> > > For your suggestions: +1 to separators and indexing, i.e. '[', ']'
> and
> > > '/',
> > > though I would probably also add ',' and maybe '.' (unless it's
> likely
> > > to
> > > cause issues with some protocol we are likely to want to use).
> >
> > After having slept on it, I think we should also allow characters
> that could appear in IP and MAC addresses, i.e. '.' and ':' (and '/'
> for subnetting).
> >
> > > For the wildcarding, I find it hard to see why we would want those?
> >
> > Initially, I thought a wildcard might be useful as a placeholder in
> templates.
> >
> > But it might also be useful for partial IP or MAC addresses. E.g.:
> > - The SmartShare Systems OUI could be represented by the MAC address
> "00:1F:B4:??:??:??".
> > - A default gateway address in a template configuration could be
> "192.168.*.1".
> >
> > On the other hand, wildcard characters could be disallowed or require
> escaping in other (non-JSON) protocols.
> >
> > So I'm just being a bit creative here, throwing out ideas in our
> search for the right balance in the restrictions.
> >
> 
> I could see those characters certainly being needed in data values, but
> do
> you foresee them being required in the names of fields?

We don't use the Telemetry library, because we have our own libraries for similar and related purposes. So I'm mostly speculating, trying to transform our experience into how I would expect the Telemetry library to work, while also trying to look farther into the future.

Answering your question:

Yes, if you consider the names as keys in a key/value store, there might be single entries that look like a template. Although the names of such entries might as well be "00:1F:B4:xx:xx:xx" or "192.168.z.1", using 'x' and 'z' as the wildcard characters.

Perhaps we should start with the low risk choice, and not allow the special wild card characters, such as '*', '?', '%', since 'x' is just as good in those cases.

> 
> > >
> > > The other advantage of using an allowlist of characters is that it
> > > makes it
> > > possible to expand over time, compared to a blocklist which always
> runs
> > > the
> > > risk of breaking something if you expand it. Therefore I suggest we
> > > keep
> > > the list as small as we need right now, and expand it only as we
> need.
> >
> > +1
> >
> 
> From previous on-list discussion, I take it that SNMP is a possible
> target
> protocol you might have in mind. Any other protocols you can think of
> and
> what restrictions (if any) would SNMP or those other protocols add?

JSON and UTF-8 seems to have taken over the world entirely.

SNMP support is usually required for legacy reasons. The SNMP lookup key is always an OID (Object Identifier), which basically is a sequence of numbers with a well known length of the sequence. In theory, any BLOB could be converted to an OID. With that in mind, I don't think SNMP puts any restrictions to the character set of the Telemetry names. The translation between OID format (i.e. a sequence of numbers) and Telemetry name format (i.e. a string) could be a very simple encoder/decoder, since there are no special characters requiring special treatment.

Going back to the IP address topic above, some of the SNMP MIBs use the IP address as the last four numbers in the OID, e.g. "ipAdEntIfIndex.192.0.1.1" (where ipAdEntIfIndex is short for "1.3.6.1.2.1.4.20.1.2"). My point here is: The names available for lookup in the telemetry database could be highly dynamic. 

As for other protocols, there could be something like InfluxDB [1], for direct streaming of statistics and other telemetry, but I don't have real experience with any of them. Our customers currently use scripts to poll the JSON data from our API and push them into their InfluxDB databases.

There could also be limitations in the structured format for SYSLOG [2], but again I don't have any experience with it. We just use classic SYSLOG text messages.

[1] https://docs.influxdata.com/influxdb/cloud/reference/syntax/line-protocol/
[2] https://datatracker.ietf.org/doc/html/rfc5424

> 
> /Bruce

RE: [RFC PATCH 0/6] add json string escaping to telemetry

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Thursday, 23 June 2022 18.43
> 
> This RFC shows one possible approach for escaping strings for the json
> output of telemetry library. For now this RFC supports escaping strings
> for the cases of returning a single string, or returning an array of
> strings. Not done is escaping of strings in objs/dicts [see more below
> on TODO]

Bugzilla ID: 1037

-Morten

Re: [RFC PATCH 0/6] add json string escaping to telemetry

[Bruce Richardson]

On Thu, Jul 14, 2022 at 05:42:59PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Thursday, 23 June 2022 18.43
> > 
> > This RFC shows one possible approach for escaping strings for the json
> > output of telemetry library. For now this RFC supports escaping strings
> > for the cases of returning a single string, or returning an array of
> > strings. Not done is escaping of strings in objs/dicts [see more below
> > on TODO]
> 
> Bugzilla ID: 1037
> 
Noted in the v2 patchset now sent to the list.

Thanks for all the feedback on the RFC. Hopefully I've managed to take all
- or at least most of it - correctly into account on the v2.

/Bruce

[PATCH v2 00/13] telemetry JSON escaping and other enhancements

[Bruce Richardson]

This patchset contains fixes for the problem of handling characters
returned by telemetry callbacks which require escaping when encoded in
JSON format. It also includes unit tests to validate the correct
encoding in such scenarios and a number of smaller enhancements to
telemetry and telemetry testing.

RFC->V2:
* limited characters allowed in dictionary element names and command
  names to side-step the encoding problems there.
* added support for proper escaping of dictionary string values
* added more testing and test cases
* added other misc telemetry cleanups and refactoring

Bruce Richardson (13):
  test/telemetry_json: print success or failure per subtest
  telemetry: fix escaping of invalid json characters
  test/telemetry_json: add test for string character escaping
  telemetry: add escaping of strings in arrays
  test/telemetry-json: add test for escaping strings in arrays
  telemetry: limit characters allowed in dictionary names
  telemetry: add escaping of strings in dicts
  test/telemetry_json: add test for string escaping in objects
  telemetry: limit command characters
  test/telemetry_data: refactor for maintainability
  test/telemetry_data: add test cases for character escaping
  telemetry: eliminate duplicate code for json output
  telemetry: make help command more helpful

 app/test/test_telemetry_data.c       | 138 +++++++++++++++++++--------
 app/test/test_telemetry_json.c       |  98 +++++++++++++++++--
 doc/guides/rel_notes/deprecation.rst |   8 --
 lib/telemetry/rte_telemetry.h        |   8 ++
 lib/telemetry/telemetry.c            |  51 +++++-----
 lib/telemetry/telemetry_data.c       |  32 +++++++
 lib/telemetry/telemetry_json.h       |  72 ++++++++++++--
 7 files changed, 318 insertions(+), 89 deletions(-)

--
2.34.1

[PATCH v2 01/13] test/telemetry_json: print success or failure per subtest

[Bruce Richardson]

rather than just printing out success or failure at the end of the test
only, print out "OK" or "ERROR" for each individual test case within the
overall test. As part of this, ensure each case returns 0 on success and
any other value on failure.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 790181d316..748b7cfe5a 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -102,8 +102,10 @@ test_large_array_element(void)
 
 	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, str);
 	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != 0)
+		return -1;
 
-	return strlen(buf) != 0;
+	return strncmp(expected, buf, sizeof(buf));
 }
 
 static int
@@ -117,20 +119,33 @@ test_large_obj_element(void)
 
 	used = rte_tel_json_add_obj_u64(buf, sizeof(buf), used, str, 0);
 	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != 0)
+		return -1;
 
-	return strlen(buf) != 0;
+	return strncmp(expected, buf, sizeof(buf));
 }
 
+typedef int (*test_fn)(void);
+
 static int
 test_telemetry_json(void)
 {
-	if (test_basic_array() < 0 ||
-			test_basic_obj() < 0 ||
-			test_overflow_array() < 0 ||
-			test_overflow_obj() < 0 ||
-			test_large_array_element() < 0 ||
-			test_large_obj_element() < 0)
-		return -1;
+	unsigned int i;
+	test_fn fns[] = {
+			test_basic_array,
+			test_basic_obj,
+			test_overflow_array,
+			test_overflow_obj,
+			test_large_array_element,
+			test_large_obj_element,
+	};
+	for (i = 0; i < RTE_DIM(fns); i++)
+		if (fns[i]() == 0)
+			printf("OK\n");
+		else {
+			printf("ERROR\n");
+			return -1;
+		}
 	return 0;
 }
 
-- 
2.34.1

[PATCH v2 02/13] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

For string values returned from telemetry, escape any values that cannot
normally appear in a json string. According to the json spec[1], the
characters than need to be handled are control chars (char value < 0x20)
and '"' and '\' characters.

To handle this, we replace the snprintf call with a separate string
copying and encapsulation routine which checks each character as it
copies it to the final array.

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

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Bugzilla ID: 1037

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry.c      | 11 +++++---
 lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index c6fd03a5ab..7188b1905c 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -232,9 +232,14 @@ 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);
+		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
+				MAX_CMD_LEN, cmd);
+		cb_data_buf = &out_buf[prefix_used];
+		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
+
+		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
+		used += prefix_used;
+		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
 	case RTE_TEL_DICT:
 		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index db70690274..13df5d07e3 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char *format, ...)
 	return 0; /* nothing written or modified */
 }
 
+static const char control_chars[0x20] = {
+		['\n'] = 'n',
+		['\r'] = 'r',
+		['\t'] = 't',
+};
+
+/**
+ * @internal
+ * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
+ * except that it does proper escaping as necessary.
+ * Drops any invalid characters we don't support
+ */
+static inline int
+__json_format_str(char *buf, const int len, const char *str)
+{
+	char tmp[len];
+	int tmpidx = 0;
+
+	tmp[tmpidx++] = '"';
+	while (*str != '\0') {
+		if (*str < (int)RTE_DIM(control_chars)) {
+			int idx = *str;  /* compilers don't like char type as index */
+			if (control_chars[idx] != 0) {
+				tmp[tmpidx++] = '\\';
+				tmp[tmpidx++] = control_chars[idx];
+			}
+		} else if (*str == '"' || *str == '\\') {
+			tmp[tmpidx++] = '\\';
+			tmp[tmpidx++] = *str;
+		} else
+			tmp[tmpidx++] = *str;
+		/* we always need space for closing quote and null character.
+		 * Ensuring at least two free characters also means we can always take an
+		 * escaped character like "\n" without overflowing
+		 */
+		if (tmpidx > len - 2)
+			return 0;
+		str++;
+	}
+	tmp[tmpidx++] = '"';
+	tmp[tmpidx] = '\0';
+
+	strcpy(buf, tmp);
+	return tmpidx;
+}
+
 /* Copies an empty array into the provided buffer. */
 static inline int
 rte_tel_json_empty_array(char *buf, const int len, const int used)
@@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
 static inline int
 rte_tel_json_str(char *buf, const int len, const int used, const char *str)
 {
-	return used + __json_snprintf(buf + used, len - used, "\"%s\"", str);
+	return used + __json_format_str(buf + used, len - used, str);
 }
 
 /* Appends a string into the JSON array in the provided buffer. */
-- 
2.34.1

RE: [PATCH v2 02/13] telemetry: fix escaping of invalid json characters

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 25 July 2022 18.36
> To: dev@dpdk.org
> Cc: Bruce Richardson; Ciara Power; Keith Wiles
> Subject: [PATCH v2 02/13] telemetry: fix escaping of invalid json
> characters
> 
> For string values returned from telemetry, escape any values that
> cannot
> normally appear in a json string. According to the json spec[1], the
> characters than need to be handled are control chars (char value <
> 0x20)
> and '"' and '\' characters.
> 
> To handle this, we replace the snprintf call with a separate string
> copying and encapsulation routine which checks each character as it
> copies it to the final array.
> 
> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> 
> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> Bugzilla ID: 1037
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---

Patchwork didn't pick up my reply to the 00/13 of the series, so I'll try again here...

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

Re: [PATCH v2 02/13] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

On Tue, Jul 26, 2022 at 08:25:05PM +0200, Morten Brørup wrote:
> > From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> > Sent: Monday, 25 July 2022 18.36
> > To: dev@dpdk.org
> > Cc: Bruce Richardson; Ciara Power; Keith Wiles
> > Subject: [PATCH v2 02/13] telemetry: fix escaping of invalid json
> > characters
> > 
> > For string values returned from telemetry, escape any values that
> > cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value <
> > 0x20)
> > and '"' and '\' characters.
> > 
> > To handle this, we replace the snprintf call with a separate string
> > copying and encapsulation routine which checks each character as it
> > copies it to the final array.
> > 
> > [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > 
> > Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> > Bugzilla ID: 1037
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> 
> Patchwork didn't pick up my reply to the 00/13 of the series, so I'll try again here...
> 
> Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>
> 
Unfortunately, patchwork doesn't work with series acks, only individual
ones. However, the maintainers recognise when they are present, and if I do
a V3, I'll split your ack across the set so patchwork does recognise it.

/Bruce

Re: [PATCH v2 02/13] telemetry: fix escaping of invalid json characters

[fengchengwen]

On 2022/7/26 0:35, Bruce Richardson wrote:
> For string values returned from telemetry, escape any values that cannot
> normally appear in a json string. According to the json spec[1], the
> characters than need to be handled are control chars (char value < 0x20)
> and '"' and '\' characters.
> 
> To handle this, we replace the snprintf call with a separate string
> copying and encapsulation routine which checks each character as it
> copies it to the final array.
> 
> [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> 
> Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> Bugzilla ID: 1037
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/telemetry/telemetry.c      | 11 +++++---
>  lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
>  2 files changed, 55 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index c6fd03a5ab..7188b1905c 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -232,9 +232,14 @@ 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);
> +		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> +				MAX_CMD_LEN, cmd);

The cmd need also escaped.
But I notice the [PATCH v2 06/13] limit it. Suggest move 06 at the head of patchset.

> +		cb_data_buf = &out_buf[prefix_used];
> +		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
> +
> +		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
> +		used += prefix_used;
> +		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
>  		break;
>  	case RTE_TEL_DICT:
>  		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> index db70690274..13df5d07e3 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char *format, ...)
>  	return 0; /* nothing written or modified */
>  }
>  
> +static const char control_chars[0x20] = {
> +		['\n'] = 'n',
> +		['\r'] = 'r',
> +		['\t'] = 't',
> +};
> +
> +/**
> + * @internal
> + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> + * except that it does proper escaping as necessary.
> + * Drops any invalid characters we don't support
> + */
> +static inline int
> +__json_format_str(char *buf, const int len, const char *str)
> +{
> +	char tmp[len];

Could reuse buf otherthan tmp

> +	int tmpidx = 0;
> +
> +	tmp[tmpidx++] = '"';
> +	while (*str != '\0') {
> +		if (*str < (int)RTE_DIM(control_chars)) {
> +			int idx = *str;  /* compilers don't like char type as index */
> +			if (control_chars[idx] != 0) {
> +				tmp[tmpidx++] = '\\';
> +				tmp[tmpidx++] = control_chars[idx];

Why not espace all control chars?

> +			}
> +		} else if (*str == '"' || *str == '\\') {
> +			tmp[tmpidx++] = '\\';
> +			tmp[tmpidx++] = *str;
> +		} else
> +			tmp[tmpidx++] = *str;
> +		/* we always need space for closing quote and null character.
> +		 * Ensuring at least two free characters also means we can always take an
> +		 * escaped character like "\n" without overflowing
> +		 */
> +		if (tmpidx > len - 2)
> +			return 0;

Suggest add log here to help find out problem.

> +		str++;
> +	}
> +	tmp[tmpidx++] = '"';
> +	tmp[tmpidx] = '\0';
> +
> +	strcpy(buf, tmp);
> +	return tmpidx;
> +}
> +
>  /* Copies an empty array into the provided buffer. */
>  static inline int
>  rte_tel_json_empty_array(char *buf, const int len, const int used)
> @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
>  static inline int
>  rte_tel_json_str(char *buf, const int len, const int used, const char *str)
>  {
> -	return used + __json_snprintf(buf + used, len - used, "\"%s\"", str);
> +	return used + __json_format_str(buf + used, len - used, str);
>  }
>  
>  /* Appends a string into the JSON array in the provided buffer. */
> 

Re: [PATCH v2 02/13] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

On Wed, Jul 27, 2022 at 09:13:18AM +0800, fengchengwen wrote:
> On 2022/7/26 0:35, Bruce Richardson wrote:
> > For string values returned from telemetry, escape any values that cannot
> > normally appear in a json string. According to the json spec[1], the
> > characters than need to be handled are control chars (char value < 0x20)
> > and '"' and '\' characters.
> > 
> > To handle this, we replace the snprintf call with a separate string
> > copying and encapsulation routine which checks each character as it
> > copies it to the final array.
> > 
> > [1] https://www.rfc-editor.org/rfc/rfc8259.txt
> > 
> > Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
> > Bugzilla ID: 1037
> > 
> > Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> > ---
> >  lib/telemetry/telemetry.c      | 11 +++++---
> >  lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
> >  2 files changed, 55 insertions(+), 4 deletions(-)
> > 
> > diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> > index c6fd03a5ab..7188b1905c 100644
> > --- a/lib/telemetry/telemetry.c
> > +++ b/lib/telemetry/telemetry.c
> > @@ -232,9 +232,14 @@ 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);
> > +		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> > +				MAX_CMD_LEN, cmd);
> 
> The cmd need also escaped.
> But I notice the [PATCH v2 06/13] limit it. Suggest move 06 at the head of patchset.
>
Right. I'll try some patch reordering in the next version of this set.
 
> > +		cb_data_buf = &out_buf[prefix_used];
> > +		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
> > +
> > +		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
> > +		used += prefix_used;
> > +		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
> >  		break;
> >  	case RTE_TEL_DICT:
> >  		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
> > diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> > index db70690274..13df5d07e3 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char *format, ...)
> >  	return 0; /* nothing written or modified */
> >  }
> >  
> > +static const char control_chars[0x20] = {
> > +		['\n'] = 'n',
> > +		['\r'] = 'r',
> > +		['\t'] = 't',
> > +};
> > +
> > +/**
> > + * @internal
> > + * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
> > + * except that it does proper escaping as necessary.
> > + * Drops any invalid characters we don't support
> > + */
> > +static inline int
> > +__json_format_str(char *buf, const int len, const char *str)
> > +{
> > +	char tmp[len];
> 
> Could reuse buf otherthan tmp
> 
The approach here is to guarantee that we always output valid json.
Therefore, we build up the output in a temporary buffer until we are sure
that it's all correct and can fit, before moving it into the final buffer.
That way, if there are any issues, the original buffer is unmodified, and
we can return the bytes-appended as 0.

> > +	int tmpidx = 0;
> > +
> > +	tmp[tmpidx++] = '"';
> > +	while (*str != '\0') {
> > +		if (*str < (int)RTE_DIM(control_chars)) {
> > +			int idx = *str;  /* compilers don't like char type as index */
> > +			if (control_chars[idx] != 0) {
> > +				tmp[tmpidx++] = '\\';
> > +				tmp[tmpidx++] = control_chars[idx];
> 
> Why not espace all control chars?
> 
Because only certain characters have valid escape codes, and any other
characters would have to be replaced with unicode values. These should not
be ever appearing in our text output fields anyway.

> > +			}
> > +		} else if (*str == '"' || *str == '\\') {
> > +			tmp[tmpidx++] = '\\';
> > +			tmp[tmpidx++] = *str;
> > +		} else
> > +			tmp[tmpidx++] = *str;
> > +		/* we always need space for closing quote and null character.
> > +		 * Ensuring at least two free characters also means we can always take an
> > +		 * escaped character like "\n" without overflowing
> > +		 */
> > +		if (tmpidx > len - 2)
> > +			return 0;
> 
> Suggest add log here to help find out problem.
> 
Telemetry is operating in a background thread, so not sure logging is a
good idea in such cases. I'd look for other opinions on this...

> > +		str++;
> > +	}
> > +	tmp[tmpidx++] = '"';
> > +	tmp[tmpidx] = '\0';
> > +
> > +	strcpy(buf, tmp);
> > +	return tmpidx;
> > +}
> > +
> >  /* Copies an empty array into the provided buffer. */
> >  static inline int
> >  rte_tel_json_empty_array(char *buf, const int len, const int used)
> > @@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
> >  static inline int
> >  rte_tel_json_str(char *buf, const int len, const int used, const char *str)
> >  {
> > -	return used + __json_snprintf(buf + used, len - used, "\"%s\"", str);
> > +	return used + __json_format_str(buf + used, len - used, str);
> >  }
> >  
> >  /* Appends a string into the JSON array in the provided buffer. */
> > 
> 

[PATCH v2 03/13] test/telemetry_json: add test for string character escaping

[Bruce Richardson]

Add unit test to validate that when creating a string response in json,
that characters such as \n or quotes are properly escaped.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 748b7cfe5a..955c2e5b1b 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -125,6 +125,22 @@ test_large_obj_element(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_string_char_escaping(void)
+{
+	static const char str[] = "A string across\ntwo lines and \"with quotes\"!";
+	const char *expected = "\"A string across\\ntwo lines and \\\"with quotes\\\"!\"";
+	char buf[sizeof(str) + 10];
+	int used = 0;
+
+	used = rte_tel_json_str(buf, sizeof(buf), used, str);
+	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -138,6 +154,7 @@ test_telemetry_json(void)
 			test_overflow_obj,
 			test_large_array_element,
 			test_large_obj_element,
+			test_string_char_escaping,
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[PATCH v2 04/13] telemetry: add escaping of strings in arrays

[Bruce Richardson]

When strings are added to an array variable, we need to properly escape
the invalid json characters in the strings.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 13df5d07e3..c4442a0bf0 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -52,17 +52,22 @@ static const char control_chars[0x20] = {
 
 /**
  * @internal
- * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
- * except that it does proper escaping as necessary.
+ * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
+ * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
+ * time constants not needing escaping.
  * Drops any invalid characters we don't support
  */
 static inline int
-__json_format_str(char *buf, const int len, const char *str)
+__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
 {
 	char tmp[len];
 	int tmpidx = 0;
 
-	tmp[tmpidx++] = '"';
+	while (*prefix != '\0' && tmpidx < len)
+		tmp[tmpidx++] = *prefix++;
+	if (tmpidx >= len)
+		return 0;
+
 	while (*str != '\0') {
 		if (*str < (int)RTE_DIM(control_chars)) {
 			int idx = *str;  /* compilers don't like char type as index */
@@ -75,7 +80,7 @@ __json_format_str(char *buf, const int len, const char *str)
 			tmp[tmpidx++] = *str;
 		} else
 			tmp[tmpidx++] = *str;
-		/* we always need space for closing quote and null character.
+		/* we always need space for (at minimum) closing quote and null character.
 		 * Ensuring at least two free characters also means we can always take an
 		 * escaped character like "\n" without overflowing
 		 */
@@ -83,7 +88,12 @@ __json_format_str(char *buf, const int len, const char *str)
 			return 0;
 		str++;
 	}
-	tmp[tmpidx++] = '"';
+
+	while (*suffix != '\0' && tmpidx < len)
+		tmp[tmpidx++] = *suffix++;
+	if (tmpidx >= len)
+		return 0;
+
 	tmp[tmpidx] = '\0';
 
 	strcpy(buf, tmp);
@@ -108,7 +118,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
 static inline int
 rte_tel_json_str(char *buf, const int len, const int used, const char *str)
 {
-	return used + __json_format_str(buf + used, len - used, str);
+	return used + __json_format_str(buf + used, len - used, "\"", str, "\"");
 }
 
 /* Appends a string into the JSON array in the provided buffer. */
@@ -118,9 +128,9 @@ rte_tel_json_add_array_string(char *buf, const int len, const int used,
 {
 	int ret, end = used - 1; /* strip off final delimiter */
 	if (used <= 2) /* assume empty, since minimum is '[]' */
-		return __json_snprintf(buf, len, "[\"%s\"]", str);
+		return __json_format_str(buf, len, "[\"", str, "\"]");
 
-	ret = __json_snprintf(buf + end, len - end, ",\"%s\"]", str);
+	ret = __json_format_str(buf + end, len - end, ",\"", str, "\"]");
 	return ret == 0 ? used : end + ret;
 }
 
-- 
2.34.1

[PATCH v2 05/13] test/telemetry-json: add test for escaping strings in arrays

[Bruce Richardson]

Add test-case to validate that when adding strings to arrays, the
strings are properly escaped to remove any invalid characters.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 955c2e5b1b..31a13ea1d7 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -141,6 +141,29 @@ test_string_char_escaping(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_array_char_escaping(void)
+{
+	/* "meaning of life", with tab between first two words, '\n' at end,
+	 * and "life" in quotes, followed by "all the fish" in quotes
+	 */
+	const char *expected = "[\"meaning\\tof \\\"life\\\"\\n\",\"\\\"all the fish\\\"\"]";
+	char buf[1024];
+	int used = 0;
+
+	used = rte_tel_json_empty_array(buf, sizeof(buf), used);
+	if (used != 2 || strcmp(buf, "[]"))
+		return -1;
+
+	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, "meaning\tof \"life\"\n");
+	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, "\"all the fish\"");
+
+	printf("buf = '%s', expected = '%s'\n", buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -155,6 +178,7 @@ test_telemetry_json(void)
 			test_large_array_element,
 			test_large_obj_element,
 			test_string_char_escaping,
+			test_array_char_escaping,
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[PATCH v2 06/13] telemetry: limit characters allowed in dictionary names

[Bruce Richardson]

To save issues with encoding the names of values in dicts, we limit the
allowed names to a subset of character values. This list of allowed
characters can be expanded as necessary in future.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/rte_telemetry.h  |  8 ++++++++
 lib/telemetry/telemetry_data.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index d586dd0fc1..a0d21d6b7f 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -64,6 +64,10 @@ rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type);
 /**
  * Start a dictionary of values for returning from a callback
  *
+ * Dictionaries consist of key-values pairs to be returned, where the keys,
+ * or names, are strings and the values can be any of the types supported by telemetry.
+ * Name strings may only contain alphanumeric characters as well as '_' or '/'
+ *
  * @param d
  *   The data structure passed to the callback
  * @return
@@ -159,6 +163,7 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The string to be stored in the dict
  * @return
@@ -177,6 +182,7 @@ rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The number to be stored in the dict
  * @return
@@ -193,6 +199,7 @@ rte_tel_data_add_dict_int(struct rte_tel_data *d, const char *name, int val);
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The number to be stored in the dict
  * @return
@@ -212,6 +219,7 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict.
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The pointer to the container to be stored in the dict.
  * @param keep
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index e14ae3c4d4..b5cd74b25b 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -3,6 +3,8 @@
  */
 
 #undef RTE_USE_LIBBSD
+#include <stdbool.h>
+
 #include <rte_string_fns.h>
 
 #include "telemetry_data.h"
@@ -92,6 +94,24 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
 	return 0;
 }
 
+static bool
+valid_name(const char *name)
+{
+	char allowed[128] = {
+			['0' ... '9'] = 1,
+			['A' ... 'Z'] = 1,
+			['a' ... 'z'] = 1,
+			['_'] = 1,
+			['/'] = 1,
+	};
+	while (*name != '\0') {
+		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
+			return false;
+		name++;
+	}
+	return true;
+}
+
 int
 rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
 		const char *val)
@@ -104,6 +124,9 @@ rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_STRING_VAL;
 	vbytes = strlcpy(e->value.sval, val, RTE_TEL_MAX_STRING_LEN);
@@ -123,6 +146,9 @@ rte_tel_data_add_dict_int(struct rte_tel_data *d, const char *name, int val)
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_INT_VAL;
 	e->value.ival = val;
@@ -140,6 +166,9 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_U64_VAL;
 	e->value.u64val = val;
@@ -161,6 +190,9 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_CONTAINER;
 	e->value.container.data = val;
-- 
2.34.1

[PATCH v2 07/13] telemetry: add escaping of strings in dicts

[Bruce Richardson]

When strings are added to an dict variable, we need to properly escape
the invalid json characters in the strings.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry_json.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index c4442a0bf0..e3fae7c30d 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -54,7 +54,7 @@ static const char control_chars[0x20] = {
  * @internal
  * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
  * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
- * time constants not needing escaping.
+ * time constants, or values not needing escaping.
  * Drops any invalid characters we don't support
  */
 static inline int
@@ -219,12 +219,16 @@ static inline int
 rte_tel_json_add_obj_str(char *buf, const int len, const int used,
 		const char *name, const char *val)
 {
+	char tmp_name[RTE_TEL_MAX_STRING_LEN + 5];
 	int ret, end = used - 1;
+
+	/* names are limited to certain characters so need no escaping */
+	snprintf(tmp_name, sizeof(tmp_name), "{\"%s\":\"", name);
 	if (used <= 2) /* assume empty, since minimum is '{}' */
-		return __json_snprintf(buf, len, "{\"%s\":\"%s\"}", name, val);
+		return __json_format_str(buf, len, tmp_name, val, "\"}");
 
-	ret = __json_snprintf(buf + end, len - end, ",\"%s\":\"%s\"}",
-			name, val);
+	tmp_name[0] = ',';  /* replace '{' with ',' at start */
+	ret = __json_format_str(buf + end, len - end, tmp_name, val, "\"}");
 	return ret == 0 ? used : end + ret;
 }
 
-- 
2.34.1

[PATCH v2 08/13] test/telemetry_json: add test for string escaping in objects

[Bruce Richardson]

Add a test-case to validate that when adding strings either as the name
or the value of an entry in an object, that all values are escaped
properly.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_json.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 31a13ea1d7..184c3ba9f1 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -164,6 +164,29 @@ test_array_char_escaping(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_obj_char_escaping(void)
+{
+	const char *expected = "{\"good\":\"Clint Eastwood\\n\","
+			"\"bad\":\"Lee\\tVan\\tCleef\","
+			"\"ugly\":\"\\rEli Wallach\"}";
+	char buf[1024];
+	int used = 0;
+
+	used = rte_tel_json_empty_obj(buf, sizeof(buf), used);
+	if (used != 2 || strcmp(buf, "{}"))
+		return -1;
+
+	used = rte_tel_json_add_obj_str(buf, sizeof(buf), used, "good", "Clint Eastwood\n");
+	used = rte_tel_json_add_obj_str(buf, sizeof(buf), used, "bad", "Lee\tVan\tCleef");
+	used = rte_tel_json_add_obj_str(buf, sizeof(buf), used, "ugly", "\rEli Wallach");
+
+	printf("buf = '%s', expected = '%s'\n", buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -179,6 +202,7 @@ test_telemetry_json(void)
 			test_large_obj_element,
 			test_string_char_escaping,
 			test_array_char_escaping,
+			test_obj_char_escaping
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[PATCH v2 09/13] telemetry: limit command characters

[Bruce Richardson]

Limit the telemetry command characters to the minimum set needed for
current implementations. This prevents issues with invalid json
characters needing to be escaped on replies.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 doc/guides/rel_notes/deprecation.rst | 8 --------
 lib/telemetry/telemetry.c            | 7 +++++++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e7583cae4c..d1c93ca7e3 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -212,14 +212,6 @@ Deprecation Notices
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
-* telemetry: The allowed characters in names for dictionary values
-  will be limited to alphanumeric characters
-  and a small subset of additional printable characters.
-  This will ensure that all dictionary parameter names can be output
-  without escaping in JSON - or in any future output format used.
-  Names for the telemetry commands will be similarly limited.
-  The parameters for telemetry commands are unaffected by this change.
-
 * net/octeontx_ep: The driver ``octeontx_ep`` was to support OCTEON TX
   line of products.
   It will be renamed to ``octeon_ep`` in DPDK 22.11 to apply for
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 7188b1905c..03651e947d 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -70,12 +70,19 @@ int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 {
 	struct cmd_callback *new_callbacks;
+	const char *cmdp = cmd;
 	int i = 0;
 
 	if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
 			|| strlen(help) >= RTE_TEL_MAX_STRING_LEN)
 		return -EINVAL;
 
+	while (*cmdp != '\0') {
+		if (!isalnum(*cmdp) && *cmdp != '_' && *cmdp != '/')
+			return -EINVAL;
+		cmdp++;
+	}
+
 	rte_spinlock_lock(&callback_sl);
 	new_callbacks = realloc(callbacks, sizeof(callbacks[0]) * (num_callbacks + 1));
 	if (new_callbacks == NULL) {
-- 
2.34.1

[PATCH v2 10/13] test/telemetry_data: refactor for maintainability

[Bruce Richardson]

To help with the writing and maintaining of test cases in this file we
can make the following changes to it:

- rename non-test-case functions i.e. the infrastructure functions, to
  not start with "test_", so that each sub-test case can be identified
  by starting with that prefix.
- add a comment at the start of the file explaining how tests are to be
  written and managed, so as to keep consistency.
- add a trivial test-case for returning a simple string value to use as
  a reference example for those wanting to add test cases.
- improve the key macro used for validating the output from each
  function, so that the standard json preamble can be skipped for each
  function. This hides more of the infrastructure implementation from
  the user i.e. they don't need to worry what the actual command used is
  called, and also shortens the output strings so we can avoid line
  splitting in most cases.
- add clearing the "response_data" structure to the loop calling each
  test to avoid each test function having to do so individually.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_data.c | 101 ++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 41 deletions(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 73eee293a1..5a85e790d3 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -21,18 +21,45 @@
 #define TELEMETRY_VERSION "v2"
 #define REQUEST_CMD "/test"
 #define BUF_SIZE 1024
-#define TEST_OUTPUT(exp) test_output(__func__, exp)
+#define CHECK_OUTPUT(exp) check_output(__func__, "{\"" REQUEST_CMD "\":" exp "}")
+
+/*
+ * Runs a series of test cases, checking the output of telemetry for various different types of
+ * responses. On init, a single connection to DPDK telemetry is made, and a single telemetry
+ * callback "/test" is registered. That callback always returns the value of the static global
+ * variable "response_data", so each test case builds up that structure, and then calls the
+ * "check_output" function to ensure the response received over the socket for "/test" matches
+ * that expected for the response_data value populated.
+ *
+ * NOTE:
+ * - each test case function in this file should be added to the "test_cases" array in
+ *   test_telemetry_data function at the bottom of the file.
+ * - each test case function should populate the "response_data" global variable (below)
+ *   with the appropriate values which would be returned from a simulated telemetry function.
+ *   Then the test case function should have "return TEST_OUTPUT(<expected_data>);" as it's
+ *   last line. The test infrastructure will then validate that the output when returning
+ *   "response_data" structure matches that in "<expected_data>".
+ * - the response_data structure will be zeroed on entry to each test function, so each function
+ *   can begin with a call to "rte_tel_data_string/start_array/start_dict" as so desired.
+ * - the expected_output for each function can be just the actual json data from the
+ *   "response_data" value. The CHECK_OUTPUT macro will include the appropriate "{\"/test\": ... }"
+ *   structure around the json output.
+ *
+ *  See test_simple_string(), or test_case_array_int() for a basic examples of test cases.
+ */
+
 
 static struct rte_tel_data response_data;
 static int sock;
 
+
 /*
  * This function is the callback registered with Telemetry to be used when
  * the /test command is requested. This callback returns the global data built
  * up by the individual test cases.
  */
 static int
-test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
+telemetry_test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
 		struct rte_tel_data *d)
 {
 	*d = response_data;
@@ -46,7 +73,7 @@ test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
  * and is compared to the actual response received from Telemetry.
  */
 static int
-test_output(const char *func_name, const char *expected)
+check_output(const char *func_name, const char *expected)
 {
 	int bytes;
 	char buf[BUF_SIZE * 16];
@@ -66,6 +93,14 @@ test_output(const char *func_name, const char *expected)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_simple_string(void)
+{
+	rte_tel_data_string(&response_data, "Simple string");
+
+	return CHECK_OUTPUT("\"Simple string\"");
+}
+
 static int
 test_dict_with_array_int_values(void)
 {
@@ -77,7 +112,6 @@ test_dict_with_array_int_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
@@ -90,8 +124,7 @@ test_dict_with_array_int_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4],"
-			"\"dict_1\":[0,1,2,3,4]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[0,1,2,3,4],\"dict_1\":[0,1,2,3,4]}");
 }
 
 static int
@@ -105,7 +138,6 @@ test_array_with_array_int_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	for (i = 0; i < 5; i++) {
@@ -115,18 +147,18 @@ test_array_with_array_int_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
+	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
 static int
 test_case_array_int(void)
 {
 	int i;
-	memset(&response_data, 0, sizeof(response_data));
+
 	rte_tel_data_start_array(&response_data, RTE_TEL_INT_VAL);
 	for (i = 0; i < 5; i++)
 		rte_tel_data_add_array_int(&response_data, i);
-	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
+	return CHECK_OUTPUT("[0,1,2,3,4]");
 }
 
 static int
@@ -135,7 +167,6 @@ test_case_add_dict_int(void)
 	int i = 0;
 	char name_of_value[8];
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
@@ -143,14 +174,12 @@ test_case_add_dict_int(void)
 		rte_tel_data_add_dict_int(&response_data, name_of_value, i);
 	}
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,"
-			"\"dict_3\":3,\"dict_4\":4}}");
+	return CHECK_OUTPUT("{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}");
 }
 
 static int
 test_case_array_string(void)
 {
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_STRING_VAL);
 	rte_tel_data_add_array_string(&response_data, "aaaa");
 	rte_tel_data_add_array_string(&response_data, "bbbb");
@@ -158,14 +187,12 @@ test_case_array_string(void)
 	rte_tel_data_add_array_string(&response_data, "dddd");
 	rte_tel_data_add_array_string(&response_data, "eeee");
 
-	return TEST_OUTPUT("{\"/test\":[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\","
-			"\"eeee\"]}");
+	return CHECK_OUTPUT("[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\",\"eeee\"]");
 }
 
 static int
 test_case_add_dict_string(void)
 {
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_dict_string(&response_data, "dict_0", "aaaa");
@@ -173,8 +200,7 @@ test_case_add_dict_string(void)
 	rte_tel_data_add_dict_string(&response_data, "dict_2", "cccc");
 	rte_tel_data_add_dict_string(&response_data, "dict_3", "dddd");
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":\"aaaa\",\"dict_1\":"
-			"\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}}");
+	return CHECK_OUTPUT("{\"dict_0\":\"aaaa\",\"dict_1\":\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}");
 }
 
 
@@ -187,7 +213,6 @@ test_dict_with_array_string_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -198,8 +223,7 @@ test_dict_with_array_string_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[\"aaaa\"],\"dict_1\":"
-			"[\"bbbb\"]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}");
 }
 
 static int
@@ -214,7 +238,6 @@ test_dict_with_dict_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -226,8 +249,7 @@ test_dict_with_dict_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_of_dicts",
 			dict_of_dicts, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_of_dicts\":{\"dict_0\":"
-			"[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}}");
+	return CHECK_OUTPUT("{\"dict_of_dicts\":{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}");
 }
 
 static int
@@ -239,7 +261,6 @@ test_array_with_array_string_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -248,18 +269,18 @@ test_array_with_array_string_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[\"aaaa\"],[\"bbbb\"]]}");
+	return CHECK_OUTPUT("[[\"aaaa\"],[\"bbbb\"]]");
 }
 
 static int
 test_case_array_u64(void)
 {
 	int i;
-	memset(&response_data, 0, sizeof(response_data));
+
 	rte_tel_data_start_array(&response_data, RTE_TEL_U64_VAL);
 	for (i = 0; i < 5; i++)
 		rte_tel_data_add_array_u64(&response_data, i);
-	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
+	return CHECK_OUTPUT("[0,1,2,3,4]");
 }
 
 static int
@@ -268,15 +289,13 @@ test_case_add_dict_u64(void)
 	int i = 0;
 	char name_of_value[8];
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
 		sprintf(name_of_value, "dict_%d", i);
 		rte_tel_data_add_dict_u64(&response_data, name_of_value, i);
 	}
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,"
-			"\"dict_3\":3,\"dict_4\":4}}");
+	return CHECK_OUTPUT("{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}");
 }
 
 static int
@@ -290,7 +309,6 @@ test_dict_with_array_u64_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 10; i++) {
@@ -303,8 +321,7 @@ test_dict_with_array_u64_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],"
-			"\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}");
 }
 
 static int
@@ -318,7 +335,6 @@ test_array_with_array_u64_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	for (i = 0; i < 5; i++) {
@@ -328,7 +344,7 @@ test_array_with_array_u64_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
+	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
 static int
@@ -369,7 +385,7 @@ connect_to_socket(void)
 }
 
 static int
-test_telemetry_data(void)
+telemetry_data_autotest(void)
 {
 	typedef int (*test_case)(void);
 	unsigned int i = 0;
@@ -378,7 +394,9 @@ test_telemetry_data(void)
 	if (sock <= 0)
 		return -1;
 
-	test_case test_cases[] = {test_case_array_string,
+	test_case test_cases[] = {
+			test_simple_string,
+			test_case_array_string,
 			test_case_array_int, test_case_array_u64,
 			test_case_add_dict_int, test_case_add_dict_u64,
 			test_case_add_dict_string,
@@ -390,8 +408,9 @@ test_telemetry_data(void)
 			test_array_with_array_u64_values,
 			test_array_with_array_string_values };
 
-	rte_telemetry_register_cmd(REQUEST_CMD, test_cb, "Test");
+	rte_telemetry_register_cmd(REQUEST_CMD, telemetry_test_cb, "Test");
 	for (i = 0; i < RTE_DIM(test_cases); i++) {
+		memset(&response_data, 0, sizeof(response_data));
 		if (test_cases[i]() != 0) {
 			close(sock);
 			return -1;
@@ -401,4 +420,4 @@ test_telemetry_data(void)
 	return 0;
 }
 
-REGISTER_TEST_COMMAND(telemetry_data_autotest, test_telemetry_data);
+REGISTER_TEST_COMMAND(telemetry_data_autotest, telemetry_data_autotest);
-- 
2.34.1

RE: [PATCH v2 10/13] test/telemetry_data: refactor for maintainability

[Power, Ciara]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Monday 25 July 2022 17:36
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Power, Ciara
> <ciara.power@intel.com>
> Subject: [PATCH v2 10/13] test/telemetry_data: refactor for maintainability
> 
> To help with the writing and maintaining of test cases in this file we can make
> the following changes to it:
> 
> - rename non-test-case functions i.e. the infrastructure functions, to
>   not start with "test_", so that each sub-test case can be identified
>   by starting with that prefix.
> - add a comment at the start of the file explaining how tests are to be
>   written and managed, so as to keep consistency.
> - add a trivial test-case for returning a simple string value to use as
>   a reference example for those wanting to add test cases.
> - improve the key macro used for validating the output from each
>   function, so that the standard json preamble can be skipped for each
>   function. This hides more of the infrastructure implementation from
>   the user i.e. they don't need to worry what the actual command used is
>   called, and also shortens the output strings so we can avoid line
>   splitting in most cases.
> - add clearing the "response_data" structure to the loop calling each
>   test to avoid each test function having to do so individually.
> 
> Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  app/test/test_telemetry_data.c | 101 ++++++++++++++++++++-------------
>  1 file changed, 60 insertions(+), 41 deletions(-)
> 
> diff --git a/app/test/test_telemetry_data.c
> b/app/test/test_telemetry_data.c index 73eee293a1..5a85e790d3 100644
> --- a/app/test/test_telemetry_data.c
> +++ b/app/test/test_telemetry_data.c
> @@ -21,18 +21,45 @@
>  #define TELEMETRY_VERSION "v2"
>  #define REQUEST_CMD "/test"
>  #define BUF_SIZE 1024
> -#define TEST_OUTPUT(exp) test_output(__func__, exp)
> +#define CHECK_OUTPUT(exp) check_output(__func__, "{\""
> REQUEST_CMD
> +"\":" exp "}")
> +
> +/*
> + * Runs a series of test cases, checking the output of telemetry for
> +various different types of
> + * responses. On init, a single connection to DPDK telemetry is made,
> +and a single telemetry
> + * callback "/test" is registered. That callback always returns the
> +value of the static global
> + * variable "response_data", so each test case builds up that
> +structure, and then calls the
> + * "check_output" function to ensure the response received over the
> +socket for "/test" matches
> + * that expected for the response_data value populated.
> + *
> + * NOTE:
> + * - each test case function in this file should be added to the "test_cases"
> array in
> + *   test_telemetry_data function at the bottom of the file.
> + * - each test case function should populate the "response_data" global
> variable (below)
> + *   with the appropriate values which would be returned from a simulated
> telemetry function.
> + *   Then the test case function should have "return
> TEST_OUTPUT(<expected_data>);" as it's


[CP] nit: I think this should be CHECK_OUTPUT based on the macro rename above.


> + *   last line. The test infrastructure will then validate that the output when
> returning
> + *   "response_data" structure matches that in "<expected_data>".
> + * - the response_data structure will be zeroed on entry to each test
> function, so each function
> + *   can begin with a call to "rte_tel_data_string/start_array/start_dict" as so
> desired.
> + * - the expected_output for each function can be just the actual json data
> from the
> + *   "response_data" value. The CHECK_OUTPUT macro will include the
> appropriate "{\"/test\": ... }"
> + *   structure around the json output.
> + *
> + *  See test_simple_string(), or test_case_array_int() for a basic examples
> of test cases.
> + */

<snip>

[PATCH v2 11/13] test/telemetry_data: add test cases for character escaping

[Bruce Richardson]

Add in some basic unit tests to validate the character escaping being
done on string data values, which tests end-to-end processing of those
values beyond just the json-encoding steps tested by the
"telemetry_json_autotest".

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_data.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 5a85e790d3..cfa4d636f0 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -347,6 +347,30 @@ test_array_with_array_u64_values(void)
 	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
+static int
+test_string_char_escaping(void)
+{
+	rte_tel_data_string(&response_data, "hello,\nworld\n");
+	return CHECK_OUTPUT("\"hello,\\nworld\\n\"");
+}
+
+static int
+test_array_char_escaping(void)
+{
+	rte_tel_data_start_array(&response_data, RTE_TEL_STRING_VAL);
+	rte_tel_data_add_array_string(&response_data, "\\escape\r");
+	rte_tel_data_add_array_string(&response_data, "characters\n");
+	return CHECK_OUTPUT("[\"\\\\escape\\r\",\"characters\\n\"]");
+}
+
+static int
+test_dict_char_escaping(void)
+{
+	rte_tel_data_start_dict(&response_data);
+	rte_tel_data_add_dict_string(&response_data, "name", "escaped\n\tvalue");
+	return CHECK_OUTPUT("{\"name\":\"escaped\\n\\tvalue\"}");
+}
+
 static int
 connect_to_socket(void)
 {
@@ -406,7 +430,11 @@ telemetry_data_autotest(void)
 			test_dict_with_dict_values,
 			test_array_with_array_int_values,
 			test_array_with_array_u64_values,
-			test_array_with_array_string_values };
+			test_array_with_array_string_values,
+			test_string_char_escaping,
+			test_array_char_escaping,
+			test_dict_char_escaping,
+	};
 
 	rte_telemetry_register_cmd(REQUEST_CMD, telemetry_test_cb, "Test");
 	for (i = 0; i < RTE_DIM(test_cases); i++) {
-- 
2.34.1

[PATCH v2 12/13] telemetry: eliminate duplicate code for json output

[Bruce Richardson]

When preparing the json response to a telemetry socket query, the code
for prefixing the command name, and appending the file "}" on the end of
the response was duplicated for multiple reply types. Taking this code
out of the switch statement reduces the duplication and makes the code
more maintainable.

For completeness of testing, add in a test case to validate the "null"
response type - the only leg of the switch statment not already covered
by an existing test case in the telemetry_data tests.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 app/test/test_telemetry_data.c |  7 +++++++
 lib/telemetry/telemetry.c      | 35 ++++++++++++----------------------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index cfa4d636f0..d0fc78474e 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -93,6 +93,12 @@ check_output(const char *func_name, const char *expected)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_null_return(void)
+{
+	return CHECK_OUTPUT("null");
+}
+
 static int
 test_simple_string(void)
 {
@@ -419,6 +425,7 @@ telemetry_data_autotest(void)
 		return -1;
 
 	test_case test_cases[] = {
+			test_null_return,
 			test_simple_string,
 			test_case_array_string,
 			test_case_array_int, test_case_array_u64,
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 03651e947d..cf60d27bd4 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -233,27 +233,22 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 
 	RTE_BUILD_BUG_ON(sizeof(out_buf) < MAX_CMD_LEN +
 			RTE_TEL_MAX_SINGLE_STRING_LEN + 10);
+
+	prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
+			MAX_CMD_LEN, cmd);
+	cb_data_buf = &out_buf[prefix_used];
+	buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
+
 	switch (d->type) {
 	case RTE_TEL_NULL:
-		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":null}",
-				MAX_CMD_LEN, cmd ? cmd : "none");
+		used = strlcpy(cb_data_buf, "null", buf_len);
 		break;
-	case RTE_TEL_STRING:
-		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-				MAX_CMD_LEN, cmd);
-		cb_data_buf = &out_buf[prefix_used];
-		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
 
+	case RTE_TEL_STRING:
 		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
-		used += prefix_used;
-		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
-	case RTE_TEL_DICT:
-		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-				MAX_CMD_LEN, cmd);
-		cb_data_buf = &out_buf[prefix_used];
-		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
 
+	case RTE_TEL_DICT:
 		used = rte_tel_json_empty_obj(cb_data_buf, buf_len, 0);
 		for (i = 0; i < d->data_len; i++) {
 			const struct tel_dict_entry *v = &d->data.dict[i];
@@ -289,18 +284,12 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 			}
 			}
 		}
-		used += prefix_used;
-		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
+
 	case RTE_TEL_ARRAY_STRING:
 	case RTE_TEL_ARRAY_INT:
 	case RTE_TEL_ARRAY_U64:
 	case RTE_TEL_ARRAY_CONTAINER:
-		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-				MAX_CMD_LEN, cmd);
-		cb_data_buf = &out_buf[prefix_used];
-		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
-
 		used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
 		for (i = 0; i < d->data_len; i++)
 			if (d->type == RTE_TEL_ARRAY_STRING)
@@ -328,10 +317,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				if (!rec_data->keep)
 					rte_tel_data_free(rec_data->data);
 			}
-		used += prefix_used;
-		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
 	}
+	used += prefix_used;
+	used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 	if (write(s, out_buf, used) < 0)
 		perror("Error writing to socket");
 }
-- 
2.34.1

[PATCH v2 13/13] telemetry: make help command more helpful

[Bruce Richardson]

The /help telemetry command prints out the help text for the given
command passed in as parameter. However, entering /help without any
parameters does not give any useful information as to the fact that you
need to pass in a command to get help on. Update the command so it
prints its own help text when called without any parameters.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/telemetry.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index cf60d27bd4..09febff0ae 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -139,15 +139,17 @@ command_help(const char *cmd __rte_unused, const char *params,
 		struct rte_tel_data *d)
 {
 	int i;
+	/* if no parameters return our own help text */
+	const char *to_lookup = (params == NULL ? cmd : params);
 
-	if (!params)
-		return -1;
 	rte_tel_data_start_dict(d);
 	rte_spinlock_lock(&callback_sl);
 	for (i = 0; i < num_callbacks; i++)
-		if (strcmp(params, callbacks[i].cmd) == 0) {
-			rte_tel_data_add_dict_string(d, params,
-					callbacks[i].help);
+		if (strcmp(to_lookup, callbacks[i].cmd) == 0) {
+			if (params == NULL)
+				rte_tel_data_string(d, callbacks[i].help);
+			else
+				rte_tel_data_add_dict_string(d, params,	callbacks[i].help);
 			break;
 		}
 	rte_spinlock_unlock(&callback_sl);
-- 
2.34.1

RE: [PATCH v2 00/13] telemetry JSON escaping and other enhancements

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Monday, 25 July 2022 18.35
> 
> This patchset contains fixes for the problem of handling characters
> returned by telemetry callbacks which require escaping when encoded in
> JSON format. It also includes unit tests to validate the correct
> encoding in such scenarios and a number of smaller enhancements to
> telemetry and telemetry testing.
> 
> RFC->V2:
> * limited characters allowed in dictionary element names and command
>   names to side-step the encoding problems there.
> * added support for proper escaping of dictionary string values
> * added more testing and test cases
> * added other misc telemetry cleanups and refactoring
> 

Good job, Bruce!

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

Re: [PATCH v2 00/13] telemetry JSON escaping and other enhancements

[fengchengwen]

Hi Bruce,

I think escape the string at begin (following function) seem more simple:
	rte_tel_data_string
	rte_tel_data_add_array_string
	rte_tel_data_add_dict_string

int
rte_tel_data_string(struct rte_tel_data *d, const char *str)
{
	d->type = RTE_TEL_STRING;
	d->data_len = strlcpy(d->data.str, str, sizeof(d->data.str));
		// e.g. do escape here!
	if (d->data_len >= RTE_TEL_MAX_SINGLE_STRING_LEN) {
		d->data_len = RTE_TEL_MAX_SINGLE_STRING_LEN - 1;
		return E2BIG; /* not necessarily and error, just truncation */
	}
	return 0;
}

Advantages:
1. simpler implementation
2. application are directly visible the result (by judge API retval) without waiting for JSON encapsulation.

Disadvantages:
1. not friend for new output format, but currently telemetry deep depend on json, so I think it's OK for it.


On 2022/7/26 0:35, Bruce Richardson wrote:
> This patchset contains fixes for the problem of handling characters
> returned by telemetry callbacks which require escaping when encoded in
> JSON format. It also includes unit tests to validate the correct
> encoding in such scenarios and a number of smaller enhancements to
> telemetry and telemetry testing.
> 
> RFC->V2:
> * limited characters allowed in dictionary element names and command
>   names to side-step the encoding problems there.
> * added support for proper escaping of dictionary string values
> * added more testing and test cases
> * added other misc telemetry cleanups and refactoring
> 
> Bruce Richardson (13):
>   test/telemetry_json: print success or failure per subtest
>   telemetry: fix escaping of invalid json characters
>   test/telemetry_json: add test for string character escaping
>   telemetry: add escaping of strings in arrays
>   test/telemetry-json: add test for escaping strings in arrays
>   telemetry: limit characters allowed in dictionary names
>   telemetry: add escaping of strings in dicts
>   test/telemetry_json: add test for string escaping in objects
>   telemetry: limit command characters
>   test/telemetry_data: refactor for maintainability
>   test/telemetry_data: add test cases for character escaping
>   telemetry: eliminate duplicate code for json output
>   telemetry: make help command more helpful
> 
>  app/test/test_telemetry_data.c       | 138 +++++++++++++++++++--------
>  app/test/test_telemetry_json.c       |  98 +++++++++++++++++--
>  doc/guides/rel_notes/deprecation.rst |   8 --
>  lib/telemetry/rte_telemetry.h        |   8 ++
>  lib/telemetry/telemetry.c            |  51 +++++-----
>  lib/telemetry/telemetry_data.c       |  32 +++++++
>  lib/telemetry/telemetry_json.h       |  72 ++++++++++++--
>  7 files changed, 318 insertions(+), 89 deletions(-)
> 
> --
> 2.34.1
> 
> 
> .
> 

Re: [PATCH v2 00/13] telemetry JSON escaping and other enhancements

[Bruce Richardson]

On Wed, Jul 27, 2022 at 09:51:04AM +0800, fengchengwen wrote:
> Hi Bruce,
> 
> I think escape the string at begin (following function) seem more simple:
> 	rte_tel_data_string
> 	rte_tel_data_add_array_string
> 	rte_tel_data_add_dict_string
> 
> int
> rte_tel_data_string(struct rte_tel_data *d, const char *str)
> {
> 	d->type = RTE_TEL_STRING;
> 	d->data_len = strlcpy(d->data.str, str, sizeof(d->data.str));
> 		// e.g. do escape here!
> 	if (d->data_len >= RTE_TEL_MAX_SINGLE_STRING_LEN) {
> 		d->data_len = RTE_TEL_MAX_SINGLE_STRING_LEN - 1;
> 		return E2BIG; /* not necessarily and error, just truncation */
> 	}
> 	return 0;
> }
> 
> Advantages:
> 1. simpler implementation
> 2. application are directly visible the result (by judge API retval) without waiting for JSON encapsulation.
> 
> Disadvantages:
> 1. not friend for new output format, but currently telemetry deep depend on json, so I think it's OK for it.
>
I'm quite happy to implement things in a simpler way, however, in the past
there was a great concern to keep things flexible enough for future changes
to add other output formats. By that logic, keeping the escaping in the
json layer is the correct design choice.

However, adding escaping on addition to the return data structure may not
be that much of an inconvience to other output formats too, so perhaps it's
acceptable.

Again, looking for more input and consensus from the community. I am happy
to go with either approach for escaping - putting it in the data return
layer or the json one.

/Bruce

RE: [PATCH v2 00/13] telemetry JSON escaping and other enhancements

[Morten Brørup]

> From: Bruce Richardson [mailto:bruce.richardson@intel.com]
> Sent: Wednesday, 27 July 2022 11.13
> 
> On Wed, Jul 27, 2022 at 09:51:04AM +0800, fengchengwen wrote:
> > Hi Bruce,
> >
> > I think escape the string at begin (following function) seem more
> simple:
> > 	rte_tel_data_string
> > 	rte_tel_data_add_array_string
> > 	rte_tel_data_add_dict_string
> >
> > int
> > rte_tel_data_string(struct rte_tel_data *d, const char *str)
> > {
> > 	d->type = RTE_TEL_STRING;
> > 	d->data_len = strlcpy(d->data.str, str, sizeof(d->data.str));
> > 		// e.g. do escape here!
> > 	if (d->data_len >= RTE_TEL_MAX_SINGLE_STRING_LEN) {
> > 		d->data_len = RTE_TEL_MAX_SINGLE_STRING_LEN - 1;
> > 		return E2BIG; /* not necessarily and error, just truncation
> */
> > 	}
> > 	return 0;
> > }
> >
> > Advantages:
> > 1. simpler implementation
> > 2. application are directly visible the result (by judge API retval)
> without waiting for JSON encapsulation.
> >
> > Disadvantages:
> > 1. not friend for new output format, but currently telemetry deep
> depend on json, so I think it's OK for it.

The telemetry library currently only implements JSON as the output format. I understand why this gives the impression that it depends on JSON. But it does not.

When the DPDK telemetry library was initially discussed, JSON was the only target. Before that settled in too deeply, I argued for using generic data types internally, so it could support other formats (e.g. SNMP), which resulted in the current layering of the telemetry library - a both elegant and practical solution.

> >
> I'm quite happy to implement things in a simpler way, however, in the
> past
> there was a great concern to keep things flexible enough for future
> changes
> to add other output formats. By that logic, keeping the escaping in the
> json layer is the correct design choice.
> 
> However, adding escaping on addition to the return data structure may
> not
> be that much of an inconvience to other output formats too, so perhaps
> it's
> acceptable.
> 
> Again, looking for more input and consensus from the community. I am
> happy
> to go with either approach for escaping - putting it in the data return
> layer or the json one.
> 
> /Bruce

JSON encoding belongs in the JSON output layer.

Don't make assumptions about other output formats! The char '\' might not be legal in all output formats. And I certainly don't want any newlines escaped in SNMP text objects.

RE: [PATCH v2 00/13] telemetry JSON escaping and other enhancements

[Power, Ciara]

> -----Original Message-----
> From: Bruce Richardson <bruce.richardson@intel.com>
> Sent: Monday 25 July 2022 17:35
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>
> Subject: [PATCH v2 00/13] telemetry JSON escaping and other
> enhancements
> 
> This patchset contains fixes for the problem of handling characters returned
> by telemetry callbacks which require escaping when encoded in JSON
> format. It also includes unit tests to validate the correct encoding in such
> scenarios and a number of smaller enhancements to telemetry and
> telemetry testing.
> 
> RFC->V2:
> * limited characters allowed in dictionary element names and command
>   names to side-step the encoding problems there.
> * added support for proper escaping of dictionary string values
> * added more testing and test cases
> * added other misc telemetry cleanups and refactoring
> 
> Bruce Richardson (13):
>   test/telemetry_json: print success or failure per subtest
>   telemetry: fix escaping of invalid json characters
>   test/telemetry_json: add test for string character escaping
>   telemetry: add escaping of strings in arrays
>   test/telemetry-json: add test for escaping strings in arrays
>   telemetry: limit characters allowed in dictionary names
>   telemetry: add escaping of strings in dicts
>   test/telemetry_json: add test for string escaping in objects
>   telemetry: limit command characters
>   test/telemetry_data: refactor for maintainability
>   test/telemetry_data: add test cases for character escaping
>   telemetry: eliminate duplicate code for json output
>   telemetry: make help command more helpful
> 
>  app/test/test_telemetry_data.c       | 138 +++++++++++++++++++--------
>  app/test/test_telemetry_json.c       |  98 +++++++++++++++++--
>  doc/guides/rel_notes/deprecation.rst |   8 --
>  lib/telemetry/rte_telemetry.h        |   8 ++
>  lib/telemetry/telemetry.c            |  51 +++++-----
>  lib/telemetry/telemetry_data.c       |  32 +++++++
>  lib/telemetry/telemetry_json.h       |  72 ++++++++++++--
>  7 files changed, 318 insertions(+), 89 deletions(-)
> 
> --
> 2.34.1
 
Looks great, thanks Bruce.

Series-Acked-by: Ciara Power <ciara.power@intel.com>

[PATCH v3 00/13] telemetry JSON escaping and other enhancements

[Bruce Richardson]

This patchset contains fixes for the problem of handling characters returned by
telemetry callbacks which require escaping when encoded in JSON format. It also
includes unit tests to validate the correct encoding in such scenarios and a
number of smaller enhancements to telemetry and telemetry testing

V3:
* reorder patchset to put previous patch 6 (dict name limiting) at the
  start of the series
* fix missed rename of TEST_OUTPUT to CHECK_OUTPUT in patch 10

RFC->V2:
* limited characters allowed in dictionary element names and command
  names to side-step the encoding problems there
* added support for proper escaping of dictionary string values
* added more testing and test cases
* added other misc telemetry cleanups and refactoring

Bruce Richardson (13):
  telemetry: limit characters allowed in dictionary names
  test/telemetry_json: print success or failure per subtest
  telemetry: fix escaping of invalid json characters
  test/telemetry_json: add test for string character escaping
  telemetry: add escaping of strings in arrays
  test/telemetry-json: add test for escaping strings in arrays
  telemetry: add escaping of strings in dicts
  test/telemetry_json: add test for string escaping in objects
  telemetry: limit command characters
  test/telemetry_data: refactor for maintainability
  test/telemetry_data: add test cases for character escaping
  telemetry: eliminate duplicate code for json output
  telemetry: make help command more helpful

 app/test/test_telemetry_data.c       | 138 +++++++++++++++++++--------
 app/test/test_telemetry_json.c       |  98 +++++++++++++++++--
 doc/guides/rel_notes/deprecation.rst |   8 --
 lib/telemetry/rte_telemetry.h        |   8 ++
 lib/telemetry/telemetry.c            |  51 +++++-----
 lib/telemetry/telemetry_data.c       |  32 +++++++
 lib/telemetry/telemetry_json.h       |  72 ++++++++++++--
 7 files changed, 318 insertions(+), 89 deletions(-)

-- 
2.34.1

[PATCH v3 01/13] telemetry: limit characters allowed in dictionary names

[Bruce Richardson]

To save issues with encoding the names of values in dicts, we limit the
allowed names to a subset of character values. This list of allowed
characters can be expanded as necessary in future.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/telemetry/rte_telemetry.h  |  8 ++++++++
 lib/telemetry/telemetry_data.c | 32 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+)

diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index d586dd0fc1..a0d21d6b7f 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -64,6 +64,10 @@ rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type);
 /**
  * Start a dictionary of values for returning from a callback
  *
+ * Dictionaries consist of key-values pairs to be returned, where the keys,
+ * or names, are strings and the values can be any of the types supported by telemetry.
+ * Name strings may only contain alphanumeric characters as well as '_' or '/'
+ *
  * @param d
  *   The data structure passed to the callback
  * @return
@@ -159,6 +163,7 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The string to be stored in the dict
  * @return
@@ -177,6 +182,7 @@ rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The number to be stored in the dict
  * @return
@@ -193,6 +199,7 @@ rte_tel_data_add_dict_int(struct rte_tel_data *d, const char *name, int val);
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The number to be stored in the dict
  * @return
@@ -212,6 +219,7 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
  *   The data structure passed to the callback
  * @param name
  *   The name the value is to be stored under in the dict.
+ *   Must contain only alphanumeric characters or the symbols: '_' or '/'
  * @param val
  *   The pointer to the container to be stored in the dict.
  * @param keep
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index e14ae3c4d4..b5cd74b25b 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -3,6 +3,8 @@
  */
 
 #undef RTE_USE_LIBBSD
+#include <stdbool.h>
+
 #include <rte_string_fns.h>
 
 #include "telemetry_data.h"
@@ -92,6 +94,24 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
 	return 0;
 }
 
+static bool
+valid_name(const char *name)
+{
+	char allowed[128] = {
+			['0' ... '9'] = 1,
+			['A' ... 'Z'] = 1,
+			['a' ... 'z'] = 1,
+			['_'] = 1,
+			['/'] = 1,
+	};
+	while (*name != '\0') {
+		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
+			return false;
+		name++;
+	}
+	return true;
+}
+
 int
 rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
 		const char *val)
@@ -104,6 +124,9 @@ rte_tel_data_add_dict_string(struct rte_tel_data *d, const char *name,
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_STRING_VAL;
 	vbytes = strlcpy(e->value.sval, val, RTE_TEL_MAX_STRING_LEN);
@@ -123,6 +146,9 @@ rte_tel_data_add_dict_int(struct rte_tel_data *d, const char *name, int val)
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_INT_VAL;
 	e->value.ival = val;
@@ -140,6 +166,9 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_U64_VAL;
 	e->value.u64val = val;
@@ -161,6 +190,9 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
 		return -ENOSPC;
 
+	if (!valid_name(name))
+		return -EINVAL;
+
 	d->data_len++;
 	e->type = RTE_TEL_CONTAINER;
 	e->value.container.data = val;
-- 
2.34.1

[PATCH v3 02/13] test/telemetry_json: print success or failure per subtest

[Bruce Richardson]

rather than just printing out success or failure at the end of the test
only, print out "OK" or "ERROR" for each individual test case within the
overall test. As part of this, ensure each case returns 0 on success and
any other value on failure.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_json.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 790181d316..748b7cfe5a 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -102,8 +102,10 @@ test_large_array_element(void)
 
 	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, str);
 	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != 0)
+		return -1;
 
-	return strlen(buf) != 0;
+	return strncmp(expected, buf, sizeof(buf));
 }
 
 static int
@@ -117,20 +119,33 @@ test_large_obj_element(void)
 
 	used = rte_tel_json_add_obj_u64(buf, sizeof(buf), used, str, 0);
 	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != 0)
+		return -1;
 
-	return strlen(buf) != 0;
+	return strncmp(expected, buf, sizeof(buf));
 }
 
+typedef int (*test_fn)(void);
+
 static int
 test_telemetry_json(void)
 {
-	if (test_basic_array() < 0 ||
-			test_basic_obj() < 0 ||
-			test_overflow_array() < 0 ||
-			test_overflow_obj() < 0 ||
-			test_large_array_element() < 0 ||
-			test_large_obj_element() < 0)
-		return -1;
+	unsigned int i;
+	test_fn fns[] = {
+			test_basic_array,
+			test_basic_obj,
+			test_overflow_array,
+			test_overflow_obj,
+			test_large_array_element,
+			test_large_obj_element,
+	};
+	for (i = 0; i < RTE_DIM(fns); i++)
+		if (fns[i]() == 0)
+			printf("OK\n");
+		else {
+			printf("ERROR\n");
+			return -1;
+		}
 	return 0;
 }
 
-- 
2.34.1

[PATCH v3 03/13] telemetry: fix escaping of invalid json characters

[Bruce Richardson]

For string values returned from telemetry, escape any values that cannot
normally appear in a json string. According to the json spec[1], the
characters than need to be handled are control chars (char value < 0x20)
and '"' and '\' characters.

To handle this, we replace the snprintf call with a separate string
copying and encapsulation routine which checks each character as it
copies it to the final array.

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

Fixes: 6dd571fd07c3 ("telemetry: introduce new functionality")
Bugzilla ID: 1037

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/telemetry/telemetry.c      | 11 +++++---
 lib/telemetry/telemetry_json.h | 48 +++++++++++++++++++++++++++++++++-
 2 files changed, 55 insertions(+), 4 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index c6fd03a5ab..7188b1905c 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -232,9 +232,14 @@ 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);
+		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
+				MAX_CMD_LEN, cmd);
+		cb_data_buf = &out_buf[prefix_used];
+		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
+
+		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
+		used += prefix_used;
+		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
 	case RTE_TEL_DICT:
 		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index db70690274..13df5d07e3 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -44,6 +44,52 @@ __json_snprintf(char *buf, const int len, const char *format, ...)
 	return 0; /* nothing written or modified */
 }
 
+static const char control_chars[0x20] = {
+		['\n'] = 'n',
+		['\r'] = 'r',
+		['\t'] = 't',
+};
+
+/**
+ * @internal
+ * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
+ * except that it does proper escaping as necessary.
+ * Drops any invalid characters we don't support
+ */
+static inline int
+__json_format_str(char *buf, const int len, const char *str)
+{
+	char tmp[len];
+	int tmpidx = 0;
+
+	tmp[tmpidx++] = '"';
+	while (*str != '\0') {
+		if (*str < (int)RTE_DIM(control_chars)) {
+			int idx = *str;  /* compilers don't like char type as index */
+			if (control_chars[idx] != 0) {
+				tmp[tmpidx++] = '\\';
+				tmp[tmpidx++] = control_chars[idx];
+			}
+		} else if (*str == '"' || *str == '\\') {
+			tmp[tmpidx++] = '\\';
+			tmp[tmpidx++] = *str;
+		} else
+			tmp[tmpidx++] = *str;
+		/* we always need space for closing quote and null character.
+		 * Ensuring at least two free characters also means we can always take an
+		 * escaped character like "\n" without overflowing
+		 */
+		if (tmpidx > len - 2)
+			return 0;
+		str++;
+	}
+	tmp[tmpidx++] = '"';
+	tmp[tmpidx] = '\0';
+
+	strcpy(buf, tmp);
+	return tmpidx;
+}
+
 /* Copies an empty array into the provided buffer. */
 static inline int
 rte_tel_json_empty_array(char *buf, const int len, const int used)
@@ -62,7 +108,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
 static inline int
 rte_tel_json_str(char *buf, const int len, const int used, const char *str)
 {
-	return used + __json_snprintf(buf + used, len - used, "\"%s\"", str);
+	return used + __json_format_str(buf + used, len - used, str);
 }
 
 /* Appends a string into the JSON array in the provided buffer. */
-- 
2.34.1

[PATCH v3 04/13] test/telemetry_json: add test for string character escaping

[Bruce Richardson]

Add unit test to validate that when creating a string response in json,
that characters such as \n or quotes are properly escaped.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_json.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 748b7cfe5a..955c2e5b1b 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -125,6 +125,22 @@ test_large_obj_element(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_string_char_escaping(void)
+{
+	static const char str[] = "A string across\ntwo lines and \"with quotes\"!";
+	const char *expected = "\"A string across\\ntwo lines and \\\"with quotes\\\"!\"";
+	char buf[sizeof(str) + 10];
+	int used = 0;
+
+	used = rte_tel_json_str(buf, sizeof(buf), used, str);
+	printf("%s: buf = '%s', expected = '%s'\n", __func__, buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -138,6 +154,7 @@ test_telemetry_json(void)
 			test_overflow_obj,
 			test_large_array_element,
 			test_large_obj_element,
+			test_string_char_escaping,
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[PATCH v3 05/13] telemetry: add escaping of strings in arrays

[Bruce Richardson]

When strings are added to an array variable, we need to properly escape
the invalid json characters in the strings.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/telemetry/telemetry_json.h | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index 13df5d07e3..c4442a0bf0 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -52,17 +52,22 @@ static const char control_chars[0x20] = {
 
 /**
  * @internal
- * Does the same as __json_snprintf(buf, len, "\"%s\"", str)
- * except that it does proper escaping as necessary.
+ * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
+ * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
+ * time constants not needing escaping.
  * Drops any invalid characters we don't support
  */
 static inline int
-__json_format_str(char *buf, const int len, const char *str)
+__json_format_str(char *buf, const int len, const char *prefix, const char *str, const char *suffix)
 {
 	char tmp[len];
 	int tmpidx = 0;
 
-	tmp[tmpidx++] = '"';
+	while (*prefix != '\0' && tmpidx < len)
+		tmp[tmpidx++] = *prefix++;
+	if (tmpidx >= len)
+		return 0;
+
 	while (*str != '\0') {
 		if (*str < (int)RTE_DIM(control_chars)) {
 			int idx = *str;  /* compilers don't like char type as index */
@@ -75,7 +80,7 @@ __json_format_str(char *buf, const int len, const char *str)
 			tmp[tmpidx++] = *str;
 		} else
 			tmp[tmpidx++] = *str;
-		/* we always need space for closing quote and null character.
+		/* we always need space for (at minimum) closing quote and null character.
 		 * Ensuring at least two free characters also means we can always take an
 		 * escaped character like "\n" without overflowing
 		 */
@@ -83,7 +88,12 @@ __json_format_str(char *buf, const int len, const char *str)
 			return 0;
 		str++;
 	}
-	tmp[tmpidx++] = '"';
+
+	while (*suffix != '\0' && tmpidx < len)
+		tmp[tmpidx++] = *suffix++;
+	if (tmpidx >= len)
+		return 0;
+
 	tmp[tmpidx] = '\0';
 
 	strcpy(buf, tmp);
@@ -108,7 +118,7 @@ rte_tel_json_empty_obj(char *buf, const int len, const int used)
 static inline int
 rte_tel_json_str(char *buf, const int len, const int used, const char *str)
 {
-	return used + __json_format_str(buf + used, len - used, str);
+	return used + __json_format_str(buf + used, len - used, "\"", str, "\"");
 }
 
 /* Appends a string into the JSON array in the provided buffer. */
@@ -118,9 +128,9 @@ rte_tel_json_add_array_string(char *buf, const int len, const int used,
 {
 	int ret, end = used - 1; /* strip off final delimiter */
 	if (used <= 2) /* assume empty, since minimum is '[]' */
-		return __json_snprintf(buf, len, "[\"%s\"]", str);
+		return __json_format_str(buf, len, "[\"", str, "\"]");
 
-	ret = __json_snprintf(buf + end, len - end, ",\"%s\"]", str);
+	ret = __json_format_str(buf + end, len - end, ",\"", str, "\"]");
 	return ret == 0 ? used : end + ret;
 }
 
-- 
2.34.1

[PATCH v3 06/13] test/telemetry-json: add test for escaping strings in arrays

[Bruce Richardson]

Add test-case to validate that when adding strings to arrays, the
strings are properly escaped to remove any invalid characters.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_json.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 955c2e5b1b..31a13ea1d7 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -141,6 +141,29 @@ test_string_char_escaping(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_array_char_escaping(void)
+{
+	/* "meaning of life", with tab between first two words, '\n' at end,
+	 * and "life" in quotes, followed by "all the fish" in quotes
+	 */
+	const char *expected = "[\"meaning\\tof \\\"life\\\"\\n\",\"\\\"all the fish\\\"\"]";
+	char buf[1024];
+	int used = 0;
+
+	used = rte_tel_json_empty_array(buf, sizeof(buf), used);
+	if (used != 2 || strcmp(buf, "[]"))
+		return -1;
+
+	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, "meaning\tof \"life\"\n");
+	used = rte_tel_json_add_array_string(buf, sizeof(buf), used, "\"all the fish\"");
+
+	printf("buf = '%s', expected = '%s'\n", buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -155,6 +178,7 @@ test_telemetry_json(void)
 			test_large_array_element,
 			test_large_obj_element,
 			test_string_char_escaping,
+			test_array_char_escaping,
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[PATCH v3 07/13] telemetry: add escaping of strings in dicts

[Bruce Richardson]

When strings are added to an dict variable, we need to properly escape
the invalid json characters in the strings.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/telemetry/telemetry_json.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index c4442a0bf0..e3fae7c30d 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -54,7 +54,7 @@ static const char control_chars[0x20] = {
  * @internal
  * This function acts the same as __json_snprintf(buf, len, "%s%s%s", prefix, str, suffix)
  * except that it does proper escaping of "str" as necessary. Prefix and suffix should be compile-
- * time constants not needing escaping.
+ * time constants, or values not needing escaping.
  * Drops any invalid characters we don't support
  */
 static inline int
@@ -219,12 +219,16 @@ static inline int
 rte_tel_json_add_obj_str(char *buf, const int len, const int used,
 		const char *name, const char *val)
 {
+	char tmp_name[RTE_TEL_MAX_STRING_LEN + 5];
 	int ret, end = used - 1;
+
+	/* names are limited to certain characters so need no escaping */
+	snprintf(tmp_name, sizeof(tmp_name), "{\"%s\":\"", name);
 	if (used <= 2) /* assume empty, since minimum is '{}' */
-		return __json_snprintf(buf, len, "{\"%s\":\"%s\"}", name, val);
+		return __json_format_str(buf, len, tmp_name, val, "\"}");
 
-	ret = __json_snprintf(buf + end, len - end, ",\"%s\":\"%s\"}",
-			name, val);
+	tmp_name[0] = ',';  /* replace '{' with ',' at start */
+	ret = __json_format_str(buf + end, len - end, tmp_name, val, "\"}");
 	return ret == 0 ? used : end + ret;
 }
 
-- 
2.34.1

[PATCH v3 08/13] test/telemetry_json: add test for string escaping in objects

[Bruce Richardson]

Add a test-case to validate that when adding strings either as the name
or the value of an entry in an object, that all values are escaped
properly.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_json.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/app/test/test_telemetry_json.c b/app/test/test_telemetry_json.c
index 31a13ea1d7..184c3ba9f1 100644
--- a/app/test/test_telemetry_json.c
+++ b/app/test/test_telemetry_json.c
@@ -164,6 +164,29 @@ test_array_char_escaping(void)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_obj_char_escaping(void)
+{
+	const char *expected = "{\"good\":\"Clint Eastwood\\n\","
+			"\"bad\":\"Lee\\tVan\\tCleef\","
+			"\"ugly\":\"\\rEli Wallach\"}";
+	char buf[1024];
+	int used = 0;
+
+	used = rte_tel_json_empty_obj(buf, sizeof(buf), used);
+	if (used != 2 || strcmp(buf, "{}"))
+		return -1;
+
+	used = rte_tel_json_add_obj_str(buf, sizeof(buf), used, "good", "Clint Eastwood\n");
+	used = rte_tel_json_add_obj_str(buf, sizeof(buf), used, "bad", "Lee\tVan\tCleef");
+	used = rte_tel_json_add_obj_str(buf, sizeof(buf), used, "ugly", "\rEli Wallach");
+
+	printf("buf = '%s', expected = '%s'\n", buf, expected);
+	if (used != (int)strlen(expected))
+		return -1;
+	return strncmp(expected, buf, sizeof(buf));
+}
+
 typedef int (*test_fn)(void);
 
 static int
@@ -179,6 +202,7 @@ test_telemetry_json(void)
 			test_large_obj_element,
 			test_string_char_escaping,
 			test_array_char_escaping,
+			test_obj_char_escaping
 	};
 	for (i = 0; i < RTE_DIM(fns); i++)
 		if (fns[i]() == 0)
-- 
2.34.1

[PATCH v3 09/13] telemetry: limit command characters

[Bruce Richardson]

Limit the telemetry command characters to the minimum set needed for
current implementations. This prevents issues with invalid json
characters needing to be escaped on replies.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 doc/guides/rel_notes/deprecation.rst | 8 --------
 lib/telemetry/telemetry.c            | 7 +++++++
 2 files changed, 7 insertions(+), 8 deletions(-)

diff --git a/doc/guides/rel_notes/deprecation.rst b/doc/guides/rel_notes/deprecation.rst
index e7583cae4c..d1c93ca7e3 100644
--- a/doc/guides/rel_notes/deprecation.rst
+++ b/doc/guides/rel_notes/deprecation.rst
@@ -212,14 +212,6 @@ Deprecation Notices
 * metrics: The function ``rte_metrics_init`` will have a non-void return
   in order to notify errors instead of calling ``rte_exit``.
 
-* telemetry: The allowed characters in names for dictionary values
-  will be limited to alphanumeric characters
-  and a small subset of additional printable characters.
-  This will ensure that all dictionary parameter names can be output
-  without escaping in JSON - or in any future output format used.
-  Names for the telemetry commands will be similarly limited.
-  The parameters for telemetry commands are unaffected by this change.
-
 * net/octeontx_ep: The driver ``octeontx_ep`` was to support OCTEON TX
   line of products.
   It will be renamed to ``octeon_ep`` in DPDK 22.11 to apply for
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 7188b1905c..03651e947d 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -70,12 +70,19 @@ int
 rte_telemetry_register_cmd(const char *cmd, telemetry_cb fn, const char *help)
 {
 	struct cmd_callback *new_callbacks;
+	const char *cmdp = cmd;
 	int i = 0;
 
 	if (strlen(cmd) >= MAX_CMD_LEN || fn == NULL || cmd[0] != '/'
 			|| strlen(help) >= RTE_TEL_MAX_STRING_LEN)
 		return -EINVAL;
 
+	while (*cmdp != '\0') {
+		if (!isalnum(*cmdp) && *cmdp != '_' && *cmdp != '/')
+			return -EINVAL;
+		cmdp++;
+	}
+
 	rte_spinlock_lock(&callback_sl);
 	new_callbacks = realloc(callbacks, sizeof(callbacks[0]) * (num_callbacks + 1));
 	if (new_callbacks == NULL) {
-- 
2.34.1

[PATCH v3 10/13] test/telemetry_data: refactor for maintainability

[Bruce Richardson]

To help with the writing and maintaining of test cases in this file we
can make the following changes to it:

- rename non-test-case functions i.e. the infrastructure functions, to
  not start with "test_", so that each sub-test case can be identified
  by starting with that prefix.
- add a comment at the start of the file explaining how tests are to be
  written and managed, so as to keep consistency.
- add a trivial test-case for returning a simple string value to use as
  a reference example for those wanting to add test cases.
- improve the key macro used for validating the output from each
  function, so that the standard json preamble can be skipped for each
  function. This hides more of the infrastructure implementation from
  the user i.e. they don't need to worry what the actual command used is
  called, and also shortens the output strings so we can avoid line
  splitting in most cases.
- add clearing the "response_data" structure to the loop calling each
  test to avoid each test function having to do so individually.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_data.c | 101 ++++++++++++++++++++-------------
 1 file changed, 60 insertions(+), 41 deletions(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 73eee293a1..9d174dc99f 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -21,18 +21,45 @@
 #define TELEMETRY_VERSION "v2"
 #define REQUEST_CMD "/test"
 #define BUF_SIZE 1024
-#define TEST_OUTPUT(exp) test_output(__func__, exp)
+#define CHECK_OUTPUT(exp) check_output(__func__, "{\"" REQUEST_CMD "\":" exp "}")
+
+/*
+ * Runs a series of test cases, checking the output of telemetry for various different types of
+ * responses. On init, a single connection to DPDK telemetry is made, and a single telemetry
+ * callback "/test" is registered. That callback always returns the value of the static global
+ * variable "response_data", so each test case builds up that structure, and then calls the
+ * "check_output" function to ensure the response received over the socket for "/test" matches
+ * that expected for the response_data value populated.
+ *
+ * NOTE:
+ * - each test case function in this file should be added to the "test_cases" array in
+ *   test_telemetry_data function at the bottom of the file.
+ * - each test case function should populate the "response_data" global variable (below)
+ *   with the appropriate values which would be returned from a simulated telemetry function.
+ *   Then the test case function should have "return CHECK_OUTPUT(<expected_data>);" as it's
+ *   last line. The test infrastructure will then validate that the output when returning
+ *   "response_data" structure matches that in "<expected_data>".
+ * - the response_data structure will be zeroed on entry to each test function, so each function
+ *   can begin with a call to "rte_tel_data_string/start_array/start_dict" as so desired.
+ * - the expected_output for each function can be just the actual json data from the
+ *   "response_data" value. The CHECK_OUTPUT macro will include the appropriate "{\"/test\": ... }"
+ *   structure around the json output.
+ *
+ *  See test_simple_string(), or test_case_array_int() for a basic examples of test cases.
+ */
+
 
 static struct rte_tel_data response_data;
 static int sock;
 
+
 /*
  * This function is the callback registered with Telemetry to be used when
  * the /test command is requested. This callback returns the global data built
  * up by the individual test cases.
  */
 static int
-test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
+telemetry_test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
 		struct rte_tel_data *d)
 {
 	*d = response_data;
@@ -46,7 +73,7 @@ test_cb(const char *cmd __rte_unused, const char *params __rte_unused,
  * and is compared to the actual response received from Telemetry.
  */
 static int
-test_output(const char *func_name, const char *expected)
+check_output(const char *func_name, const char *expected)
 {
 	int bytes;
 	char buf[BUF_SIZE * 16];
@@ -66,6 +93,14 @@ test_output(const char *func_name, const char *expected)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_simple_string(void)
+{
+	rte_tel_data_string(&response_data, "Simple string");
+
+	return CHECK_OUTPUT("\"Simple string\"");
+}
+
 static int
 test_dict_with_array_int_values(void)
 {
@@ -77,7 +112,6 @@ test_dict_with_array_int_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
@@ -90,8 +124,7 @@ test_dict_with_array_int_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4],"
-			"\"dict_1\":[0,1,2,3,4]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[0,1,2,3,4],\"dict_1\":[0,1,2,3,4]}");
 }
 
 static int
@@ -105,7 +138,6 @@ test_array_with_array_int_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_INT_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	for (i = 0; i < 5; i++) {
@@ -115,18 +147,18 @@ test_array_with_array_int_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
+	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
 static int
 test_case_array_int(void)
 {
 	int i;
-	memset(&response_data, 0, sizeof(response_data));
+
 	rte_tel_data_start_array(&response_data, RTE_TEL_INT_VAL);
 	for (i = 0; i < 5; i++)
 		rte_tel_data_add_array_int(&response_data, i);
-	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
+	return CHECK_OUTPUT("[0,1,2,3,4]");
 }
 
 static int
@@ -135,7 +167,6 @@ test_case_add_dict_int(void)
 	int i = 0;
 	char name_of_value[8];
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
@@ -143,14 +174,12 @@ test_case_add_dict_int(void)
 		rte_tel_data_add_dict_int(&response_data, name_of_value, i);
 	}
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,"
-			"\"dict_3\":3,\"dict_4\":4}}");
+	return CHECK_OUTPUT("{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}");
 }
 
 static int
 test_case_array_string(void)
 {
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_STRING_VAL);
 	rte_tel_data_add_array_string(&response_data, "aaaa");
 	rte_tel_data_add_array_string(&response_data, "bbbb");
@@ -158,14 +187,12 @@ test_case_array_string(void)
 	rte_tel_data_add_array_string(&response_data, "dddd");
 	rte_tel_data_add_array_string(&response_data, "eeee");
 
-	return TEST_OUTPUT("{\"/test\":[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\","
-			"\"eeee\"]}");
+	return CHECK_OUTPUT("[\"aaaa\",\"bbbb\",\"cccc\",\"dddd\",\"eeee\"]");
 }
 
 static int
 test_case_add_dict_string(void)
 {
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_dict_string(&response_data, "dict_0", "aaaa");
@@ -173,8 +200,7 @@ test_case_add_dict_string(void)
 	rte_tel_data_add_dict_string(&response_data, "dict_2", "cccc");
 	rte_tel_data_add_dict_string(&response_data, "dict_3", "dddd");
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":\"aaaa\",\"dict_1\":"
-			"\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}}");
+	return CHECK_OUTPUT("{\"dict_0\":\"aaaa\",\"dict_1\":\"bbbb\",\"dict_2\":\"cccc\",\"dict_3\":\"dddd\"}");
 }
 
 
@@ -187,7 +213,6 @@ test_dict_with_array_string_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -198,8 +223,7 @@ test_dict_with_array_string_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[\"aaaa\"],\"dict_1\":"
-			"[\"bbbb\"]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}");
 }
 
 static int
@@ -214,7 +238,6 @@ test_dict_with_dict_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -226,8 +249,7 @@ test_dict_with_dict_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_of_dicts",
 			dict_of_dicts, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_of_dicts\":{\"dict_0\":"
-			"[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}}");
+	return CHECK_OUTPUT("{\"dict_of_dicts\":{\"dict_0\":[\"aaaa\"],\"dict_1\":[\"bbbb\"]}}");
 }
 
 static int
@@ -239,7 +261,6 @@ test_array_with_array_string_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_STRING_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	rte_tel_data_add_array_string(child_data, "aaaa");
@@ -248,18 +269,18 @@ test_array_with_array_string_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[\"aaaa\"],[\"bbbb\"]]}");
+	return CHECK_OUTPUT("[[\"aaaa\"],[\"bbbb\"]]");
 }
 
 static int
 test_case_array_u64(void)
 {
 	int i;
-	memset(&response_data, 0, sizeof(response_data));
+
 	rte_tel_data_start_array(&response_data, RTE_TEL_U64_VAL);
 	for (i = 0; i < 5; i++)
 		rte_tel_data_add_array_u64(&response_data, i);
-	return TEST_OUTPUT("{\"/test\":[0,1,2,3,4]}");
+	return CHECK_OUTPUT("[0,1,2,3,4]");
 }
 
 static int
@@ -268,15 +289,13 @@ test_case_add_dict_u64(void)
 	int i = 0;
 	char name_of_value[8];
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 5; i++) {
 		sprintf(name_of_value, "dict_%d", i);
 		rte_tel_data_add_dict_u64(&response_data, name_of_value, i);
 	}
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,"
-			"\"dict_3\":3,\"dict_4\":4}}");
+	return CHECK_OUTPUT("{\"dict_0\":0,\"dict_1\":1,\"dict_2\":2,\"dict_3\":3,\"dict_4\":4}");
 }
 
 static int
@@ -290,7 +309,6 @@ test_dict_with_array_u64_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_dict(&response_data);
 
 	for (i = 0; i < 10; i++) {
@@ -303,8 +321,7 @@ test_dict_with_array_u64_values(void)
 	rte_tel_data_add_dict_container(&response_data, "dict_1",
 	 child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],"
-			"\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}}");
+	return CHECK_OUTPUT("{\"dict_0\":[0,1,2,3,4,5,6,7,8,9],\"dict_1\":[0,1,2,3,4,5,6,7,8,9]}");
 }
 
 static int
@@ -318,7 +335,6 @@ test_array_with_array_u64_values(void)
 	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
 	rte_tel_data_start_array(child_data2, RTE_TEL_U64_VAL);
 
-	memset(&response_data, 0, sizeof(response_data));
 	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
 
 	for (i = 0; i < 5; i++) {
@@ -328,7 +344,7 @@ test_array_with_array_u64_values(void)
 	rte_tel_data_add_array_container(&response_data, child_data, 0);
 	rte_tel_data_add_array_container(&response_data, child_data2, 0);
 
-	return TEST_OUTPUT("{\"/test\":[[0,1,2,3,4],[0,1,2,3,4]]}");
+	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
 static int
@@ -369,7 +385,7 @@ connect_to_socket(void)
 }
 
 static int
-test_telemetry_data(void)
+telemetry_data_autotest(void)
 {
 	typedef int (*test_case)(void);
 	unsigned int i = 0;
@@ -378,7 +394,9 @@ test_telemetry_data(void)
 	if (sock <= 0)
 		return -1;
 
-	test_case test_cases[] = {test_case_array_string,
+	test_case test_cases[] = {
+			test_simple_string,
+			test_case_array_string,
 			test_case_array_int, test_case_array_u64,
 			test_case_add_dict_int, test_case_add_dict_u64,
 			test_case_add_dict_string,
@@ -390,8 +408,9 @@ test_telemetry_data(void)
 			test_array_with_array_u64_values,
 			test_array_with_array_string_values };
 
-	rte_telemetry_register_cmd(REQUEST_CMD, test_cb, "Test");
+	rte_telemetry_register_cmd(REQUEST_CMD, telemetry_test_cb, "Test");
 	for (i = 0; i < RTE_DIM(test_cases); i++) {
+		memset(&response_data, 0, sizeof(response_data));
 		if (test_cases[i]() != 0) {
 			close(sock);
 			return -1;
@@ -401,4 +420,4 @@ test_telemetry_data(void)
 	return 0;
 }
 
-REGISTER_TEST_COMMAND(telemetry_data_autotest, test_telemetry_data);
+REGISTER_TEST_COMMAND(telemetry_data_autotest, telemetry_data_autotest);
-- 
2.34.1

[PATCH v3 11/13] test/telemetry_data: add test cases for character escaping

[Bruce Richardson]

Add in some basic unit tests to validate the character escaping being
done on string data values, which tests end-to-end processing of those
values beyond just the json-encoding steps tested by the
"telemetry_json_autotest".

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_data.c | 30 +++++++++++++++++++++++++++++-
 1 file changed, 29 insertions(+), 1 deletion(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 9d174dc99f..69ca8b6c6f 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -347,6 +347,30 @@ test_array_with_array_u64_values(void)
 	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
+static int
+test_string_char_escaping(void)
+{
+	rte_tel_data_string(&response_data, "hello,\nworld\n");
+	return CHECK_OUTPUT("\"hello,\\nworld\\n\"");
+}
+
+static int
+test_array_char_escaping(void)
+{
+	rte_tel_data_start_array(&response_data, RTE_TEL_STRING_VAL);
+	rte_tel_data_add_array_string(&response_data, "\\escape\r");
+	rte_tel_data_add_array_string(&response_data, "characters\n");
+	return CHECK_OUTPUT("[\"\\\\escape\\r\",\"characters\\n\"]");
+}
+
+static int
+test_dict_char_escaping(void)
+{
+	rte_tel_data_start_dict(&response_data);
+	rte_tel_data_add_dict_string(&response_data, "name", "escaped\n\tvalue");
+	return CHECK_OUTPUT("{\"name\":\"escaped\\n\\tvalue\"}");
+}
+
 static int
 connect_to_socket(void)
 {
@@ -406,7 +430,11 @@ telemetry_data_autotest(void)
 			test_dict_with_dict_values,
 			test_array_with_array_int_values,
 			test_array_with_array_u64_values,
-			test_array_with_array_string_values };
+			test_array_with_array_string_values,
+			test_string_char_escaping,
+			test_array_char_escaping,
+			test_dict_char_escaping,
+	};
 
 	rte_telemetry_register_cmd(REQUEST_CMD, telemetry_test_cb, "Test");
 	for (i = 0; i < RTE_DIM(test_cases); i++) {
-- 
2.34.1

[PATCH v3 12/13] telemetry: eliminate duplicate code for json output

[Bruce Richardson]

When preparing the json response to a telemetry socket query, the code
for prefixing the command name, and appending the file "}" on the end of
the response was duplicated for multiple reply types. Taking this code
out of the switch statement reduces the duplication and makes the code
more maintainable.

For completeness of testing, add in a test case to validate the "null"
response type - the only leg of the switch statment not already covered
by an existing test case in the telemetry_data tests.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 app/test/test_telemetry_data.c |  7 +++++++
 lib/telemetry/telemetry.c      | 35 ++++++++++++----------------------
 2 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index 69ca8b6c6f..d92667a527 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -93,6 +93,12 @@ check_output(const char *func_name, const char *expected)
 	return strncmp(expected, buf, sizeof(buf));
 }
 
+static int
+test_null_return(void)
+{
+	return CHECK_OUTPUT("null");
+}
+
 static int
 test_simple_string(void)
 {
@@ -419,6 +425,7 @@ telemetry_data_autotest(void)
 		return -1;
 
 	test_case test_cases[] = {
+			test_null_return,
 			test_simple_string,
 			test_case_array_string,
 			test_case_array_int, test_case_array_u64,
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 03651e947d..cf60d27bd4 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -233,27 +233,22 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 
 	RTE_BUILD_BUG_ON(sizeof(out_buf) < MAX_CMD_LEN +
 			RTE_TEL_MAX_SINGLE_STRING_LEN + 10);
+
+	prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
+			MAX_CMD_LEN, cmd);
+	cb_data_buf = &out_buf[prefix_used];
+	buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
+
 	switch (d->type) {
 	case RTE_TEL_NULL:
-		used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":null}",
-				MAX_CMD_LEN, cmd ? cmd : "none");
+		used = strlcpy(cb_data_buf, "null", buf_len);
 		break;
-	case RTE_TEL_STRING:
-		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-				MAX_CMD_LEN, cmd);
-		cb_data_buf = &out_buf[prefix_used];
-		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
 
+	case RTE_TEL_STRING:
 		used = rte_tel_json_str(cb_data_buf, buf_len, 0, d->data.str);
-		used += prefix_used;
-		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
-	case RTE_TEL_DICT:
-		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-				MAX_CMD_LEN, cmd);
-		cb_data_buf = &out_buf[prefix_used];
-		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
 
+	case RTE_TEL_DICT:
 		used = rte_tel_json_empty_obj(cb_data_buf, buf_len, 0);
 		for (i = 0; i < d->data_len; i++) {
 			const struct tel_dict_entry *v = &d->data.dict[i];
@@ -289,18 +284,12 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 			}
 			}
 		}
-		used += prefix_used;
-		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
+
 	case RTE_TEL_ARRAY_STRING:
 	case RTE_TEL_ARRAY_INT:
 	case RTE_TEL_ARRAY_U64:
 	case RTE_TEL_ARRAY_CONTAINER:
-		prefix_used = snprintf(out_buf, sizeof(out_buf), "{\"%.*s\":",
-				MAX_CMD_LEN, cmd);
-		cb_data_buf = &out_buf[prefix_used];
-		buf_len = sizeof(out_buf) - prefix_used - 1; /* space for '}' */
-
 		used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
 		for (i = 0; i < d->data_len; i++)
 			if (d->type == RTE_TEL_ARRAY_STRING)
@@ -328,10 +317,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				if (!rec_data->keep)
 					rte_tel_data_free(rec_data->data);
 			}
-		used += prefix_used;
-		used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 		break;
 	}
+	used += prefix_used;
+	used += strlcat(out_buf + used, "}", sizeof(out_buf) - used);
 	if (write(s, out_buf, used) < 0)
 		perror("Error writing to socket");
 }
-- 
2.34.1

[PATCH v3 13/13] telemetry: make help command more helpful

[Bruce Richardson]

The /help telemetry command prints out the help text for the given
command passed in as parameter. However, entering /help without any
parameters does not give any useful information as to the fact that you
need to pass in a command to get help on. Update the command so it
prints its own help text when called without any parameters.

Signed-off-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
---
 lib/telemetry/telemetry.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index cf60d27bd4..09febff0ae 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -139,15 +139,17 @@ command_help(const char *cmd __rte_unused, const char *params,
 		struct rte_tel_data *d)
 {
 	int i;
+	/* if no parameters return our own help text */
+	const char *to_lookup = (params == NULL ? cmd : params);
 
-	if (!params)
-		return -1;
 	rte_tel_data_start_dict(d);
 	rte_spinlock_lock(&callback_sl);
 	for (i = 0; i < num_callbacks; i++)
-		if (strcmp(params, callbacks[i].cmd) == 0) {
-			rte_tel_data_add_dict_string(d, params,
-					callbacks[i].help);
+		if (strcmp(to_lookup, callbacks[i].cmd) == 0) {
+			if (params == NULL)
+				rte_tel_data_string(d, callbacks[i].help);
+			else
+				rte_tel_data_add_dict_string(d, params,	callbacks[i].help);
 			break;
 		}
 	rte_spinlock_unlock(&callback_sl);
-- 
2.34.1

Re: [PATCH v3 00/13] telemetry JSON escaping and other enhancements

[fengchengwen]

Series-acked-by: Chengwen Feng <fengchengwen@huawei.com>

On 2022/9/9 17:35, Bruce Richardson wrote:
> This patchset contains fixes for the problem of handling characters returned by
> telemetry callbacks which require escaping when encoded in JSON format. It also
> includes unit tests to validate the correct encoding in such scenarios and a
> number of smaller enhancements to telemetry and telemetry testing
> 
> V3:
> * reorder patchset to put previous patch 6 (dict name limiting) at the
>   start of the series
> * fix missed rename of TEST_OUTPUT to CHECK_OUTPUT in patch 10
> 

...

Re: [PATCH v3 00/13] telemetry JSON escaping and other enhancements

[David Marchand]

On Fri, Sep 9, 2022 at 11:36 AM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> This patchset contains fixes for the problem of handling characters returned by
> telemetry callbacks which require escaping when encoded in JSON format. It also
> includes unit tests to validate the correct encoding in such scenarios and a
> number of smaller enhancements to telemetry and telemetry testing
>
> V3:
> * reorder patchset to put previous patch 6 (dict name limiting) at the
>   start of the series
> * fix missed rename of TEST_OUTPUT to CHECK_OUTPUT in patch 10
>
> RFC->V2:
> * limited characters allowed in dictionary element names and command
>   names to side-step the encoding problems there
> * added support for proper escaping of dictionary string values
> * added more testing and test cases
> * added other misc telemetry cleanups and refactoring
>
> Bruce Richardson (13):
>   telemetry: limit characters allowed in dictionary names
>   test/telemetry_json: print success or failure per subtest
>   telemetry: fix escaping of invalid json characters
>   test/telemetry_json: add test for string character escaping
>   telemetry: add escaping of strings in arrays
>   test/telemetry-json: add test for escaping strings in arrays
>   telemetry: add escaping of strings in dicts
>   test/telemetry_json: add test for string escaping in objects
>   telemetry: limit command characters
>   test/telemetry_data: refactor for maintainability
>   test/telemetry_data: add test cases for character escaping
>   telemetry: eliminate duplicate code for json output
>   telemetry: make help command more helpful
>
>  app/test/test_telemetry_data.c       | 138 +++++++++++++++++++--------
>  app/test/test_telemetry_json.c       |  98 +++++++++++++++++--
>  doc/guides/rel_notes/deprecation.rst |   8 --
>  lib/telemetry/rte_telemetry.h        |   8 ++
>  lib/telemetry/telemetry.c            |  51 +++++-----
>  lib/telemetry/telemetry_data.c       |  32 +++++++
>  lib/telemetry/telemetry_json.h       |  72 ++++++++++++--
>  7 files changed, 318 insertions(+), 89 deletions(-)

Updated release notes, and applied, thanks.


-- 
David Marchand