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

[Bruce Richardson <bruce.richardson@intel.com>]

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