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

[David Marchand <david.marchand@redhat.com>]

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