[PATCH] trace: take live traces via telemetry

[PATCH] trace: take live traces via telemetry

[David Marchand]

Register telemetry commands to list and configure trace points and later
save traces for a running DPDK application.

Note: trace point names contain a '.', so the list of valid characters
used in telemetry commands and dictionary keys has been extended.

Example with testpmd running with two net/null ports (startup command
from devtools/test-null.sh):

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": "Disabled",
    "lib.ethdev.rxq.setup": "Disabled",
    "lib.ethdev.txq.setup": "Disabled",
    "lib.ethdev.start": "Disabled",
    "lib.ethdev.stop": "Disabled",
    "lib.ethdev.close": "Disabled",
    "lib.ethdev.rx.burst": "Disabled",
    "lib.ethdev.tx.burst": "Disabled"}}

--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 2}}
--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 0}}

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": "Disabled",
    "lib.ethdev.rxq.setup": "Disabled",
    "lib.ethdev.txq.setup": "Disabled",
    "lib.ethdev.start": "Enabled",
    "lib.ethdev.stop": "Enabled",
    "lib.ethdev.close": "Disabled",
    "lib.ethdev.rx.burst": "Disabled",
    "lib.ethdev.tx.burst": "Disabled"}}

testpmd> stop
...
testpmd> port stop all
...
testpmd> port start all
...
testpmd> start
...

--> /trace/save
{"/trace/save": {"Status": "OK",
    "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}

$ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
[10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
[10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
[10:51:40.449359774] (+4.219479523) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
[10:51:40.449377877] (+0.000018103) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }

--> /trace/disable,*
{"/trace/disable": {"Count": 2}}

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
For runtime testing, please use this patch in addition with series 25183.
Depends-on: series-25183 ("Trace subsystem fixes")

---
 lib/eal/common/eal_common_trace.c | 78 +++++++++++++++++++++++++++++++
 lib/telemetry/telemetry_data.c    |  1 +
 2 files changed, 79 insertions(+)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index f9b187d15f..9a54987b42 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -12,6 +12,7 @@
 #include <rte_lcore.h>
 #include <rte_per_lcore.h>
 #include <rte_string_fns.h>
+#include <rte_telemetry.h>
 
 #include "eal_trace.h"
 
@@ -530,3 +531,80 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	return -rte_errno;
 }
+
+static int
+trace_telemetry_enable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, true);
+	rte_tel_data_add_dict_int(d, "Count",
+		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);
+	return 0;
+
+}
+
+static int
+trace_telemetry_disable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, false);
+	rte_tel_data_add_dict_int(d, "Count",
+		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
+	return 0;
+
+}
+
+static int
+trace_telemetry_list(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	struct trace_point *tp;
+
+	rte_tel_data_start_dict(d);
+	STAILQ_FOREACH(tp, &tp_list, next) {
+		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
+			continue;
+
+		rte_tel_data_add_dict_string(d, tp->name,
+			rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");
+	}
+
+	return 0;
+}
+
+static int
+trace_telemetry_save(const char *cmd __rte_unused,
+	const char *params __rte_unused, struct rte_tel_data *d)
+{
+	struct trace *trace = trace_obj_get();
+
+	rte_tel_data_start_dict(d);
+	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
+	rte_tel_data_add_dict_string(d, "Path", trace->dir);
+
+	return 0;
+}
+
+RTE_INIT(trace_telemetry)
+{
+	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
+		"Enable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
+		"Disable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
+		"List trace points. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
+		"Save current traces. Takes no parameter.");
+}
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 34366ecee3..5b319c18fb 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -106,6 +106,7 @@ valid_name(const char *name)
 			['a' ... 'z'] = 1,
 			['_'] = 1,
 			['/'] = 1,
+			['.'] = 1,
 	};
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
-- 
2.37.3

Re: [PATCH] trace: take live traces via telemetry

[Jerin Jacob]

On Thu, Oct 13, 2022 at 1:20 PM David Marchand
<david.marchand@redhat.com> wrote:
>

I would suggest to change the subject as "trace: enable trace
operations via telemetry" or so

> Register telemetry commands to list and configure trace points and later
> save traces for a running DPDK application.
>
> Note: trace point names contain a '.', so the list of valid characters
> used in telemetry commands and dictionary keys has been extended.
>
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
>
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>

> +}
> +
> +static int
> +trace_telemetry_list(const char *cmd __rte_unused,
> +       const char *params, struct rte_tel_data *d)
> +{
> +       struct trace_point *tp;
> +
> +       rte_tel_data_start_dict(d);
> +       STAILQ_FOREACH(tp, &tp_list, next) {
> +               if (params != NULL && fnmatch(params, tp->name, 0) != 0)
> +                       continue;
> +
> +               rte_tel_data_add_dict_string(d, tp->name,
> +                       rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");

Could be changed to "Ena" and "Dis" or similar to reduce traffic on wire.

Also, it may be good to add a few text in
doc/guides/prog_guide/trace_lib.rst to tell this feature.

Acked-by: Jerin Jacob <jerinj@marvell.com>

Re: [PATCH] trace: take live traces via telemetry

[Bruce Richardson]

On Thu, Oct 13, 2022 at 09:49:28AM +0200, David Marchand wrote:
> Register telemetry commands to list and configure trace points and later
> save traces for a running DPDK application.
> 
> Note: trace point names contain a '.', so the list of valid characters
> used in telemetry commands and dictionary keys has been extended.
> 
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Disabled",
>     "lib.ethdev.stop": "Disabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}
> 
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 2}}
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 0}}
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Enabled",
>     "lib.ethdev.stop": "Enabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}
> 
> testpmd> stop
> ...
> testpmd> port stop all
> ...
> testpmd> port start all
> ...
> testpmd> start
> ...
> 
> --> /trace/save
> {"/trace/save": {"Status": "OK",
>     "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
> 
> $ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
> [10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
> [10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
> [10:51:40.449359774] (+4.219479523) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
> [10:51:40.449377877] (+0.000018103) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }
> 
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> For runtime testing, please use this patch in addition with series 25183.
> Depends-on: series-25183 ("Trace subsystem fixes")
> 
> ---
>  lib/eal/common/eal_common_trace.c | 78 +++++++++++++++++++++++++++++++
>  lib/telemetry/telemetry_data.c    |  1 +
>  2 files changed, 79 insertions(+)
> 
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index f9b187d15f..9a54987b42 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -12,6 +12,7 @@
>  #include <rte_lcore.h>
>  #include <rte_per_lcore.h>
>  #include <rte_string_fns.h>
> +#include <rte_telemetry.h>
>  
>  #include "eal_trace.h"
>  
> @@ -530,3 +531,80 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>  
>  	return -rte_errno;
>  }
> +
> +static int
> +trace_telemetry_enable(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	unsigned int count;
> +
> +	if (params == NULL || strlen(params) == 0)
> +		return -1;
> +	rte_tel_data_start_dict(d);
> +	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
> +	rte_trace_pattern(params, true);
> +	rte_tel_data_add_dict_int(d, "Count",
> +		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);
> +	return 0;
> +
> +}
> +
> +static int
> +trace_telemetry_disable(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	unsigned int count;
> +
> +	if (params == NULL || strlen(params) == 0)
> +		return -1;
> +	rte_tel_data_start_dict(d);
> +	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
> +	rte_trace_pattern(params, false);
> +	rte_tel_data_add_dict_int(d, "Count",
> +		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
> +	return 0;
> +
> +}
> +
> +static int
> +trace_telemetry_list(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	struct trace_point *tp;
> +
> +	rte_tel_data_start_dict(d);
> +	STAILQ_FOREACH(tp, &tp_list, next) {
> +		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
> +			continue;
> +
> +		rte_tel_data_add_dict_string(d, tp->name,
> +			rte_trace_point_is_enabled(tp->handle) ?  "Enabled" : "Disabled");
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +trace_telemetry_save(const char *cmd __rte_unused,
> +	const char *params __rte_unused, struct rte_tel_data *d)
> +{
> +	struct trace *trace = trace_obj_get();
> +
> +	rte_tel_data_start_dict(d);
> +	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
> +	rte_tel_data_add_dict_string(d, "Path", trace->dir);
> +
> +	return 0;
> +}
> +
> +RTE_INIT(trace_telemetry)
> +{
> +	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
> +		"Enable trace points matching the provided pattern. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
> +		"Disable trace points matching the provided pattern. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
> +		"List trace points. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
> +		"Save current traces. Takes no parameter.");
> +}
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 34366ecee3..5b319c18fb 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -106,6 +106,7 @@ valid_name(const char *name)
>  			['a' ... 'z'] = 1,
>  			['_'] = 1,
>  			['/'] = 1,
> +			['.'] = 1,
>  	};
>  	while (*name != '\0') {
>  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)

I don't see an issue with allowing "." characters in dictionary names, so
for this part:

Acked-by: Bruce Richardson <bruce.richardson@intel.com>

Re: [PATCH] trace: take live traces via telemetry

[Bruce Richardson]

On Tue, Oct 18, 2022 at 02:14:37PM +0100, Bruce Richardson wrote:
> On Thu, Oct 13, 2022 at 09:49:28AM +0200, David Marchand wrote:
> > Register telemetry commands to list and configure trace points and later
> > save traces for a running DPDK application.
> > 
> > Note: trace point names contain a '.', so the list of valid characters
> > used in telemetry commands and dictionary keys has been extended.
> > 
<snip>

> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 34366ecee3..5b319c18fb 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -106,6 +106,7 @@ valid_name(const char *name)
> >  			['a' ... 'z'] = 1,
> >  			['_'] = 1,
> >  			['/'] = 1,
> > +			['.'] = 1,
> >  	};
> >  	while (*name != '\0') {
> >  		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> 
> I don't see an issue with allowing "." characters in dictionary names, so
> for this part:
> 
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>

By way of additional minor follow-up:
* I think the addition of "." to the list of allowed characters should be a
  separate patch, rather than just added as part of this larger patch. If
  doing a separate commit to add boolean type, this could be a change in that
  set.
* There are a couple of API doxygen comments for the dictionary functions,
  rte_tel_add_dict_*, where the allowed characters in the name are called
  out. You probably should add "." to those comments too.

Regards,
/Bruce

Re: [PATCH] trace: take live traces via telemetry

[David Marchand]

On Wed, Oct 19, 2022 at 12:53 PM Bruce Richardson
<bruce.richardson@intel.com> wrote:
>
> On Tue, Oct 18, 2022 at 02:14:37PM +0100, Bruce Richardson wrote:
> > On Thu, Oct 13, 2022 at 09:49:28AM +0200, David Marchand wrote:
> > > Register telemetry commands to list and configure trace points and later
> > > save traces for a running DPDK application.
> > >
> > > Note: trace point names contain a '.', so the list of valid characters
> > > used in telemetry commands and dictionary keys has been extended.
> > >
> <snip>
>
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index 34366ecee3..5b319c18fb 100644
> > > --- a/lib/telemetry/telemetry_data.c
> > > +++ b/lib/telemetry/telemetry_data.c
> > > @@ -106,6 +106,7 @@ valid_name(const char *name)
> > >                     ['a' ... 'z'] = 1,
> > >                     ['_'] = 1,
> > >                     ['/'] = 1,
> > > +                   ['.'] = 1,
> > >     };
> > >     while (*name != '\0') {
> > >             if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
> >
> > I don't see an issue with allowing "." characters in dictionary names, so
> > for this part:
> >
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
>
> By way of additional minor follow-up:
> * I think the addition of "." to the list of allowed characters should be a
>   separate patch, rather than just added as part of this larger patch. If
>   doing a separate commit to add boolean type, this could be a change in that
>   set.
> * There are a couple of API doxygen comments for the dictionary functions,
>   rte_tel_add_dict_*, where the allowed characters in the name are called
>   out. You probably should add "." to those comments too.

Oh indeed, so ok let's go with a separate patch.

Previously, I thought the telemetry additions for traces were fine to
go with the fixes series.
But seeing how it evolved, I'll merge the traces fixes (who got acked)
now and respin a separate series for the rest.


-- 
David Marchand

RE: [PATCH] trace: take live traces via telemetry

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Thursday, 13 October 2022 09.49
> 
> Register telemetry commands to list and configure trace points and
> later
> save traces for a running DPDK application.
> 
> Note: trace point names contain a '.', so the list of valid characters
> used in telemetry commands and dictionary keys has been extended.

Regarding '.' in telemetry commands and dictionary keys, I agree with Bruce to allow it.

> 
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Disabled",
>     "lib.ethdev.stop": "Disabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}

Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.

It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.

Alternatively, use integer values 0 or 1.

If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

> 
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 2}}
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 0}}
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": "Disabled",
>     "lib.ethdev.rxq.setup": "Disabled",
>     "lib.ethdev.txq.setup": "Disabled",
>     "lib.ethdev.start": "Enabled",
>     "lib.ethdev.stop": "Enabled",
>     "lib.ethdev.close": "Disabled",
>     "lib.ethdev.rx.burst": "Disabled",
>     "lib.ethdev.tx.burst": "Disabled"}}
> 
> testpmd> stop
> ...
> testpmd> port stop all
> ...
> testpmd> port start all
> ...
> testpmd> start
> ...
> 
> --> /trace/save
> {"/trace/save": {"Status": "OK",
>     "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
> 
> $ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
> [10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
> [10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
> [10:51:40.449359774] (+4.219479523) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
> [10:51:40.449377877] (+0.000018103) lib.ethdev.start:
>     { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }
> 
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---

Re: [PATCH] trace: take live traces via telemetry

[Bruce Richardson]

On Tue, Oct 18, 2022 at 04:33:49PM +0200, Morten Brørup wrote:
> > From: David Marchand [mailto:david.marchand@redhat.com]
> > Sent: Thursday, 13 October 2022 09.49
> > 
> > Register telemetry commands to list and configure trace points and
> > later
> > save traces for a running DPDK application.
> > 
> > Note: trace point names contain a '.', so the list of valid characters
> > used in telemetry commands and dictionary keys has been extended.
> 
> Regarding '.' in telemetry commands and dictionary keys, I agree with Bruce to allow it.
> 
> > 
> > Example with testpmd running with two net/null ports (startup command
> > from devtools/test-null.sh):
> > 
> > --> /trace/list,lib.ethdev.*
> > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> >     "lib.ethdev.rxq.setup": "Disabled",
> >     "lib.ethdev.txq.setup": "Disabled",
> >     "lib.ethdev.start": "Disabled",
> >     "lib.ethdev.stop": "Disabled",
> >     "lib.ethdev.close": "Disabled",
> >     "lib.ethdev.rx.burst": "Disabled",
> >     "lib.ethdev.tx.burst": "Disabled"}}
> 
> Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.
> 
> It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.
> 
> Alternatively, use integer values 0 or 1.
> 
> If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

+1 for integer values (at least in the short term). I don't think we want
these as strings.

/Bruce

Re: [PATCH] trace: take live traces via telemetry

[David Marchand]

Hello Morten, Bruce,

On Tue, Oct 18, 2022 at 4:34 PM Morten Brørup <mb@smartsharesystems.com> wrote:
> > --> /trace/list,lib.ethdev.*
> > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> >     "lib.ethdev.rxq.setup": "Disabled",
> >     "lib.ethdev.txq.setup": "Disabled",
> >     "lib.ethdev.start": "Disabled",
> >     "lib.ethdev.stop": "Disabled",
> >     "lib.ethdev.close": "Disabled",
> >     "lib.ethdev.rx.burst": "Disabled",
> >     "lib.ethdev.tx.burst": "Disabled"}}
>
> Jerin commented that "Disabled"/"Enabled" are a bit verbose, and suggested shortening them.

I forgot to handle this part (EINTR...), thanks for reminding.


> It seems to me that these values are Boolean, and should be true or false (not surrounded by quotation marks), instead of some string representing a Boolean value. Note: This would require expanding the telemetry library with a Boolean type.

Indeed.

>
> Alternatively, use integer values 0 or 1.
>
> If we want to represent Boolean values as strings, I vote for "TRUE" and "FALSE", using all upper case to indicate that they are magic strings - and also to help avoid confusion with the JSON true/false Boolean values, which are all lower case.

Introducing those strings is confusing, especially if we later
introduce the boolean type.
I had a quick try and adding the boolean type is relatively easy (I
just posted a patch, see
https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-david.marchand@redhat.com/).

I would either go with adding this new type (though we are past rc1,
this addition is self contained and low risk), or use simple integers.


-- 
David Marchand

RE: [PATCH] trace: take live traces via telemetry

[Morten Brørup]

> From: David Marchand [mailto:david.marchand@redhat.com]
> Sent: Wednesday, 19 October 2022 09.39
> 
> Hello Morten, Bruce,
> 
> On Tue, Oct 18, 2022 at 4:34 PM Morten Brørup
> <mb@smartsharesystems.com> wrote:
> > > --> /trace/list,lib.ethdev.*
> > > {"/trace/list": {"lib.ethdev.configure": "Disabled",
> > >     "lib.ethdev.rxq.setup": "Disabled",
> > >     "lib.ethdev.txq.setup": "Disabled",
> > >     "lib.ethdev.start": "Disabled",
> > >     "lib.ethdev.stop": "Disabled",
> > >     "lib.ethdev.close": "Disabled",
> > >     "lib.ethdev.rx.burst": "Disabled",
> > >     "lib.ethdev.tx.burst": "Disabled"}}
> >
> > Jerin commented that "Disabled"/"Enabled" are a bit verbose, and
> suggested shortening them.
> 
> I forgot to handle this part (EINTR...), thanks for reminding.
> 
> 
> > It seems to me that these values are Boolean, and should be true or
> false (not surrounded by quotation marks), instead of some string
> representing a Boolean value. Note: This would require expanding the
> telemetry library with a Boolean type.
> 
> Indeed.
> 
> >
> > Alternatively, use integer values 0 or 1.
> >
> > If we want to represent Boolean values as strings, I vote for "TRUE"
> and "FALSE", using all upper case to indicate that they are magic
> strings - and also to help avoid confusion with the JSON true/false
> Boolean values, which are all lower case.
> 
> Introducing those strings is confusing, especially if we later
> introduce the boolean type.
> I had a quick try and adding the boolean type is relatively easy (I
> just posted a patch, see
> https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-
> david.marchand@redhat.com/).
> 
> I would either go with adding this new type (though we are past rc1,
> this addition is self contained and low risk), or use simple integers.

+1 to adding this new type.

It might be wider ranging, though:

Are other existing telemetry data in fact Boolean, but currently using some other type, and should switch to using the new Boolean type too?

> 
> 
> --
> David Marchand
> 

Re: [PATCH] trace: take live traces via telemetry

[David Marchand]

On Wed, Oct 19, 2022 at 10:21 AM Morten Brørup <mb@smartsharesystems.com> wrote:
> > > It seems to me that these values are Boolean, and should be true or
> > false (not surrounded by quotation marks), instead of some string
> > representing a Boolean value. Note: This would require expanding the
> > telemetry library with a Boolean type.
> >
> > Indeed.
> >
> > >
> > > Alternatively, use integer values 0 or 1.
> > >
> > > If we want to represent Boolean values as strings, I vote for "TRUE"
> > and "FALSE", using all upper case to indicate that they are magic
> > strings - and also to help avoid confusion with the JSON true/false
> > Boolean values, which are all lower case.
> >
> > Introducing those strings is confusing, especially if we later
> > introduce the boolean type.
> > I had a quick try and adding the boolean type is relatively easy (I
> > just posted a patch, see
> > https://patchwork.dpdk.org/project/dpdk/patch/20221019073702.3948624-1-
> > david.marchand@redhat.com/).
> >
> > I would either go with adding this new type (though we are past rc1,
> > this addition is self contained and low risk), or use simple integers.
>
> +1 to adding this new type.
>
> It might be wider ranging, though:
>
> Are other existing telemetry data in fact Boolean, but currently using some other type, and should switch to using the new Boolean type too?

I wondered about the same :-) but I did not check, for now.


-- 
David marchand

[PATCH v3 0/4] Telemetry support for traces

[David Marchand]

This series is a continuation of the traces fixes series that is now
merged.


-- 
David Marchand

Changes since v1/v2:
- after some back and forth, the original patch has been split so that
  telemetry changes are separate and to introduce the new rte_trace_save()
  behavior,

David Marchand (4):
  telemetry: support boolean type
  telemetry: extend valid command characters
  trace: enable trace operations via telemetry
  trace: create new directory for each trace dump

 app/test/test_telemetry_data.c          | 88 +++++++++++++++++++++++-
 doc/guides/prog_guide/trace_lib.rst     | 39 +++++++++++
 lib/eal/common/eal_common_trace.c       | 82 +++++++++++++++++++++++
 lib/eal/common/eal_common_trace_utils.c | 89 +++++++++++--------------
 lib/eal/common/eal_trace.h              |  1 +
 lib/telemetry/rte_telemetry.h           | 46 +++++++++++--
 lib/telemetry/telemetry.c               | 24 ++++++-
 lib/telemetry/telemetry_data.c          | 45 +++++++++++--
 lib/telemetry/telemetry_data.h          |  5 ++
 lib/telemetry/telemetry_json.h          | 34 ++++++++++
 lib/telemetry/version.map               | 10 +++
 11 files changed, 401 insertions(+), 62 deletions(-)

-- 
2.37.3

[PATCH v3 1/4] telemetry: support boolean type

[David Marchand]

Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Morten Brørup <mb@smartsharesystems.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
Acked-by: Ciara Power <ciara.power@intel.com>
---
Changes since v1:
- fixed doxygen description,

---
 app/test/test_telemetry_data.c | 88 +++++++++++++++++++++++++++++++++-
 lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++
 lib/telemetry/telemetry.c      | 24 +++++++++-
 lib/telemetry/telemetry_data.c | 44 +++++++++++++++--
 lib/telemetry/telemetry_data.h |  5 ++
 lib/telemetry/telemetry_json.h | 34 +++++++++++++
 lib/telemetry/version.map      | 10 ++++
 7 files changed, 233 insertions(+), 8 deletions(-)

diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
index d92667a527..134e018fde 100644
--- a/app/test/test_telemetry_data.c
+++ b/app/test/test_telemetry_data.c
@@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
 	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
 }
 
+static int
+test_case_array_bool(void)
+{
+	int i;
+
+	rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
+	for (i = 0; i < 5; i++)
+		rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
+	return CHECK_OUTPUT("[true,false,true,false,true]");
+}
+
+static int
+test_case_add_dict_bool(void)
+{
+	int i = 0;
+	char name_of_value[8];
+
+	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_bool(&response_data, name_of_value,
+			(i % 2) == 0);
+	}
+	return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
+		"\"dict_3\":false,\"dict_4\":true}");
+}
+
+static int
+test_dict_with_array_bool_values(void)
+{
+	int i;
+
+	struct rte_tel_data *child_data = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
+
+	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
+
+	rte_tel_data_start_dict(&response_data);
+
+	for (i = 0; i < 10; i++) {
+		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
+		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
+	}
+
+	rte_tel_data_add_dict_container(&response_data, "dict_0",
+	 child_data, 0);
+	rte_tel_data_add_dict_container(&response_data, "dict_1",
+	 child_data2, 0);
+
+	return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
+		"\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
+}
+
+static int
+test_array_with_array_bool_values(void)
+{
+	int i;
+
+	struct rte_tel_data *child_data = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
+
+	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
+	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
+
+	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
+
+	for (i = 0; i < 5; i++) {
+		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
+		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
+	}
+	rte_tel_data_add_array_container(&response_data, child_data, 0);
+	rte_tel_data_add_array_container(&response_data, child_data2, 0);
+
+	return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
+}
+
 static int
 test_string_char_escaping(void)
 {
@@ -428,15 +506,21 @@ telemetry_data_autotest(void)
 			test_null_return,
 			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_array_int,
+			test_case_array_u64,
+			test_case_array_bool,
+			test_case_add_dict_int,
+			test_case_add_dict_u64,
+			test_case_add_dict_bool,
 			test_case_add_dict_string,
 			test_dict_with_array_int_values,
 			test_dict_with_array_u64_values,
+			test_dict_with_array_bool_values,
 			test_dict_with_array_string_values,
 			test_dict_with_dict_values,
 			test_array_with_array_int_values,
 			test_array_with_array_u64_values,
+			test_array_with_array_bool_values,
 			test_array_with_array_string_values,
 			test_string_char_escaping,
 			test_array_char_escaping,
diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index a0d21d6b7f..e7f6c2ae43 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -2,6 +2,7 @@
  * Copyright(c) 2018 Intel Corporation
  */
 
+#include <stdbool.h>
 #include <stdint.h>
 
 #include <rte_compat.h>
@@ -46,6 +47,7 @@ enum rte_tel_value_type {
 	RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
 	RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
 	RTE_TEL_CONTAINER, /** a container struct */
+	RTE_TEL_BOOL_VAL,   /** a boolean value */
 };
 
 /**
@@ -155,6 +157,22 @@ int
 rte_tel_data_add_array_container(struct rte_tel_data *d,
 		struct rte_tel_data *val, int keep);
 
+/**
+ * Add a boolean to an array.
+ * The array must have been started by rte_tel_data_start_array() with
+ * RTE_TEL_BOOL_VAL as the type parameter.
+ *
+ * @param d
+ *   The data structure passed to the callback
+ * @param x
+ *   The boolean value to be returned in the array
+ * @return
+ *   0 on success, negative errno on error
+ */
+__rte_experimental
+int
+rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
+
 /**
  * Add a string value to a dictionary.
  * The dict must have been started by rte_tel_data_start_dict().
@@ -233,6 +251,24 @@ int
 rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 		struct rte_tel_data *val, int keep);
 
+/**
+ * Add a boolean value to a dictionary.
+ * The dict must have been started by rte_tel_data_start_dict().
+ *
+ * @param 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 boolean value to be stored in the dict
+ * @return
+ *   0 on success, negative errno on error, E2BIG on string truncation of name.
+ */
+__rte_experimental
+int
+rte_tel_data_add_dict_bool(struct rte_tel_data *d, const char *name, bool val);
+
 /**
  * This telemetry callback is used when registering a telemetry command.
  * It handles getting and formatting information to be returned to telemetry
diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
index 8fbb4f3060..276d0f337d 100644
--- a/lib/telemetry/telemetry.c
+++ b/lib/telemetry/telemetry.c
@@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 	unsigned int i;
 
 	if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
-		d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
+			d->type != RTE_TEL_ARRAY_INT &&
+			d->type != RTE_TEL_ARRAY_STRING &&
+			d->type != RTE_TEL_ARRAY_BOOL)
 		return snprintf(out_buf, buf_len, "null");
 
 	used = rte_tel_json_empty_array(out_buf, buf_len, 0);
@@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 			used = rte_tel_json_add_array_string(out_buf,
 				buf_len, used,
 				d->data.array[i].sval);
+	if (d->type == RTE_TEL_ARRAY_BOOL)
+		for (i = 0; i < d->data_len; i++)
+			used = rte_tel_json_add_array_bool(out_buf,
+				buf_len, used,
+				d->data.array[i].boolval);
 	if (d->type == RTE_TEL_DICT)
 		for (i = 0; i < d->data_len; i++) {
 			const struct tel_dict_entry *v = &d->data.dict[i];
@@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
 						buf_len, used,
 						v->name, v->value.u64val);
 				break;
+			case RTE_TEL_BOOL_VAL:
+				used = rte_tel_json_add_obj_bool(out_buf,
+						buf_len, used,
+						v->name, v->value.boolval);
+				break;
 			case RTE_TEL_CONTAINER:
 			{
 				char temp[buf_len];
@@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 						buf_len, used,
 						v->name, v->value.u64val);
 				break;
+			case RTE_TEL_BOOL_VAL:
+				used = rte_tel_json_add_obj_bool(cb_data_buf,
+						buf_len, used,
+						v->name, v->value.boolval);
+				break;
 			case RTE_TEL_CONTAINER:
 			{
 				char temp[buf_len];
@@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 	case RTE_TEL_ARRAY_STRING:
 	case RTE_TEL_ARRAY_INT:
 	case RTE_TEL_ARRAY_U64:
+	case RTE_TEL_ARRAY_BOOL:
 	case RTE_TEL_ARRAY_CONTAINER:
 		used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
 		for (i = 0; i < d->data_len; i++)
@@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
 				used = rte_tel_json_add_array_u64(cb_data_buf,
 						buf_len, used,
 						d->data.array[i].u64val);
+			else if (d->type == RTE_TEL_ARRAY_BOOL)
+				used = rte_tel_json_add_array_bool(cb_data_buf,
+						buf_len, used,
+						d->data.array[i].boolval);
 			else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
 				char temp[buf_len];
 				const struct container *rec_data =
diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
index 34366ecee3..13a7ce7034 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -16,10 +16,11 @@ int
 rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
 {
 	enum tel_container_types array_types[] = {
-			RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
-			RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
-			RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
-			RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
+		[RTE_TEL_STRING_VAL] = RTE_TEL_ARRAY_STRING,
+		[RTE_TEL_INT_VAL] = RTE_TEL_ARRAY_INT,
+		[RTE_TEL_U64_VAL] = RTE_TEL_ARRAY_U64,
+		[RTE_TEL_CONTAINER] = RTE_TEL_ARRAY_CONTAINER,
+		[RTE_TEL_BOOL_VAL] = RTE_TEL_ARRAY_BOOL,
 	};
 	d->type = array_types[type];
 	d->data_len = 0;
@@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
 	return 0;
 }
 
+int
+rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x)
+{
+	if (d->type != RTE_TEL_ARRAY_BOOL)
+		return -EINVAL;
+	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
+		return -ENOSPC;
+	d->data.array[d->data_len++].boolval = x;
+	return 0;
+}
+
 int
 rte_tel_data_add_array_container(struct rte_tel_data *d,
 		struct rte_tel_data *val, int keep)
@@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
 	if (d->type != RTE_TEL_ARRAY_CONTAINER ||
 			(val->type != RTE_TEL_ARRAY_U64
 			&& val->type != RTE_TEL_ARRAY_INT
-			&& val->type != RTE_TEL_ARRAY_STRING))
+			&& val->type != RTE_TEL_ARRAY_STRING
+			&& val->type != RTE_TEL_ARRAY_BOOL))
 		return -EINVAL;
 	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
 		return -ENOSPC;
@@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
 	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
 }
 
+int
+rte_tel_data_add_dict_bool(struct rte_tel_data *d,
+		const char *name, bool val)
+{
+	struct tel_dict_entry *e = &d->data.dict[d->data_len];
+	if (d->type != RTE_TEL_DICT)
+		return -EINVAL;
+	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
+		return -ENOSPC;
+
+	if (!valid_name(name))
+		return -EINVAL;
+
+	d->data_len++;
+	e->type = RTE_TEL_BOOL_VAL;
+	e->value.boolval = val;
+	const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
+	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
+}
+
 int
 rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 		struct rte_tel_data *val, int keep)
@@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
 	if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
 			&& val->type != RTE_TEL_ARRAY_INT
 			&& val->type != RTE_TEL_ARRAY_STRING
+			&& val->type != RTE_TEL_ARRAY_BOOL
 			&& val->type != RTE_TEL_DICT))
 		return -EINVAL;
 	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
index 26aa28e72c..c840486b18 100644
--- a/lib/telemetry/telemetry_data.h
+++ b/lib/telemetry/telemetry_data.h
@@ -5,6 +5,9 @@
 #ifndef _TELEMETRY_DATA_H_
 #define _TELEMETRY_DATA_H_
 
+#include <stdbool.h>
+#include <stdint.h>
+
 #include "rte_telemetry.h"
 
 enum tel_container_types {
@@ -15,6 +18,7 @@ enum tel_container_types {
 	RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
 	RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
 	RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
+	RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
 };
 
 struct container {
@@ -30,6 +34,7 @@ union tel_value {
 	char sval[RTE_TEL_MAX_STRING_LEN];
 	int ival;
 	uint64_t u64val;
+	bool boolval;
 	struct container container;
 };
 
diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
index e3fae7c30d..c97da97366 100644
--- a/lib/telemetry/telemetry_json.h
+++ b/lib/telemetry/telemetry_json.h
@@ -7,6 +7,7 @@
 
 #include <inttypes.h>
 #include <stdarg.h>
+#include <stdbool.h>
 #include <stdio.h>
 #include <rte_common.h>
 #include <rte_telemetry.h>
@@ -159,6 +160,21 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
 	return ret == 0 ? used : end + ret;
 }
 
+/* Appends a boolean into the JSON array in the provided buffer. */
+static inline int
+rte_tel_json_add_array_bool(char *buf, const int len, const int used,
+		bool val)
+{
+	int ret, end = used - 1; /* strip off final delimiter */
+	if (used <= 2) /* assume empty, since minimum is '[]' */
+		return __json_snprintf(buf, len, "[%s]",
+				val ? "true" : "false");
+
+	ret = __json_snprintf(buf + end, len - end, ",%s]",
+			val ? "true" : "false");
+	return ret == 0 ? used : end + ret;
+}
+
 /*
  * Add a new element with raw JSON value to the JSON array stored in the
  * provided buffer.
@@ -193,6 +209,24 @@ rte_tel_json_add_obj_u64(char *buf, const int len, const int used,
 	return ret == 0 ? used : end + ret;
 }
 
+/**
+ * Add a new element with boolean value to the JSON object stored in the
+ * provided buffer.
+ */
+static inline int
+rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
+		const char *name, bool val)
+{
+	int ret, end = used - 1;
+	if (used <= 2) /* assume empty, since minimum is '{}' */
+		return __json_snprintf(buf, len, "{\"%s\":%s}", name,
+				val ? "true" : "false");
+
+	ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
+			name, val ? "true" : "false");
+	return ret == 0 ? used : end + ret;
+}
+
 /**
  * Add a new element with int value to the JSON object stored in the
  * provided buffer.
diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
index 9794f9ea20..88f58d4d89 100644
--- a/lib/telemetry/version.map
+++ b/lib/telemetry/version.map
@@ -19,7 +19,17 @@ DPDK_23 {
 	local: *;
 };
 
+EXPERIMENTAL {
+	global:
+
+	# added in 22.11
+	rte_tel_data_add_array_bool;
+	rte_tel_data_add_dict_bool;
+};
+
 INTERNAL {
+	global:
+
 	rte_telemetry_legacy_register;
 	rte_telemetry_init;
 };
-- 
2.37.3

Re: [PATCH v3 1/4] telemetry: support boolean type

[Mattias Rönnblom]

On 2022-10-25 11:00, David Marchand wrote:
> Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Morten Brørup <mb@smartsharesystems.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> Acked-by: Ciara Power <ciara.power@intel.com>
> ---
> Changes since v1:
> - fixed doxygen description,
> 
> ---
>   app/test/test_telemetry_data.c | 88 +++++++++++++++++++++++++++++++++-
>   lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++
>   lib/telemetry/telemetry.c      | 24 +++++++++-
>   lib/telemetry/telemetry_data.c | 44 +++++++++++++++--
>   lib/telemetry/telemetry_data.h |  5 ++
>   lib/telemetry/telemetry_json.h | 34 +++++++++++++
>   lib/telemetry/version.map      | 10 ++++
>   7 files changed, 233 insertions(+), 8 deletions(-)
> 
> diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> index d92667a527..134e018fde 100644
> --- a/app/test/test_telemetry_data.c
> +++ b/app/test/test_telemetry_data.c
> @@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
>   	return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
>   }
>   
> +static int
> +test_case_array_bool(void)
> +{
> +	int i;
> +
> +	rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
> +	for (i = 0; i < 5; i++)
> +		rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
> +	return CHECK_OUTPUT("[true,false,true,false,true]");
> +}
> +
> +static int
> +test_case_add_dict_bool(void)
> +{
> +	int i = 0;
> +	char name_of_value[8];
> +
> +	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_bool(&response_data, name_of_value,
> +			(i % 2) == 0);
> +	}
> +	return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
> +		"\"dict_3\":false,\"dict_4\":true}");
> +}
> +
> +static int
> +test_dict_with_array_bool_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> +
> +	rte_tel_data_start_dict(&response_data);
> +
> +	for (i = 0; i < 10; i++) {
> +		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> +		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> +	}
> +
> +	rte_tel_data_add_dict_container(&response_data, "dict_0",
> +	 child_data, 0);
> +	rte_tel_data_add_dict_container(&response_data, "dict_1",
> +	 child_data2, 0);
> +
> +	return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
> +		"\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
> +}
> +
> +static int
> +test_array_with_array_bool_values(void)
> +{
> +	int i;
> +
> +	struct rte_tel_data *child_data = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> +
> +	struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> +	rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> +
> +	rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> +
> +	for (i = 0; i < 5; i++) {
> +		rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> +		rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> +	}
> +	rte_tel_data_add_array_container(&response_data, child_data, 0);
> +	rte_tel_data_add_array_container(&response_data, child_data2, 0);
> +
> +	return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
> +}
> +
>   static int
>   test_string_char_escaping(void)
>   {
> @@ -428,15 +506,21 @@ telemetry_data_autotest(void)
>   			test_null_return,
>   			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_array_int,
> +			test_case_array_u64,
> +			test_case_array_bool,
> +			test_case_add_dict_int,
> +			test_case_add_dict_u64,
> +			test_case_add_dict_bool,
>   			test_case_add_dict_string,
>   			test_dict_with_array_int_values,
>   			test_dict_with_array_u64_values,
> +			test_dict_with_array_bool_values,
>   			test_dict_with_array_string_values,
>   			test_dict_with_dict_values,
>   			test_array_with_array_int_values,
>   			test_array_with_array_u64_values,
> +			test_array_with_array_bool_values,
>   			test_array_with_array_string_values,
>   			test_string_char_escaping,
>   			test_array_char_escaping,
> diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> index a0d21d6b7f..e7f6c2ae43 100644
> --- a/lib/telemetry/rte_telemetry.h
> +++ b/lib/telemetry/rte_telemetry.h
> @@ -2,6 +2,7 @@
>    * Copyright(c) 2018 Intel Corporation
>    */
>   
> +#include <stdbool.h>
>   #include <stdint.h>
>   
>   #include <rte_compat.h>
> @@ -46,6 +47,7 @@ enum rte_tel_value_type {
>   	RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
>   	RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
>   	RTE_TEL_CONTAINER, /** a container struct */
> +	RTE_TEL_BOOL_VAL,   /** a boolean value */
>   };
>   
>   /**
> @@ -155,6 +157,22 @@ int
>   rte_tel_data_add_array_container(struct rte_tel_data *d,
>   		struct rte_tel_data *val, int keep);
>   
> +/**
> + * Add a boolean to an array.
> + * The array must have been started by rte_tel_data_start_array() with
> + * RTE_TEL_BOOL_VAL as the type parameter.
> + *
> + * @param d
> + *   The data structure passed to the callback
> + * @param x
> + *   The boolean value to be returned in the array
> + * @return
> + *   0 on success, negative errno on error
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
> +
>   /**
>    * Add a string value to a dictionary.
>    * The dict must have been started by rte_tel_data_start_dict().
> @@ -233,6 +251,24 @@ int
>   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
>   		struct rte_tel_data *val, int keep);
>   
> +/**
> + * Add a boolean value to a dictionary.
> + * The dict must have been started by rte_tel_data_start_dict().

...as RTE_TEL_BOOL_VAL as the type parameter?

> + *
> + * @param 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 boolean value to be stored in the dict
> + * @return
> + *   0 on success, negative errno on error, E2BIG on string truncation of name.
> + */
> +__rte_experimental
> +int
> +rte_tel_data_add_dict_bool(struct rte_tel_data *d, const char *name, bool val);
> +
>   /**
>    * This telemetry callback is used when registering a telemetry command.
>    * It handles getting and formatting information to be returned to telemetry
> diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> index 8fbb4f3060..276d0f337d 100644
> --- a/lib/telemetry/telemetry.c
> +++ b/lib/telemetry/telemetry.c
> @@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>   	unsigned int i;
>   
>   	if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
> -		d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
> +			d->type != RTE_TEL_ARRAY_INT &&
> +			d->type != RTE_TEL_ARRAY_STRING &&
> +			d->type != RTE_TEL_ARRAY_BOOL)

Use a switch for improved readability.

>   		return snprintf(out_buf, buf_len, "null");
>   
>   	used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>   			used = rte_tel_json_add_array_string(out_buf,
>   				buf_len, used,
>   				d->data.array[i].sval);
> +	if (d->type == RTE_TEL_ARRAY_BOOL)

This whole section should be refactor to use a switch statement, imo.

> +		for (i = 0; i < d->data_len; i++)
> +			used = rte_tel_json_add_array_bool(out_buf,
> +				buf_len, used,
> +				d->data.array[i].boolval);
>   	if (d->type == RTE_TEL_DICT)
>   		for (i = 0; i < d->data_len; i++) {
>   			const struct tel_dict_entry *v = &d->data.dict[i];
> @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
>   						buf_len, used,
>   						v->name, v->value.u64val);
>   				break;
> +			case RTE_TEL_BOOL_VAL:
> +				used = rte_tel_json_add_obj_bool(out_buf,
> +						buf_len, used,
> +						v->name, v->value.boolval);
> +				break;
>   			case RTE_TEL_CONTAINER:
>   			{
>   				char temp[buf_len];
> @@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>   						buf_len, used,
>   						v->name, v->value.u64val);
>   				break;
> +			case RTE_TEL_BOOL_VAL:
> +				used = rte_tel_json_add_obj_bool(cb_data_buf,
> +						buf_len, used,
> +						v->name, v->value.boolval);
> +				break;
>   			case RTE_TEL_CONTAINER:
>   			{
>   				char temp[buf_len];
> @@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>   	case RTE_TEL_ARRAY_STRING:
>   	case RTE_TEL_ARRAY_INT:
>   	case RTE_TEL_ARRAY_U64:
> +	case RTE_TEL_ARRAY_BOOL:
>   	case RTE_TEL_ARRAY_CONTAINER:
>   		used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
>   		for (i = 0; i < d->data_len; i++)
> @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
>   				used = rte_tel_json_add_array_u64(cb_data_buf,
>   						buf_len, used,
>   						d->data.array[i].u64val);
> +			else if (d->type == RTE_TEL_ARRAY_BOOL)
> +				used = rte_tel_json_add_array_bool(cb_data_buf,
> +						buf_len, used,
> +						d->data.array[i].boolval);
>   			else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
>   				char temp[buf_len];
>   				const struct container *rec_data =
> diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> index 34366ecee3..13a7ce7034 100644
> --- a/lib/telemetry/telemetry_data.c
> +++ b/lib/telemetry/telemetry_data.c
> @@ -16,10 +16,11 @@ int
>   rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
>   {
>   	enum tel_container_types array_types[] = {
> -			RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> -			RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> -			RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
> -			RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> +		[RTE_TEL_STRING_VAL] = RTE_TEL_ARRAY_STRING,
> +		[RTE_TEL_INT_VAL] = RTE_TEL_ARRAY_INT,
> +		[RTE_TEL_U64_VAL] = RTE_TEL_ARRAY_U64,
> +		[RTE_TEL_CONTAINER] = RTE_TEL_ARRAY_CONTAINER,
> +		[RTE_TEL_BOOL_VAL] = RTE_TEL_ARRAY_BOOL,

Seems like a brittle construct to me. Replace this with a switch statement.

>   	};
>   	d->type = array_types[type];
>   	d->data_len = 0;
> @@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
>   	return 0;
>   }
>   
> +int
> +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x)
> +{
> +	if (d->type != RTE_TEL_ARRAY_BOOL)
> +		return -EINVAL;
> +	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> +		return -ENOSPC;
> +	d->data.array[d->data_len++].boolval = x;
> +	return 0;
> +}
> +
>   int
>   rte_tel_data_add_array_container(struct rte_tel_data *d,
>   		struct rte_tel_data *val, int keep)
> @@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
>   	if (d->type != RTE_TEL_ARRAY_CONTAINER ||
>   			(val->type != RTE_TEL_ARRAY_U64
>   			&& val->type != RTE_TEL_ARRAY_INT
> -			&& val->type != RTE_TEL_ARRAY_STRING))
> +			&& val->type != RTE_TEL_ARRAY_STRING
> +			&& val->type != RTE_TEL_ARRAY_BOOL))

Maybe the introduction of a valid_type() static function would be in 
order? Implemented by means of a (surprise!) switch statement. :)

Could be is_valid_type() and rename valid_name() us_valid_name() in the 
process.

Use in rte_tel_data_add_dict_container() as well.

>   		return -EINVAL;
>   	if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
>   		return -ENOSPC;
> @@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
>   	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
>   }
>   
> +int
> +rte_tel_data_add_dict_bool(struct rte_tel_data *d,
> +		const char *name, bool val)
> +{
> +	struct tel_dict_entry *e = &d->data.dict[d->data_len];
> +	if (d->type != RTE_TEL_DICT)
> +		return -EINVAL;
> +	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> +		return -ENOSPC;
> +
> +	if (!valid_name(name))
> +		return -EINVAL;
> +
> +	d->data_len++;
> +	e->type = RTE_TEL_BOOL_VAL;
> +	e->value.boolval = val;
> +	const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> +	return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> +}
> +
>   int
>   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
>   		struct rte_tel_data *val, int keep)
> @@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
>   	if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
>   			&& val->type != RTE_TEL_ARRAY_INT
>   			&& val->type != RTE_TEL_ARRAY_STRING
> +			&& val->type != RTE_TEL_ARRAY_BOOL
>   			&& val->type != RTE_TEL_DICT))
>   		return -EINVAL;
>   	if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
> index 26aa28e72c..c840486b18 100644
> --- a/lib/telemetry/telemetry_data.h
> +++ b/lib/telemetry/telemetry_data.h
> @@ -5,6 +5,9 @@
>   #ifndef _TELEMETRY_DATA_H_
>   #define _TELEMETRY_DATA_H_
>   
> +#include <stdbool.h>
> +#include <stdint.h>
> +
>   #include "rte_telemetry.h"
>   
>   enum tel_container_types {
> @@ -15,6 +18,7 @@ enum tel_container_types {
>   	RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
>   	RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
>   	RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> +	RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
>   };
>   
>   struct container {
> @@ -30,6 +34,7 @@ union tel_value {
>   	char sval[RTE_TEL_MAX_STRING_LEN];
>   	int ival;
>   	uint64_t u64val;
> +	bool boolval;
>   	struct container container;
>   };
>   
> diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> index e3fae7c30d..c97da97366 100644
> --- a/lib/telemetry/telemetry_json.h
> +++ b/lib/telemetry/telemetry_json.h
> @@ -7,6 +7,7 @@
>   
>   #include <inttypes.h>
>   #include <stdarg.h>
> +#include <stdbool.h>
>   #include <stdio.h>
>   #include <rte_common.h>
>   #include <rte_telemetry.h>
> @@ -159,6 +160,21 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
>   	return ret == 0 ? used : end + ret;
>   }
>   
> +/* Appends a boolean into the JSON array in the provided buffer. */
> +static inline int
> +rte_tel_json_add_array_bool(char *buf, const int len, const int used,
> +		bool val)
> +{
> +	int ret, end = used - 1; /* strip off final delimiter */
> +	if (used <= 2) /* assume empty, since minimum is '[]' */
> +		return __json_snprintf(buf, len, "[%s]",
> +				val ? "true" : "false");

Have "true" and "false" as #defines, and make a bool_str() helper, which 
returns the (constant) string form.

> +
> +	ret = __json_snprintf(buf + end, len - end, ",%s]",
> +			val ? "true" : "false");
> +	return ret == 0 ? used : end + ret;
> +}
> +
>   /*
>    * Add a new element with raw JSON value to the JSON array stored in the
>    * provided buffer.
> @@ -193,6 +209,24 @@ rte_tel_json_add_obj_u64(char *buf, const int len, const int used,
>   	return ret == 0 ? used : end + ret;
>   }
>   
> +/**
> + * Add a new element with boolean value to the JSON object stored in the
> + * provided buffer.
> + */
> +static inline int

I know this pattern isn't introduced in this patch, but why is this 
function in the header file?

> +rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
> +		const char *name, bool val)
> +{
> +	int ret, end = used - 1;
> +	if (used <= 2) /* assume empty, since minimum is '{}' */

RTE_ASSERT() wouldn't be more helpful? The input buffer is malformed, 
correct?

Either way, the magic '2' should be a #define.

> +		return __json_snprintf(buf, len, "{\"%s\":%s}", name,
> +				val ? "true" : "false");
> +
> +	ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> +			name, val ? "true" : "false");
> +	return ret == 0 ? used : end + ret;
> +}
> +

This looks like cut-and-paste. Merge into one function, parameterized by 
the start and end delimiter.

>   /**
>    * Add a new element with int value to the JSON object stored in the
>    * provided buffer.
> diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> index 9794f9ea20..88f58d4d89 100644
> --- a/lib/telemetry/version.map
> +++ b/lib/telemetry/version.map
> @@ -19,7 +19,17 @@ DPDK_23 {
>   	local: *;
>   };
>   
> +EXPERIMENTAL {
> +	global:
> +
> +	# added in 22.11
> +	rte_tel_data_add_array_bool;
> +	rte_tel_data_add_dict_bool;
> +};
> +
>   INTERNAL {
> +	global:
> +
>   	rte_telemetry_legacy_register;
>   	rte_telemetry_init;
>   };

Re: [PATCH v3 1/4] telemetry: support boolean type

[David Marchand]

On Tue, Oct 25, 2022 at 11:34 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
>
> On 2022-10-25 11:00, David Marchand wrote:
> > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> >
> > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > Acked-by: Ciara Power <ciara.power@intel.com>
> > ---
> > Changes since v1:
> > - fixed doxygen description,
> >
> > ---
> >   app/test/test_telemetry_data.c | 88 +++++++++++++++++++++++++++++++++-
> >   lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++
> >   lib/telemetry/telemetry.c      | 24 +++++++++-
> >   lib/telemetry/telemetry_data.c | 44 +++++++++++++++--
> >   lib/telemetry/telemetry_data.h |  5 ++
> >   lib/telemetry/telemetry_json.h | 34 +++++++++++++
> >   lib/telemetry/version.map      | 10 ++++
> >   7 files changed, 233 insertions(+), 8 deletions(-)
> >
> > diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> > index d92667a527..134e018fde 100644
> > --- a/app/test/test_telemetry_data.c
> > +++ b/app/test/test_telemetry_data.c
> > @@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
> >       return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
> >   }
> >
> > +static int
> > +test_case_array_bool(void)
> > +{
> > +     int i;
> > +
> > +     rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
> > +     for (i = 0; i < 5; i++)
> > +             rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
> > +     return CHECK_OUTPUT("[true,false,true,false,true]");
> > +}
> > +
> > +static int
> > +test_case_add_dict_bool(void)
> > +{
> > +     int i = 0;
> > +     char name_of_value[8];
> > +
> > +     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_bool(&response_data, name_of_value,
> > +                     (i % 2) == 0);
> > +     }
> > +     return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
> > +             "\"dict_3\":false,\"dict_4\":true}");
> > +}
> > +
> > +static int
> > +test_dict_with_array_bool_values(void)
> > +{
> > +     int i;
> > +
> > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > +
> > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > +
> > +     rte_tel_data_start_dict(&response_data);
> > +
> > +     for (i = 0; i < 10; i++) {
> > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > +     }
> > +
> > +     rte_tel_data_add_dict_container(&response_data, "dict_0",
> > +      child_data, 0);
> > +     rte_tel_data_add_dict_container(&response_data, "dict_1",
> > +      child_data2, 0);
> > +
> > +     return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
> > +             "\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
> > +}
> > +
> > +static int
> > +test_array_with_array_bool_values(void)
> > +{
> > +     int i;
> > +
> > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > +
> > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > +
> > +     rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> > +
> > +     for (i = 0; i < 5; i++) {
> > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > +     }
> > +     rte_tel_data_add_array_container(&response_data, child_data, 0);
> > +     rte_tel_data_add_array_container(&response_data, child_data2, 0);
> > +
> > +     return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
> > +}
> > +
> >   static int
> >   test_string_char_escaping(void)
> >   {
> > @@ -428,15 +506,21 @@ telemetry_data_autotest(void)
> >                       test_null_return,
> >                       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_array_int,
> > +                     test_case_array_u64,
> > +                     test_case_array_bool,
> > +                     test_case_add_dict_int,
> > +                     test_case_add_dict_u64,
> > +                     test_case_add_dict_bool,
> >                       test_case_add_dict_string,
> >                       test_dict_with_array_int_values,
> >                       test_dict_with_array_u64_values,
> > +                     test_dict_with_array_bool_values,
> >                       test_dict_with_array_string_values,
> >                       test_dict_with_dict_values,
> >                       test_array_with_array_int_values,
> >                       test_array_with_array_u64_values,
> > +                     test_array_with_array_bool_values,
> >                       test_array_with_array_string_values,
> >                       test_string_char_escaping,
> >                       test_array_char_escaping,
> > diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> > index a0d21d6b7f..e7f6c2ae43 100644
> > --- a/lib/telemetry/rte_telemetry.h
> > +++ b/lib/telemetry/rte_telemetry.h
> > @@ -2,6 +2,7 @@
> >    * Copyright(c) 2018 Intel Corporation
> >    */
> >
> > +#include <stdbool.h>
> >   #include <stdint.h>
> >
> >   #include <rte_compat.h>
> > @@ -46,6 +47,7 @@ enum rte_tel_value_type {
> >       RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
> >       RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
> >       RTE_TEL_CONTAINER, /** a container struct */
> > +     RTE_TEL_BOOL_VAL,   /** a boolean value */
> >   };
> >
> >   /**
> > @@ -155,6 +157,22 @@ int
> >   rte_tel_data_add_array_container(struct rte_tel_data *d,
> >               struct rte_tel_data *val, int keep);
> >
> > +/**
> > + * Add a boolean to an array.
> > + * The array must have been started by rte_tel_data_start_array() with
> > + * RTE_TEL_BOOL_VAL as the type parameter.
> > + *
> > + * @param d
> > + *   The data structure passed to the callback
> > + * @param x
> > + *   The boolean value to be returned in the array
> > + * @return
> > + *   0 on success, negative errno on error
> > + */
> > +__rte_experimental
> > +int
> > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
> > +
> >   /**
> >    * Add a string value to a dictionary.
> >    * The dict must have been started by rte_tel_data_start_dict().
> > @@ -233,6 +251,24 @@ int
> >   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> >               struct rte_tel_data *val, int keep);
> >
> > +/**
> > + * Add a boolean value to a dictionary.
> > + * The dict must have been started by rte_tel_data_start_dict().
>
> ...as RTE_TEL_BOOL_VAL as the type parameter?
>
> > + *
> > + * @param 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 boolean value to be stored in the dict
> > + * @return
> > + *   0 on success, negative errno on error, E2BIG on string truncation of name.
> > + */
> > +__rte_experimental
> > +int
> > +rte_tel_data_add_dict_bool(struct rte_tel_data *d, const char *name, bool val);
> > +
> >   /**
> >    * This telemetry callback is used when registering a telemetry command.
> >    * It handles getting and formatting information to be returned to telemetry
> > diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> > index 8fbb4f3060..276d0f337d 100644
> > --- a/lib/telemetry/telemetry.c
> > +++ b/lib/telemetry/telemetry.c
> > @@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> >       unsigned int i;
> >
> >       if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
> > -             d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
> > +                     d->type != RTE_TEL_ARRAY_INT &&
> > +                     d->type != RTE_TEL_ARRAY_STRING &&
> > +                     d->type != RTE_TEL_ARRAY_BOOL)
>
> Use a switch for improved readability.
>
> >               return snprintf(out_buf, buf_len, "null");
> >
> >       used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> > @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> >                       used = rte_tel_json_add_array_string(out_buf,
> >                               buf_len, used,
> >                               d->data.array[i].sval);
> > +     if (d->type == RTE_TEL_ARRAY_BOOL)
>
> This whole section should be refactor to use a switch statement, imo.
>
> > +             for (i = 0; i < d->data_len; i++)
> > +                     used = rte_tel_json_add_array_bool(out_buf,
> > +                             buf_len, used,
> > +                             d->data.array[i].boolval);
> >       if (d->type == RTE_TEL_DICT)
> >               for (i = 0; i < d->data_len; i++) {
> >                       const struct tel_dict_entry *v = &d->data.dict[i];
> > @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> >                                               buf_len, used,
> >                                               v->name, v->value.u64val);
> >                               break;
> > +                     case RTE_TEL_BOOL_VAL:
> > +                             used = rte_tel_json_add_obj_bool(out_buf,
> > +                                             buf_len, used,
> > +                                             v->name, v->value.boolval);
> > +                             break;
> >                       case RTE_TEL_CONTAINER:
> >                       {
> >                               char temp[buf_len];
> > @@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >                                               buf_len, used,
> >                                               v->name, v->value.u64val);
> >                               break;
> > +                     case RTE_TEL_BOOL_VAL:
> > +                             used = rte_tel_json_add_obj_bool(cb_data_buf,
> > +                                             buf_len, used,
> > +                                             v->name, v->value.boolval);
> > +                             break;
> >                       case RTE_TEL_CONTAINER:
> >                       {
> >                               char temp[buf_len];
> > @@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >       case RTE_TEL_ARRAY_STRING:
> >       case RTE_TEL_ARRAY_INT:
> >       case RTE_TEL_ARRAY_U64:
> > +     case RTE_TEL_ARRAY_BOOL:
> >       case RTE_TEL_ARRAY_CONTAINER:
> >               used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
> >               for (i = 0; i < d->data_len; i++)
> > @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> >                               used = rte_tel_json_add_array_u64(cb_data_buf,
> >                                               buf_len, used,
> >                                               d->data.array[i].u64val);
> > +                     else if (d->type == RTE_TEL_ARRAY_BOOL)
> > +                             used = rte_tel_json_add_array_bool(cb_data_buf,
> > +                                             buf_len, used,
> > +                                             d->data.array[i].boolval);
> >                       else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
> >                               char temp[buf_len];
> >                               const struct container *rec_data =
> > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > index 34366ecee3..13a7ce7034 100644
> > --- a/lib/telemetry/telemetry_data.c
> > +++ b/lib/telemetry/telemetry_data.c
> > @@ -16,10 +16,11 @@ int
> >   rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> >   {
> >       enum tel_container_types array_types[] = {
> > -                     RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> > -                     RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> > -                     RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
> > -                     RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> > +             [RTE_TEL_STRING_VAL] = RTE_TEL_ARRAY_STRING,
> > +             [RTE_TEL_INT_VAL] = RTE_TEL_ARRAY_INT,
> > +             [RTE_TEL_U64_VAL] = RTE_TEL_ARRAY_U64,
> > +             [RTE_TEL_CONTAINER] = RTE_TEL_ARRAY_CONTAINER,
> > +             [RTE_TEL_BOOL_VAL] = RTE_TEL_ARRAY_BOOL,
>
> Seems like a brittle construct to me. Replace this with a switch statement.
>
> >       };
> >       d->type = array_types[type];
> >       d->data_len = 0;
> > @@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
> >       return 0;
> >   }
> >
> > +int
> > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x)
> > +{
> > +     if (d->type != RTE_TEL_ARRAY_BOOL)
> > +             return -EINVAL;
> > +     if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> > +             return -ENOSPC;
> > +     d->data.array[d->data_len++].boolval = x;
> > +     return 0;
> > +}
> > +
> >   int
> >   rte_tel_data_add_array_container(struct rte_tel_data *d,
> >               struct rte_tel_data *val, int keep)
> > @@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
> >       if (d->type != RTE_TEL_ARRAY_CONTAINER ||
> >                       (val->type != RTE_TEL_ARRAY_U64
> >                       && val->type != RTE_TEL_ARRAY_INT
> > -                     && val->type != RTE_TEL_ARRAY_STRING))
> > +                     && val->type != RTE_TEL_ARRAY_STRING
> > +                     && val->type != RTE_TEL_ARRAY_BOOL))
>
> Maybe the introduction of a valid_type() static function would be in
> order? Implemented by means of a (surprise!) switch statement. :)
>
> Could be is_valid_type() and rename valid_name() us_valid_name() in the
> process.
>
> Use in rte_tel_data_add_dict_container() as well.
>
> >               return -EINVAL;
> >       if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> >               return -ENOSPC;
> > @@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
> >       return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> >   }
> >
> > +int
> > +rte_tel_data_add_dict_bool(struct rte_tel_data *d,
> > +             const char *name, bool val)
> > +{
> > +     struct tel_dict_entry *e = &d->data.dict[d->data_len];
> > +     if (d->type != RTE_TEL_DICT)
> > +             return -EINVAL;
> > +     if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > +             return -ENOSPC;
> > +
> > +     if (!valid_name(name))
> > +             return -EINVAL;
> > +
> > +     d->data_len++;
> > +     e->type = RTE_TEL_BOOL_VAL;
> > +     e->value.boolval = val;
> > +     const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> > +     return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> > +}
> > +
> >   int
> >   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> >               struct rte_tel_data *val, int keep)
> > @@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> >       if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
> >                       && val->type != RTE_TEL_ARRAY_INT
> >                       && val->type != RTE_TEL_ARRAY_STRING
> > +                     && val->type != RTE_TEL_ARRAY_BOOL
> >                       && val->type != RTE_TEL_DICT))
> >               return -EINVAL;
> >       if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
> > index 26aa28e72c..c840486b18 100644
> > --- a/lib/telemetry/telemetry_data.h
> > +++ b/lib/telemetry/telemetry_data.h
> > @@ -5,6 +5,9 @@
> >   #ifndef _TELEMETRY_DATA_H_
> >   #define _TELEMETRY_DATA_H_
> >
> > +#include <stdbool.h>
> > +#include <stdint.h>
> > +
> >   #include "rte_telemetry.h"
> >
> >   enum tel_container_types {
> > @@ -15,6 +18,7 @@ enum tel_container_types {
> >       RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
> >       RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
> >       RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> > +     RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
> >   };
> >
> >   struct container {
> > @@ -30,6 +34,7 @@ union tel_value {
> >       char sval[RTE_TEL_MAX_STRING_LEN];
> >       int ival;
> >       uint64_t u64val;
> > +     bool boolval;
> >       struct container container;
> >   };
> >
> > diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> > index e3fae7c30d..c97da97366 100644
> > --- a/lib/telemetry/telemetry_json.h
> > +++ b/lib/telemetry/telemetry_json.h
> > @@ -7,6 +7,7 @@
> >
> >   #include <inttypes.h>
> >   #include <stdarg.h>
> > +#include <stdbool.h>
> >   #include <stdio.h>
> >   #include <rte_common.h>
> >   #include <rte_telemetry.h>
> > @@ -159,6 +160,21 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
> >       return ret == 0 ? used : end + ret;
> >   }
> >
> > +/* Appends a boolean into the JSON array in the provided buffer. */
> > +static inline int
> > +rte_tel_json_add_array_bool(char *buf, const int len, const int used,
> > +             bool val)
> > +{
> > +     int ret, end = used - 1; /* strip off final delimiter */
> > +     if (used <= 2) /* assume empty, since minimum is '[]' */
> > +             return __json_snprintf(buf, len, "[%s]",
> > +                             val ? "true" : "false");
>
> Have "true" and "false" as #defines, and make a bool_str() helper, which
> returns the (constant) string form.
>
> > +
> > +     ret = __json_snprintf(buf + end, len - end, ",%s]",
> > +                     val ? "true" : "false");
> > +     return ret == 0 ? used : end + ret;
> > +}
> > +
> >   /*
> >    * Add a new element with raw JSON value to the JSON array stored in the
> >    * provided buffer.
> > @@ -193,6 +209,24 @@ rte_tel_json_add_obj_u64(char *buf, const int len, const int used,
> >       return ret == 0 ? used : end + ret;
> >   }
> >
> > +/**
> > + * Add a new element with boolean value to the JSON object stored in the
> > + * provided buffer.
> > + */
> > +static inline int
>
> I know this pattern isn't introduced in this patch, but why is this
> function in the header file?
>
> > +rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
> > +             const char *name, bool val)
> > +{
> > +     int ret, end = used - 1;
> > +     if (used <= 2) /* assume empty, since minimum is '{}' */
>
> RTE_ASSERT() wouldn't be more helpful? The input buffer is malformed,
> correct?
>
> Either way, the magic '2' should be a #define.
>
> > +             return __json_snprintf(buf, len, "{\"%s\":%s}", name,
> > +                             val ? "true" : "false");
> > +
> > +     ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> > +                     name, val ? "true" : "false");
> > +     return ret == 0 ? used : end + ret;
> > +}
> > +
>
> This looks like cut-and-paste. Merge into one function, parameterized by
> the start and end delimiter.
>
> >   /**
> >    * Add a new element with int value to the JSON object stored in the
> >    * provided buffer.
> > diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> > index 9794f9ea20..88f58d4d89 100644
> > --- a/lib/telemetry/version.map
> > +++ b/lib/telemetry/version.map
> > @@ -19,7 +19,17 @@ DPDK_23 {
> >       local: *;
> >   };
> >
> > +EXPERIMENTAL {
> > +     global:
> > +
> > +     # added in 22.11
> > +     rte_tel_data_add_array_bool;
> > +     rte_tel_data_add_dict_bool;
> > +};
> > +
> >   INTERNAL {
> > +     global:
> > +
> >       rte_telemetry_legacy_register;
> >       rte_telemetry_init;
> >   };
>

I agree with most of the comments.
I can follow up on those cleanups, but I don't have the time now.

I'll let the telemetry maintainers decide on the fate of those patches.
Thanks.


-- 
David Marchand

Re: [PATCH v3 1/4] telemetry: support boolean type

[Bruce Richardson]

On Tue, Oct 25, 2022 at 11:43:27AM +0200, David Marchand wrote:
> On Tue, Oct 25, 2022 at 11:34 AM Mattias Rönnblom <hofors@lysator.liu.se> wrote:
> >
> > On 2022-10-25 11:00, David Marchand wrote:
> > > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and dicts.
> > >
> > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > Acked-by: Ciara Power <ciara.power@intel.com>
> > > ---
> > > Changes since v1:
> > > - fixed doxygen description,
> > >
> > > ---
> > >   app/test/test_telemetry_data.c | 88 +++++++++++++++++++++++++++++++++-
> > >   lib/telemetry/rte_telemetry.h  | 36 ++++++++++++++
> > >   lib/telemetry/telemetry.c      | 24 +++++++++-
> > >   lib/telemetry/telemetry_data.c | 44 +++++++++++++++--
> > >   lib/telemetry/telemetry_data.h |  5 ++
> > >   lib/telemetry/telemetry_json.h | 34 +++++++++++++
> > >   lib/telemetry/version.map      | 10 ++++
> > >   7 files changed, 233 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/app/test/test_telemetry_data.c b/app/test/test_telemetry_data.c
> > > index d92667a527..134e018fde 100644
> > > --- a/app/test/test_telemetry_data.c
> > > +++ b/app/test/test_telemetry_data.c
> > > @@ -353,6 +353,84 @@ test_array_with_array_u64_values(void)
> > >       return CHECK_OUTPUT("[[0,1,2,3,4],[0,1,2,3,4]]");
> > >   }
> > >
> > > +static int
> > > +test_case_array_bool(void)
> > > +{
> > > +     int i;
> > > +
> > > +     rte_tel_data_start_array(&response_data, RTE_TEL_BOOL_VAL);
> > > +     for (i = 0; i < 5; i++)
> > > +             rte_tel_data_add_array_bool(&response_data, (i % 2) == 0);
> > > +     return CHECK_OUTPUT("[true,false,true,false,true]");
> > > +}
> > > +
> > > +static int
> > > +test_case_add_dict_bool(void)
> > > +{
> > > +     int i = 0;
> > > +     char name_of_value[8];
> > > +
> > > +     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_bool(&response_data, name_of_value,
> > > +                     (i % 2) == 0);
> > > +     }
> > > +     return CHECK_OUTPUT("{\"dict_0\":true,\"dict_1\":false,\"dict_2\":true,"
> > > +             "\"dict_3\":false,\"dict_4\":true}");
> > > +}
> > > +
> > > +static int
> > > +test_dict_with_array_bool_values(void)
> > > +{
> > > +     int i;
> > > +
> > > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > > +
> > > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > > +
> > > +     rte_tel_data_start_dict(&response_data);
> > > +
> > > +     for (i = 0; i < 10; i++) {
> > > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > > +     }
> > > +
> > > +     rte_tel_data_add_dict_container(&response_data, "dict_0",
> > > +      child_data, 0);
> > > +     rte_tel_data_add_dict_container(&response_data, "dict_1",
> > > +      child_data2, 0);
> > > +
> > > +     return CHECK_OUTPUT("{\"dict_0\":[true,false,true,false,true,false,true,false,true,false],"
> > > +             "\"dict_1\":[false,true,false,true,false,true,false,true,false,true]}");
> > > +}
> > > +
> > > +static int
> > > +test_array_with_array_bool_values(void)
> > > +{
> > > +     int i;
> > > +
> > > +     struct rte_tel_data *child_data = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data, RTE_TEL_BOOL_VAL);
> > > +
> > > +     struct rte_tel_data *child_data2 = rte_tel_data_alloc();
> > > +     rte_tel_data_start_array(child_data2, RTE_TEL_BOOL_VAL);
> > > +
> > > +     rte_tel_data_start_array(&response_data, RTE_TEL_CONTAINER);
> > > +
> > > +     for (i = 0; i < 5; i++) {
> > > +             rte_tel_data_add_array_bool(child_data, (i % 2) == 0);
> > > +             rte_tel_data_add_array_bool(child_data2, (i % 2) == 1);
> > > +     }
> > > +     rte_tel_data_add_array_container(&response_data, child_data, 0);
> > > +     rte_tel_data_add_array_container(&response_data, child_data2, 0);
> > > +
> > > +     return CHECK_OUTPUT("[[true,false,true,false,true],[false,true,false,true,false]]");
> > > +}
> > > +
> > >   static int
> > >   test_string_char_escaping(void)
> > >   {
> > > @@ -428,15 +506,21 @@ telemetry_data_autotest(void)
> > >                       test_null_return,
> > >                       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_array_int,
> > > +                     test_case_array_u64,
> > > +                     test_case_array_bool,
> > > +                     test_case_add_dict_int,
> > > +                     test_case_add_dict_u64,
> > > +                     test_case_add_dict_bool,
> > >                       test_case_add_dict_string,
> > >                       test_dict_with_array_int_values,
> > >                       test_dict_with_array_u64_values,
> > > +                     test_dict_with_array_bool_values,
> > >                       test_dict_with_array_string_values,
> > >                       test_dict_with_dict_values,
> > >                       test_array_with_array_int_values,
> > >                       test_array_with_array_u64_values,
> > > +                     test_array_with_array_bool_values,
> > >                       test_array_with_array_string_values,
> > >                       test_string_char_escaping,
> > >                       test_array_char_escaping,
> > > diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
> > > index a0d21d6b7f..e7f6c2ae43 100644
> > > --- a/lib/telemetry/rte_telemetry.h
> > > +++ b/lib/telemetry/rte_telemetry.h
> > > @@ -2,6 +2,7 @@
> > >    * Copyright(c) 2018 Intel Corporation
> > >    */
> > >
> > > +#include <stdbool.h>
> > >   #include <stdint.h>
> > >
> > >   #include <rte_compat.h>
> > > @@ -46,6 +47,7 @@ enum rte_tel_value_type {
> > >       RTE_TEL_INT_VAL,    /** a signed 32-bit int value */
> > >       RTE_TEL_U64_VAL,    /** an unsigned 64-bit int value */
> > >       RTE_TEL_CONTAINER, /** a container struct */
> > > +     RTE_TEL_BOOL_VAL,   /** a boolean value */
> > >   };
> > >
> > >   /**
> > > @@ -155,6 +157,22 @@ int
> > >   rte_tel_data_add_array_container(struct rte_tel_data *d,
> > >               struct rte_tel_data *val, int keep);
> > >
> > > +/**
> > > + * Add a boolean to an array.
> > > + * The array must have been started by rte_tel_data_start_array() with
> > > + * RTE_TEL_BOOL_VAL as the type parameter.
> > > + *
> > > + * @param d
> > > + *   The data structure passed to the callback
> > > + * @param x
> > > + *   The boolean value to be returned in the array
> > > + * @return
> > > + *   0 on success, negative errno on error
> > > + */
> > > +__rte_experimental
> > > +int
> > > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
> > > +
> > >   /**
> > >    * Add a string value to a dictionary.
> > >    * The dict must have been started by rte_tel_data_start_dict().
> > > @@ -233,6 +251,24 @@ int
> > >   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> > >               struct rte_tel_data *val, int keep);
> > >
> > > +/**
> > > + * Add a boolean value to a dictionary.
> > > + * The dict must have been started by rte_tel_data_start_dict().
> >
> > ...as RTE_TEL_BOOL_VAL as the type parameter?
> >
> > > + *
> > > + * @param 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 boolean value to be stored in the dict
> > > + * @return
> > > + *   0 on success, negative errno on error, E2BIG on string truncation of name.
> > > + */
> > > +__rte_experimental
> > > +int
> > > +rte_tel_data_add_dict_bool(struct rte_tel_data *d, const char *name, bool val);
> > > +
> > >   /**
> > >    * This telemetry callback is used when registering a telemetry command.
> > >    * It handles getting and formatting information to be returned to telemetry
> > > diff --git a/lib/telemetry/telemetry.c b/lib/telemetry/telemetry.c
> > > index 8fbb4f3060..276d0f337d 100644
> > > --- a/lib/telemetry/telemetry.c
> > > +++ b/lib/telemetry/telemetry.c
> > > @@ -168,7 +168,9 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> > >       unsigned int i;
> > >
> > >       if (d->type != RTE_TEL_DICT && d->type != RTE_TEL_ARRAY_U64 &&
> > > -             d->type != RTE_TEL_ARRAY_INT && d->type != RTE_TEL_ARRAY_STRING)
> > > +                     d->type != RTE_TEL_ARRAY_INT &&
> > > +                     d->type != RTE_TEL_ARRAY_STRING &&
> > > +                     d->type != RTE_TEL_ARRAY_BOOL)
> >
> > Use a switch for improved readability.
> >
> > >               return snprintf(out_buf, buf_len, "null");
> > >
> > >       used = rte_tel_json_empty_array(out_buf, buf_len, 0);
> > > @@ -187,6 +189,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> > >                       used = rte_tel_json_add_array_string(out_buf,
> > >                               buf_len, used,
> > >                               d->data.array[i].sval);
> > > +     if (d->type == RTE_TEL_ARRAY_BOOL)
> >
> > This whole section should be refactor to use a switch statement, imo.
> >
> > > +             for (i = 0; i < d->data_len; i++)
> > > +                     used = rte_tel_json_add_array_bool(out_buf,
> > > +                             buf_len, used,
> > > +                             d->data.array[i].boolval);
> > >       if (d->type == RTE_TEL_DICT)
> > >               for (i = 0; i < d->data_len; i++) {
> > >                       const struct tel_dict_entry *v = &d->data.dict[i];
> > > @@ -206,6 +213,11 @@ container_to_json(const struct rte_tel_data *d, char *out_buf, size_t buf_len)
> > >                                               buf_len, used,
> > >                                               v->name, v->value.u64val);
> > >                               break;
> > > +                     case RTE_TEL_BOOL_VAL:
> > > +                             used = rte_tel_json_add_obj_bool(out_buf,
> > > +                                             buf_len, used,
> > > +                                             v->name, v->value.boolval);
> > > +                             break;
> > >                       case RTE_TEL_CONTAINER:
> > >                       {
> > >                               char temp[buf_len];
> > > @@ -273,6 +285,11 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> > >                                               buf_len, used,
> > >                                               v->name, v->value.u64val);
> > >                               break;
> > > +                     case RTE_TEL_BOOL_VAL:
> > > +                             used = rte_tel_json_add_obj_bool(cb_data_buf,
> > > +                                             buf_len, used,
> > > +                                             v->name, v->value.boolval);
> > > +                             break;
> > >                       case RTE_TEL_CONTAINER:
> > >                       {
> > >                               char temp[buf_len];
> > > @@ -294,6 +311,7 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> > >       case RTE_TEL_ARRAY_STRING:
> > >       case RTE_TEL_ARRAY_INT:
> > >       case RTE_TEL_ARRAY_U64:
> > > +     case RTE_TEL_ARRAY_BOOL:
> > >       case RTE_TEL_ARRAY_CONTAINER:
> > >               used = rte_tel_json_empty_array(cb_data_buf, buf_len, 0);
> > >               for (i = 0; i < d->data_len; i++)
> > > @@ -310,6 +328,10 @@ output_json(const char *cmd, const struct rte_tel_data *d, int s)
> > >                               used = rte_tel_json_add_array_u64(cb_data_buf,
> > >                                               buf_len, used,
> > >                                               d->data.array[i].u64val);
> > > +                     else if (d->type == RTE_TEL_ARRAY_BOOL)
> > > +                             used = rte_tel_json_add_array_bool(cb_data_buf,
> > > +                                             buf_len, used,
> > > +                                             d->data.array[i].boolval);
> > >                       else if (d->type == RTE_TEL_ARRAY_CONTAINER) {
> > >                               char temp[buf_len];
> > >                               const struct container *rec_data =
> > > diff --git a/lib/telemetry/telemetry_data.c b/lib/telemetry/telemetry_data.c
> > > index 34366ecee3..13a7ce7034 100644
> > > --- a/lib/telemetry/telemetry_data.c
> > > +++ b/lib/telemetry/telemetry_data.c
> > > @@ -16,10 +16,11 @@ int
> > >   rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type)
> > >   {
> > >       enum tel_container_types array_types[] = {
> > > -                     RTE_TEL_ARRAY_STRING, /* RTE_TEL_STRING_VAL = 0 */
> > > -                     RTE_TEL_ARRAY_INT,    /* RTE_TEL_INT_VAL = 1 */
> > > -                     RTE_TEL_ARRAY_U64,    /* RTE_TEL_u64_VAL = 2 */
> > > -                     RTE_TEL_ARRAY_CONTAINER, /* RTE_TEL_CONTAINER = 3 */
> > > +             [RTE_TEL_STRING_VAL] = RTE_TEL_ARRAY_STRING,
> > > +             [RTE_TEL_INT_VAL] = RTE_TEL_ARRAY_INT,
> > > +             [RTE_TEL_U64_VAL] = RTE_TEL_ARRAY_U64,
> > > +             [RTE_TEL_CONTAINER] = RTE_TEL_ARRAY_CONTAINER,
> > > +             [RTE_TEL_BOOL_VAL] = RTE_TEL_ARRAY_BOOL,
> >
> > Seems like a brittle construct to me. Replace this with a switch statement.
> >
> > >       };
> > >       d->type = array_types[type];
> > >       d->data_len = 0;
> > > @@ -80,6 +81,17 @@ rte_tel_data_add_array_u64(struct rte_tel_data *d, uint64_t x)
> > >       return 0;
> > >   }
> > >
> > > +int
> > > +rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x)
> > > +{
> > > +     if (d->type != RTE_TEL_ARRAY_BOOL)
> > > +             return -EINVAL;
> > > +     if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> > > +             return -ENOSPC;
> > > +     d->data.array[d->data_len++].boolval = x;
> > > +     return 0;
> > > +}
> > > +
> > >   int
> > >   rte_tel_data_add_array_container(struct rte_tel_data *d,
> > >               struct rte_tel_data *val, int keep)
> > > @@ -87,7 +99,8 @@ rte_tel_data_add_array_container(struct rte_tel_data *d,
> > >       if (d->type != RTE_TEL_ARRAY_CONTAINER ||
> > >                       (val->type != RTE_TEL_ARRAY_U64
> > >                       && val->type != RTE_TEL_ARRAY_INT
> > > -                     && val->type != RTE_TEL_ARRAY_STRING))
> > > +                     && val->type != RTE_TEL_ARRAY_STRING
> > > +                     && val->type != RTE_TEL_ARRAY_BOOL))
> >
> > Maybe the introduction of a valid_type() static function would be in
> > order? Implemented by means of a (surprise!) switch statement. :)
> >
> > Could be is_valid_type() and rename valid_name() us_valid_name() in the
> > process.
> >
> > Use in rte_tel_data_add_dict_container() as well.
> >
> > >               return -EINVAL;
> > >       if (d->data_len >= RTE_TEL_MAX_ARRAY_ENTRIES)
> > >               return -ENOSPC;
> > > @@ -179,6 +192,26 @@ rte_tel_data_add_dict_u64(struct rte_tel_data *d,
> > >       return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> > >   }
> > >
> > > +int
> > > +rte_tel_data_add_dict_bool(struct rte_tel_data *d,
> > > +             const char *name, bool val)
> > > +{
> > > +     struct tel_dict_entry *e = &d->data.dict[d->data_len];
> > > +     if (d->type != RTE_TEL_DICT)
> > > +             return -EINVAL;
> > > +     if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > > +             return -ENOSPC;
> > > +
> > > +     if (!valid_name(name))
> > > +             return -EINVAL;
> > > +
> > > +     d->data_len++;
> > > +     e->type = RTE_TEL_BOOL_VAL;
> > > +     e->value.boolval = val;
> > > +     const size_t bytes = strlcpy(e->name, name, RTE_TEL_MAX_STRING_LEN);
> > > +     return bytes < RTE_TEL_MAX_STRING_LEN ? 0 : E2BIG;
> > > +}
> > > +
> > >   int
> > >   rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> > >               struct rte_tel_data *val, int keep)
> > > @@ -188,6 +221,7 @@ rte_tel_data_add_dict_container(struct rte_tel_data *d, const char *name,
> > >       if (d->type != RTE_TEL_DICT || (val->type != RTE_TEL_ARRAY_U64
> > >                       && val->type != RTE_TEL_ARRAY_INT
> > >                       && val->type != RTE_TEL_ARRAY_STRING
> > > +                     && val->type != RTE_TEL_ARRAY_BOOL
> > >                       && val->type != RTE_TEL_DICT))
> > >               return -EINVAL;
> > >       if (d->data_len >= RTE_TEL_MAX_DICT_ENTRIES)
> > > diff --git a/lib/telemetry/telemetry_data.h b/lib/telemetry/telemetry_data.h
> > > index 26aa28e72c..c840486b18 100644
> > > --- a/lib/telemetry/telemetry_data.h
> > > +++ b/lib/telemetry/telemetry_data.h
> > > @@ -5,6 +5,9 @@
> > >   #ifndef _TELEMETRY_DATA_H_
> > >   #define _TELEMETRY_DATA_H_
> > >
> > > +#include <stdbool.h>
> > > +#include <stdint.h>
> > > +
> > >   #include "rte_telemetry.h"
> > >
> > >   enum tel_container_types {
> > > @@ -15,6 +18,7 @@ enum tel_container_types {
> > >       RTE_TEL_ARRAY_INT,    /** array of signed, 32-bit int values */
> > >       RTE_TEL_ARRAY_U64,    /** array of unsigned 64-bit int values */
> > >       RTE_TEL_ARRAY_CONTAINER, /** array of container structs */
> > > +     RTE_TEL_ARRAY_BOOL,   /** array of boolean values */
> > >   };
> > >
> > >   struct container {
> > > @@ -30,6 +34,7 @@ union tel_value {
> > >       char sval[RTE_TEL_MAX_STRING_LEN];
> > >       int ival;
> > >       uint64_t u64val;
> > > +     bool boolval;
> > >       struct container container;
> > >   };
> > >
> > > diff --git a/lib/telemetry/telemetry_json.h b/lib/telemetry/telemetry_json.h
> > > index e3fae7c30d..c97da97366 100644
> > > --- a/lib/telemetry/telemetry_json.h
> > > +++ b/lib/telemetry/telemetry_json.h
> > > @@ -7,6 +7,7 @@
> > >
> > >   #include <inttypes.h>
> > >   #include <stdarg.h>
> > > +#include <stdbool.h>
> > >   #include <stdio.h>
> > >   #include <rte_common.h>
> > >   #include <rte_telemetry.h>
> > > @@ -159,6 +160,21 @@ rte_tel_json_add_array_u64(char *buf, const int len, const int used,
> > >       return ret == 0 ? used : end + ret;
> > >   }
> > >
> > > +/* Appends a boolean into the JSON array in the provided buffer. */
> > > +static inline int
> > > +rte_tel_json_add_array_bool(char *buf, const int len, const int used,
> > > +             bool val)
> > > +{
> > > +     int ret, end = used - 1; /* strip off final delimiter */
> > > +     if (used <= 2) /* assume empty, since minimum is '[]' */
> > > +             return __json_snprintf(buf, len, "[%s]",
> > > +                             val ? "true" : "false");
> >
> > Have "true" and "false" as #defines, and make a bool_str() helper, which
> > returns the (constant) string form.
> >
> > > +
> > > +     ret = __json_snprintf(buf + end, len - end, ",%s]",
> > > +                     val ? "true" : "false");
> > > +     return ret == 0 ? used : end + ret;
> > > +}
> > > +
> > >   /*
> > >    * Add a new element with raw JSON value to the JSON array stored in the
> > >    * provided buffer.
> > > @@ -193,6 +209,24 @@ rte_tel_json_add_obj_u64(char *buf, const int len, const int used,
> > >       return ret == 0 ? used : end + ret;
> > >   }
> > >
> > > +/**
> > > + * Add a new element with boolean value to the JSON object stored in the
> > > + * provided buffer.
> > > + */
> > > +static inline int
> >
> > I know this pattern isn't introduced in this patch, but why is this
> > function in the header file?
> >
> > > +rte_tel_json_add_obj_bool(char *buf, const int len, const int used,
> > > +             const char *name, bool val)
> > > +{
> > > +     int ret, end = used - 1;
> > > +     if (used <= 2) /* assume empty, since minimum is '{}' */
> >
> > RTE_ASSERT() wouldn't be more helpful? The input buffer is malformed,
> > correct?
> >
> > Either way, the magic '2' should be a #define.
> >
> > > +             return __json_snprintf(buf, len, "{\"%s\":%s}", name,
> > > +                             val ? "true" : "false");
> > > +
> > > +     ret = __json_snprintf(buf + end, len - end, ",\"%s\":%s}",
> > > +                     name, val ? "true" : "false");
> > > +     return ret == 0 ? used : end + ret;
> > > +}
> > > +
> >
> > This looks like cut-and-paste. Merge into one function, parameterized by
> > the start and end delimiter.
> >
> > >   /**
> > >    * Add a new element with int value to the JSON object stored in the
> > >    * provided buffer.
> > > diff --git a/lib/telemetry/version.map b/lib/telemetry/version.map
> > > index 9794f9ea20..88f58d4d89 100644
> > > --- a/lib/telemetry/version.map
> > > +++ b/lib/telemetry/version.map
> > > @@ -19,7 +19,17 @@ DPDK_23 {
> > >       local: *;
> > >   };
> > >
> > > +EXPERIMENTAL {
> > > +     global:
> > > +
> > > +     # added in 22.11
> > > +     rte_tel_data_add_array_bool;
> > > +     rte_tel_data_add_dict_bool;
> > > +};
> > > +
> > >   INTERNAL {
> > > +     global:
> > > +
> > >       rte_telemetry_legacy_register;
> > >       rte_telemetry_init;
> > >   };
> >
> 
> I agree with most of the comments.
> I can follow up on those cleanups, but I don't have the time now.
> 
> I'll let the telemetry maintainers decide on the fate of those patches.
> Thanks.
> 
Well, given the lateness in the release process, if it's a choice between
getting the patches in as-is, vs not getting them in at all, I think having
them in 22.11 is the better choice. While there is cleanup that can be
done, I don't see this patch as making the overall code any worse.

My 2c.

/Bruce

RE: [PATCH v3 1/4] telemetry: support boolean type

[Power, Ciara]

> -----Original Message-----
> From: Richardson, Bruce <bruce.richardson@intel.com>
> Sent: Tuesday 25 October 2022 11:39
> To: David Marchand <david.marchand@redhat.com>
> Cc: Mattias Rönnblom <hofors@lysator.liu.se>; dev@dpdk.org; Morten
> Brørup <mb@smartsharesystems.com>; Power, Ciara
> <ciara.power@intel.com>
> Subject: Re: [PATCH v3 1/4] telemetry: support boolean type
> 
> On Tue, Oct 25, 2022 at 11:43:27AM +0200, David Marchand wrote:
> > On Tue, Oct 25, 2022 at 11:34 AM Mattias Rönnblom
> <hofors@lysator.liu.se> wrote:
> > >
> > > On 2022-10-25 11:00, David Marchand wrote:
> > > > Add the boolean type RTE_TEL_BOOL_VAL for values in arrays and
> dicts.
> > > >
> > > > Signed-off-by: David Marchand <david.marchand@redhat.com>
> > > > Acked-by: Morten Brørup <mb@smartsharesystems.com>
> > > > Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> > > > Acked-by: Ciara Power <ciara.power@intel.com>
> > > > ---
> > > > Changes since v1:
> > > > - fixed doxygen description,
<snip>
> > I'll let the telemetry maintainers decide on the fate of those patches.
> > Thanks.
> >
> Well, given the lateness in the release process, if it's a choice between
> getting the patches in as-is, vs not getting them in at all, I think having them
> in 22.11 is the better choice. While there is cleanup that can be done, I don't
> see this patch as making the overall code any worse.
> 
> My 2c.
> 
> /Bruce

Agreed, it's a good addition to the library, +1 to having it in 22.11

Thanks,
Ciara

[PATCH v3 2/4] telemetry: extend valid command characters

[David Marchand]

The trace framework wants to reference trace point names as keys in
dicts. Those names can contain '.', so add it to the list of valid
characters in the telemetry library.

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Bruce Richardson <bruce.richardson@intel.com>
---
 lib/telemetry/rte_telemetry.h  | 10 +++++-----
 lib/telemetry/telemetry_data.c |  1 +
 2 files changed, 6 insertions(+), 5 deletions(-)

diff --git a/lib/telemetry/rte_telemetry.h b/lib/telemetry/rte_telemetry.h
index e7f6c2ae43..5bd0fc323e 100644
--- a/lib/telemetry/rte_telemetry.h
+++ b/lib/telemetry/rte_telemetry.h
@@ -68,7 +68,7 @@ rte_tel_data_start_array(struct rte_tel_data *d, enum rte_tel_value_type type);
  *
  * 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 '/'
+ * Name strings may only contain alphanumeric characters as well as '_', '/' or '.'
  *
  * @param d
  *   The data structure passed to the callback
@@ -181,7 +181,7 @@ rte_tel_data_add_array_bool(struct rte_tel_data *d, bool x);
  *   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 '/'
+ *   Must contain only alphanumeric characters or the symbols: '_', '/' or '.'
  * @param val
  *   The string to be stored in the dict
  * @return
@@ -200,7 +200,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 '/'
+ *   Must contain only alphanumeric characters or the symbols: '_', '/' or '.'
  * @param val
  *   The number to be stored in the dict
  * @return
@@ -217,7 +217,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 '/'
+ *   Must contain only alphanumeric characters or the symbols: '_', '/' or '.'
  * @param val
  *   The number to be stored in the dict
  * @return
@@ -237,7 +237,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 '/'
+ *   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 13a7ce7034..4f81f71e03 100644
--- a/lib/telemetry/telemetry_data.c
+++ b/lib/telemetry/telemetry_data.c
@@ -119,6 +119,7 @@ valid_name(const char *name)
 			['a' ... 'z'] = 1,
 			['_'] = 1,
 			['/'] = 1,
+			['.'] = 1,
 	};
 	while (*name != '\0') {
 		if ((size_t)*name >= RTE_DIM(allowed) || allowed[(int)*name] == 0)
-- 
2.37.3

RE: [PATCH v3 2/4] telemetry: extend valid command characters

[Power, Ciara]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday 25 October 2022 10:01
> To: dev@dpdk.org
> Cc: Richardson, Bruce <bruce.richardson@intel.com>; Power, Ciara
> <ciara.power@intel.com>
> Subject: [PATCH v3 2/4] telemetry: extend valid command characters
> 
> The trace framework wants to reference trace point names as keys in dicts.
> Those names can contain '.', so add it to the list of valid characters in the
> telemetry library.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Bruce Richardson <bruce.richardson@intel.com>
> ---
>  lib/telemetry/rte_telemetry.h  | 10 +++++-----  lib/telemetry/telemetry_data.c |
> 1 +
>  2 files changed, 6 insertions(+), 5 deletions(-)

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

[PATCH v3 3/4] trace: enable trace operations via telemetry

[David Marchand]

Register telemetry commands to list and configure trace points and later
save traces for a running DPDK application.

Example with testpmd running with two net/null ports (startup command
from devtools/test-null.sh):

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": false,
    "lib.ethdev.rxq.setup": false,
    "lib.ethdev.txq.setup": false,
    "lib.ethdev.start": false,
    "lib.ethdev.stop": false,
    "lib.ethdev.close": false,
    "lib.ethdev.rx.burst": false,
    "lib.ethdev.tx.burst": false}}

--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 2}}
--> /trace/enable,lib.ethdev.st*
{"/trace/enable": {"Count": 0}}

--> /trace/list,lib.ethdev.*
{"/trace/list": {"lib.ethdev.configure": false,
    "lib.ethdev.rxq.setup": false,
    "lib.ethdev.txq.setup": false,
    "lib.ethdev.start": true,
    "lib.ethdev.stop": true,
    "lib.ethdev.close": false,
    "lib.ethdev.rx.burst": false,
    "lib.ethdev.tx.burst": false}}

testpmd> stop
...
testpmd> port stop all
...
testpmd> port start all
...
testpmd> start
...

--> /trace/save
{"/trace/save": {"Status": "OK",
    "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}

$ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
[10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
[10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
[10:51:40.449359774] (+4.219479523) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
[10:51:40.449377877] (+0.000018103) lib.ethdev.start:
    { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }

--> /trace/disable,*
{"/trace/disable": {"Count": 2}}

Signed-off-by: David Marchand <david.marchand@redhat.com>
Acked-by: Jerin Jacob <jerinj@marvell.com>
---
Changes since v2:
- separated telemetry change in a dedicated patch,
- switched to boolean types,

Changes since v1:
- added a note in the documentation,

---
 doc/guides/prog_guide/trace_lib.rst | 39 +++++++++++++++
 lib/eal/common/eal_common_trace.c   | 78 +++++++++++++++++++++++++++++
 2 files changed, 117 insertions(+)

diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
index 9a8f38073d..d0a6bdae4c 100644
--- a/doc/guides/prog_guide/trace_lib.rst
+++ b/doc/guides/prog_guide/trace_lib.rst
@@ -233,6 +233,45 @@ This section steps you through the details of generating trace and viewing it.
 
     babeltrace $HOME/dpdk-traces/rte-yyyy-mm-dd-[AP]M-hh-mm-ss/
 
+Configuring traces on a running DPDK application
+------------------------------------------------
+
+The DPDK trace library can be configured using the Telemetry interface.
+
+Examples::
+
+  --> /trace/list,lib.ethdev.*
+  {"/trace/list": {"lib.ethdev.configure": false,
+      "lib.ethdev.rxq.setup": false,
+      "lib.ethdev.txq.setup": false,
+      "lib.ethdev.start": false,
+      "lib.ethdev.stop": false,
+      "lib.ethdev.close": false,
+      "lib.ethdev.rx.burst": false,
+      "lib.ethdev.tx.burst": false}}
+
+  --> /trace/enable,lib.ethdev.st*
+  {"/trace/enable": {"Count": 2}}
+  --> /trace/enable,lib.ethdev.st*
+  {"/trace/enable": {"Count": 0}}
+
+  --> /trace/list,lib.ethdev.*
+  {"/trace/list": {"lib.ethdev.configure": false,
+      "lib.ethdev.rxq.setup": false,
+      "lib.ethdev.txq.setup": false,
+      "lib.ethdev.start": true,
+      "lib.ethdev.stop": true,
+      "lib.ethdev.close": false,
+      "lib.ethdev.rx.burst": false,
+      "lib.ethdev.tx.burst": false}}
+
+  --> /trace/save
+  {"/trace/save": {"Status": "OK",
+      "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
+
+For more information on how to use the Telemetry interface, see
+the :doc:`../howto/telemetry`.
+
 Implementation details
 ----------------------
 
diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index 5caaac8e59..bb26af618a 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -12,6 +12,7 @@
 #include <rte_lcore.h>
 #include <rte_per_lcore.h>
 #include <rte_string_fns.h>
+#include <rte_telemetry.h>
 
 #include "eal_trace.h"
 
@@ -521,3 +522,80 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
 
 	return -rte_errno;
 }
+
+static int
+trace_telemetry_enable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, true);
+	rte_tel_data_add_dict_int(d, "Count",
+		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);
+	return 0;
+
+}
+
+static int
+trace_telemetry_disable(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	unsigned int count;
+
+	if (params == NULL || strlen(params) == 0)
+		return -1;
+	rte_tel_data_start_dict(d);
+	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
+	rte_trace_pattern(params, false);
+	rte_tel_data_add_dict_int(d, "Count",
+		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
+	return 0;
+
+}
+
+static int
+trace_telemetry_list(const char *cmd __rte_unused,
+	const char *params, struct rte_tel_data *d)
+{
+	struct trace_point *tp;
+
+	rte_tel_data_start_dict(d);
+	STAILQ_FOREACH(tp, &tp_list, next) {
+		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
+			continue;
+
+		rte_tel_data_add_dict_bool(d, tp->name,
+			rte_trace_point_is_enabled(tp->handle));
+	}
+
+	return 0;
+}
+
+static int
+trace_telemetry_save(const char *cmd __rte_unused,
+	const char *params __rte_unused, struct rte_tel_data *d)
+{
+	struct trace *trace = trace_obj_get();
+
+	rte_tel_data_start_dict(d);
+	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
+	rte_tel_data_add_dict_string(d, "Path", trace->dir);
+
+	return 0;
+}
+
+RTE_INIT(trace_telemetry)
+{
+	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
+		"Enable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
+		"Disable trace points matching the provided pattern. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
+		"List trace points. Parameters: string pattern.");
+	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
+		"Save current traces. Takes no parameter.");
+}
-- 
2.37.3

Re: [PATCH v3 3/4] trace: enable trace operations via telemetry

[Mattias Rönnblom]

On 2022-10-25 11:00, David Marchand wrote:
> Register telemetry commands to list and configure trace points and later
> save traces for a running DPDK application.
> 
> Example with testpmd running with two net/null ports (startup command
> from devtools/test-null.sh):
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": false,
>      "lib.ethdev.rxq.setup": false,
>      "lib.ethdev.txq.setup": false,
>      "lib.ethdev.start": false,
>      "lib.ethdev.stop": false,
>      "lib.ethdev.close": false,
>      "lib.ethdev.rx.burst": false,
>      "lib.ethdev.tx.burst": false}}
> 
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 2}}
> --> /trace/enable,lib.ethdev.st*
> {"/trace/enable": {"Count": 0}}
> 
> --> /trace/list,lib.ethdev.*
> {"/trace/list": {"lib.ethdev.configure": false,
>      "lib.ethdev.rxq.setup": false,
>      "lib.ethdev.txq.setup": false,
>      "lib.ethdev.start": true,
>      "lib.ethdev.stop": true,
>      "lib.ethdev.close": false,
>      "lib.ethdev.rx.burst": false,
>      "lib.ethdev.tx.burst": false}}
> 
> testpmd> stop
> ...
> testpmd> port stop all
> ...
> testpmd> port start all
> ...
> testpmd> start
> ...
> 
> --> /trace/save
> {"/trace/save": {"Status": "OK",
>      "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
> 
> $ babeltrace .../dpdk-traces/rte-2022-10-12-AM-10-51-48
> [10:51:36.229878723] (+?.?????????) lib.ethdev.stop:
>      { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0, ret = 0 }
> [10:51:36.229880251] (+0.000001528) lib.ethdev.stop:
>      { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1, ret = 0 }
> [10:51:40.449359774] (+4.219479523) lib.ethdev.start:
>      { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x0 }
> [10:51:40.449377877] (+0.000018103) lib.ethdev.start:
>      { cpu_id = 0x0, name = "dpdk-testpmd" }, { port_id = 0x1 }
> 
> --> /trace/disable,*
> {"/trace/disable": {"Count": 2}}
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> Acked-by: Jerin Jacob <jerinj@marvell.com>
> ---
> Changes since v2:
> - separated telemetry change in a dedicated patch,
> - switched to boolean types,
> 
> Changes since v1:
> - added a note in the documentation,
> 
> ---
>   doc/guides/prog_guide/trace_lib.rst | 39 +++++++++++++++
>   lib/eal/common/eal_common_trace.c   | 78 +++++++++++++++++++++++++++++
>   2 files changed, 117 insertions(+)
> 
> diff --git a/doc/guides/prog_guide/trace_lib.rst b/doc/guides/prog_guide/trace_lib.rst
> index 9a8f38073d..d0a6bdae4c 100644
> --- a/doc/guides/prog_guide/trace_lib.rst
> +++ b/doc/guides/prog_guide/trace_lib.rst
> @@ -233,6 +233,45 @@ This section steps you through the details of generating trace and viewing it.
>   
>       babeltrace $HOME/dpdk-traces/rte-yyyy-mm-dd-[AP]M-hh-mm-ss/
>   
> +Configuring traces on a running DPDK application
> +------------------------------------------------
> +
> +The DPDK trace library can be configured using the Telemetry interface.
> +
> +Examples::
> +
> +  --> /trace/list,lib.ethdev.*
> +  {"/trace/list": {"lib.ethdev.configure": false,
> +      "lib.ethdev.rxq.setup": false,
> +      "lib.ethdev.txq.setup": false,
> +      "lib.ethdev.start": false,
> +      "lib.ethdev.stop": false,
> +      "lib.ethdev.close": false,
> +      "lib.ethdev.rx.burst": false,
> +      "lib.ethdev.tx.burst": false}}
> +
> +  --> /trace/enable,lib.ethdev.st*
> +  {"/trace/enable": {"Count": 2}}
> +  --> /trace/enable,lib.ethdev.st*
> +  {"/trace/enable": {"Count": 0}}
> +
> +  --> /trace/list,lib.ethdev.*
> +  {"/trace/list": {"lib.ethdev.configure": false,
> +      "lib.ethdev.rxq.setup": false,
> +      "lib.ethdev.txq.setup": false,
> +      "lib.ethdev.start": true,
> +      "lib.ethdev.stop": true,
> +      "lib.ethdev.close": false,
> +      "lib.ethdev.rx.burst": false,
> +      "lib.ethdev.tx.burst": false}}
> +
> +  --> /trace/save
> +  {"/trace/save": {"Status": "OK",
> +      "Path": ".../dpdk-traces/rte-2022-10-12-AM-10-51-48"}}
> +
> +For more information on how to use the Telemetry interface, see
> +the :doc:`../howto/telemetry`.
> +
>   Implementation details
>   ----------------------
>   
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index 5caaac8e59..bb26af618a 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -12,6 +12,7 @@
>   #include <rte_lcore.h>
>   #include <rte_per_lcore.h>
>   #include <rte_string_fns.h>
> +#include <rte_telemetry.h>
>   
>   #include "eal_trace.h"
>   
> @@ -521,3 +522,80 @@ __rte_trace_point_register(rte_trace_point_t *handle, const char *name,
>   
>   	return -rte_errno;
>   }
> +
> +static int
> +trace_telemetry_enable(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	unsigned int count;

status is uint32_t.

> +
> +	if (params == NULL || strlen(params) == 0)
> +		return -1;
> +	rte_tel_data_start_dict(d);
> +	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
> +	rte_trace_pattern(params, true);
> +	rte_tel_data_add_dict_int(d, "Count",
> +		__atomic_load_n(&trace.status, __ATOMIC_RELAXED) - count);

Why "Count"? From what I could tell, trace.status tells if tracing is 
enabled or not.

I also don't understand why &trace.status is loaded twice, especially in 
the light that future compilers, that more heavily optimize atomics, 
might merge those two loads into one, resulting in something equivalent to:

rte_tel_data_add_dict_int(d, "Count", 0);

Is one of the loads supposed to be a store (or, lock add, rather), 
enabling the tracing?

Might be worth to wrap all the atomics in functions, making the code 
more clear.

> +	return 0;
> +
> +}
> +
> +static int
> +trace_telemetry_disable(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	unsigned int count;
> +
> +	if (params == NULL || strlen(params) == 0)
> +		return -1;
> +	rte_tel_data_start_dict(d);
> +	count = __atomic_load_n(&trace.status, __ATOMIC_RELAXED);
> +	rte_trace_pattern(params, false);
> +	rte_tel_data_add_dict_int(d, "Count",
> +		count - __atomic_load_n(&trace.status, __ATOMIC_RELAXED));
> +	return 0;
> +
> +}
> +
> +static int
> +trace_telemetry_list(const char *cmd __rte_unused,
> +	const char *params, struct rte_tel_data *d)
> +{
> +	struct trace_point *tp;
> +
> +	rte_tel_data_start_dict(d);
> +	STAILQ_FOREACH(tp, &tp_list, next) {
> +		if (params != NULL && fnmatch(params, tp->name, 0) != 0)
> +			continue;
> +
> +		rte_tel_data_add_dict_bool(d, tp->name,
> +			rte_trace_point_is_enabled(tp->handle));
> +	}
> +
> +	return 0;
> +}
> +
> +static int
> +trace_telemetry_save(const char *cmd __rte_unused,
> +	const char *params __rte_unused, struct rte_tel_data *d)
> +{
> +	struct trace *trace = trace_obj_get();
> +
> +	rte_tel_data_start_dict(d);
> +	rte_tel_data_add_dict_string(d, "Status", rte_trace_save() == 0 ? "OK" : "KO");
> +	rte_tel_data_add_dict_string(d, "Path", trace->dir);
> +
> +	return 0;
> +}
> +
> +RTE_INIT(trace_telemetry)
> +{
> +	rte_telemetry_register_cmd("/trace/enable", trace_telemetry_enable,
> +		"Enable trace points matching the provided pattern. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/disable", trace_telemetry_disable,
> +		"Disable trace points matching the provided pattern. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/list", trace_telemetry_list,
> +		"List trace points. Parameters: string pattern.");
> +	rte_telemetry_register_cmd("/trace/save", trace_telemetry_save,
> +		"Save current traces. Takes no parameter.");
> +}

[PATCH v3 4/4] trace: create new directory for each trace dump

[David Marchand]

Rather than always overwrite the content of the trace directory, create
new directories for each call to rte_trace_save().

In the unlikely event that multiple rte_trace_save() gets called many
times in a single second, try to suffix the name with a digit.

Signed-off-by: David Marchand <david.marchand@redhat.com>
---
 lib/eal/common/eal_common_trace.c       |  4 ++
 lib/eal/common/eal_common_trace_utils.c | 89 +++++++++++--------------
 lib/eal/common/eal_trace.h              |  1 +
 3 files changed, 45 insertions(+), 49 deletions(-)

diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
index bb26af618a..7d411b336e 100644
--- a/lib/eal/common/eal_common_trace.c
+++ b/lib/eal/common/eal_common_trace.c
@@ -92,6 +92,10 @@ eal_trace_fini(void)
 	trace_mem_free();
 	trace_metadata_destroy();
 	eal_trace_args_free();
+	free(trace.rootdir);
+	trace.rootdir = NULL;
+	free(trace.dir);
+	trace.dir = NULL;
 }
 
 bool
diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
index 8561a0e198..6256ef6703 100644
--- a/lib/eal/common/eal_common_trace_utils.c
+++ b/lib/eal/common/eal_common_trace_utils.c
@@ -87,9 +87,10 @@ trace_uuid_generate(void)
 }
 
 static int
-trace_session_name_generate(char **trace_dir)
+trace_dir_name_generate(char **trace_dir)
 {
 	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
+	struct trace *trace = trace_obj_get();
 	struct tm *tm_result;
 	time_t tm;
 
@@ -106,7 +107,8 @@ trace_session_name_generate(char **trace_dir)
 		goto fail;
 	}
 
-	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
+	if (asprintf(trace_dir, "%s/%s-%s", trace->rootdir,
+			eal_get_hugefile_prefix(), date) == -1)
 		goto fail;
 
 	return 0;
@@ -115,21 +117,6 @@ trace_session_name_generate(char **trace_dir)
 	return -1;
 }
 
-static int
-trace_dir_update(const char *str)
-{
-	struct trace *trace = trace_obj_get();
-	char *dir;
-	int rc;
-
-	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
-	if (rc != -1) {
-		free(trace->dir);
-		trace->dir = dir;
-	}
-	return rc;
-}
-
 int
 eal_trace_args_save(const char *val)
 {
@@ -240,17 +227,16 @@ eal_trace_mode_args_save(const char *val)
 int
 eal_trace_dir_args_save(char const *val)
 {
-	char *dir_path;
-	int rc;
+	struct trace *trace = trace_obj_get();
 
-	if (asprintf(&dir_path, "%s/", val) == -1) {
+	free(trace->rootdir);
+	trace->rootdir = strdup(val);
+	if (trace->rootdir == NULL) {
 		trace_err("failed to copy directory: %s", strerror(errno));
 		return -ENOMEM;
 	}
 
-	rc = trace_dir_update(dir_path);
-	free(dir_path);
-	return rc;
+	return 0;
 }
 
 int
@@ -293,7 +279,7 @@ trace_dir_default_path_get(char **dir_path)
 	}
 
 	/* Append dpdk-traces to directory */
-	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
+	if (asprintf(dir_path, "%s/dpdk-traces", home_dir) == -1)
 		return -ENOMEM;
 
 	return 0;
@@ -303,14 +289,11 @@ static int
 trace_mkdir(void)
 {
 	struct trace *trace = trace_obj_get();
-	static bool already_done;
-	char *session;
+	unsigned int try;
+	char *trace_dir;
 	int rc;
 
-	if (already_done)
-		return 0;
-
-	if (trace->dir == NULL) {
+	if (trace->rootdir == NULL) {
 		char *dir_path;
 
 		rc = trace_dir_default_path_get(&dir_path);
@@ -318,38 +301,46 @@ trace_mkdir(void)
 			trace_err("fail to get default path");
 			return rc;
 		}
-
-		rc = trace_dir_update(dir_path);
-		free(dir_path);
-		if (rc < 0)
-			return rc;
+		trace->rootdir = dir_path;
 	}
 
 	/* Create the path if it t exist, no "mkdir -p" available here */
-	rc = mkdir(trace->dir, 0700);
+	rc = mkdir(trace->rootdir, 0700);
 	if (rc < 0 && errno != EEXIST) {
-		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
+		trace_err("mkdir %s failed [%s]", trace->rootdir, strerror(errno));
 		rte_errno = errno;
 		return -rte_errno;
 	}
 
-	rc = trace_session_name_generate(&session);
-	if (rc < 0)
-		return rc;
-	rc = trace_dir_update(session);
-	free(session);
+	rc = trace_dir_name_generate(&trace_dir);
 	if (rc < 0)
 		return rc;
-
-	rc = mkdir(trace->dir, 0700);
-	if (rc < 0) {
-		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
-		rte_errno = errno;
-		return -rte_errno;
+	rc = mkdir(trace_dir, 0700);
+	if (rc == 0)
+		goto out;
+
+	/* 1000 tries are definitely enough :-) */
+	for (try = 1; try < 1000; try++) {
+		char *trace_dir_suffix;
+
+		if (asprintf(&trace_dir_suffix, "%s.%u", trace_dir, try) == -1)
+			continue;
+		rc = mkdir(trace_dir_suffix, 0700);
+		if (rc == 0) {
+			free(trace_dir);
+			trace_dir = trace_dir_suffix;
+			goto out;
+		}
+		free(trace_dir_suffix);
 	}
 
+	trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
+	rte_errno = errno;
+	return -rte_errno;
+out:
+	free(trace->dir);
+	trace->dir = trace_dir;
 	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
-	already_done = true;
 	return 0;
 }
 
diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
index d66bcfe198..8991088253 100644
--- a/lib/eal/common/eal_trace.h
+++ b/lib/eal/common/eal_trace.h
@@ -48,6 +48,7 @@ struct trace_arg {
 };
 
 struct trace {
+	char *rootdir;
 	char *dir;
 	int register_errno;
 	uint32_t status;
-- 
2.37.3

Re: [PATCH v3 4/4] trace: create new directory for each trace dump

[Mattias Rönnblom]

On 2022-10-25 11:00, David Marchand wrote:
> Rather than always overwrite the content of the trace directory, create
> new directories for each call to rte_trace_save().
> 
> In the unlikely event that multiple rte_trace_save() gets called many
> times in a single second, try to suffix the name with a digit.
> 
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
>   lib/eal/common/eal_common_trace.c       |  4 ++
>   lib/eal/common/eal_common_trace_utils.c | 89 +++++++++++--------------
>   lib/eal/common/eal_trace.h              |  1 +
>   3 files changed, 45 insertions(+), 49 deletions(-)
> 
> diff --git a/lib/eal/common/eal_common_trace.c b/lib/eal/common/eal_common_trace.c
> index bb26af618a..7d411b336e 100644
> --- a/lib/eal/common/eal_common_trace.c
> +++ b/lib/eal/common/eal_common_trace.c
> @@ -92,6 +92,10 @@ eal_trace_fini(void)
>   	trace_mem_free();
>   	trace_metadata_destroy();
>   	eal_trace_args_free();
> +	free(trace.rootdir);
> +	trace.rootdir = NULL;
> +	free(trace.dir);
> +	trace.dir = NULL;
>   }
>   
>   bool
> diff --git a/lib/eal/common/eal_common_trace_utils.c b/lib/eal/common/eal_common_trace_utils.c
> index 8561a0e198..6256ef6703 100644
> --- a/lib/eal/common/eal_common_trace_utils.c
> +++ b/lib/eal/common/eal_common_trace_utils.c
> @@ -87,9 +87,10 @@ trace_uuid_generate(void)
>   }
>   
>   static int
> -trace_session_name_generate(char **trace_dir)
> +trace_dir_name_generate(char **trace_dir)
>   {
>   	char date[sizeof("YYYY-mm-dd-AM-HH-MM-SS")];
> +	struct trace *trace = trace_obj_get();
>   	struct tm *tm_result;
>   	time_t tm;
>   
> @@ -106,7 +107,8 @@ trace_session_name_generate(char **trace_dir)
>   		goto fail;
>   	}
>   
> -	if (asprintf(trace_dir, "%s-%s", eal_get_hugefile_prefix(), date) == -1)
> +	if (asprintf(trace_dir, "%s/%s-%s", trace->rootdir,
> +			eal_get_hugefile_prefix(), date) == -1)
>   		goto fail;
>   
>   	return 0;
> @@ -115,21 +117,6 @@ trace_session_name_generate(char **trace_dir)
>   	return -1;
>   }
>   
> -static int
> -trace_dir_update(const char *str)
> -{
> -	struct trace *trace = trace_obj_get();
> -	char *dir;
> -	int rc;
> -
> -	rc = asprintf(&dir, "%s%s", trace->dir != NULL ? trace->dir : "", str);
> -	if (rc != -1) {
> -		free(trace->dir);
> -		trace->dir = dir;
> -	}
> -	return rc;
> -}
> -
>   int
>   eal_trace_args_save(const char *val)
>   {
> @@ -240,17 +227,16 @@ eal_trace_mode_args_save(const char *val)
>   int
>   eal_trace_dir_args_save(char const *val)
>   {
> -	char *dir_path;
> -	int rc;
> +	struct trace *trace = trace_obj_get();
>   
> -	if (asprintf(&dir_path, "%s/", val) == -1) {
> +	free(trace->rootdir);
> +	trace->rootdir = strdup(val);
> +	if (trace->rootdir == NULL) {
>   		trace_err("failed to copy directory: %s", strerror(errno));
>   		return -ENOMEM;
>   	}
>   
> -	rc = trace_dir_update(dir_path);
> -	free(dir_path);
> -	return rc;
> +	return 0;
>   }
>   
>   int
> @@ -293,7 +279,7 @@ trace_dir_default_path_get(char **dir_path)
>   	}
>   
>   	/* Append dpdk-traces to directory */
> -	if (asprintf(dir_path, "%s/dpdk-traces/", home_dir) == -1)
> +	if (asprintf(dir_path, "%s/dpdk-traces", home_dir) == -1)
>   		return -ENOMEM;
>   
>   	return 0;
> @@ -303,14 +289,11 @@ static int
>   trace_mkdir(void)
>   {
>   	struct trace *trace = trace_obj_get();
> -	static bool already_done;
> -	char *session;
> +	unsigned int try;
> +	char *trace_dir;
>   	int rc;
>   
> -	if (already_done)
> -		return 0;
> -
> -	if (trace->dir == NULL) {
> +	if (trace->rootdir == NULL) {
>   		char *dir_path;
>   
>   		rc = trace_dir_default_path_get(&dir_path);
> @@ -318,38 +301,46 @@ trace_mkdir(void)
>   			trace_err("fail to get default path");
>   			return rc;
>   		}
> -
> -		rc = trace_dir_update(dir_path);
> -		free(dir_path);
> -		if (rc < 0)
> -			return rc;
> +		trace->rootdir = dir_path;
>   	}
>   
>   	/* Create the path if it t exist, no "mkdir -p" available here */
> -	rc = mkdir(trace->dir, 0700);
> +	rc = mkdir(trace->rootdir, 0700);
>   	if (rc < 0 && errno != EEXIST) {
> -		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> +		trace_err("mkdir %s failed [%s]", trace->rootdir, strerror(errno));
>   		rte_errno = errno;
>   		return -rte_errno;
>   	}
>   
> -	rc = trace_session_name_generate(&session);
> -	if (rc < 0)
> -		return rc;
> -	rc = trace_dir_update(session);
> -	free(session);
> +	rc = trace_dir_name_generate(&trace_dir);
>   	if (rc < 0)
>   		return rc;
> -
> -	rc = mkdir(trace->dir, 0700);
> -	if (rc < 0) {
> -		trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> -		rte_errno = errno;
> -		return -rte_errno;
> +	rc = mkdir(trace_dir, 0700);
> +	if (rc == 0)
> +		goto out;
> +
> +	/* 1000 tries are definitely enough :-) */

Do you need an upper bound at all?

Is there a point in retrying for anything other than EEXIST?

> +	for (try = 1; try < 1000; try++) {
> +		char *trace_dir_suffix;
> +
> +		if (asprintf(&trace_dir_suffix, "%s.%u", trace_dir, try) == -1)
> +			continue;
> +		rc = mkdir(trace_dir_suffix, 0700);
> +		if (rc == 0) {
> +			free(trace_dir);
> +			trace_dir = trace_dir_suffix;
> +			goto out;
> +		}
> +		free(trace_dir_suffix);
>   	}
>   
> +	trace_err("mkdir %s failed [%s]", trace->dir, strerror(errno));
> +	rte_errno = errno;
> +	return -rte_errno;
> +out:
> +	free(trace->dir);
> +	trace->dir = trace_dir;
>   	RTE_LOG(INFO, EAL, "Trace dir: %s\n", trace->dir);
> -	already_done = true;
>   	return 0;
>   }
>   
> diff --git a/lib/eal/common/eal_trace.h b/lib/eal/common/eal_trace.h
> index d66bcfe198..8991088253 100644
> --- a/lib/eal/common/eal_trace.h
> +++ b/lib/eal/common/eal_trace.h
> @@ -48,6 +48,7 @@ struct trace_arg {
>   };
>   
>   struct trace {
> +	char *rootdir;
>   	char *dir;
>   	int register_errno;
>   	uint32_t status;