RE: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a struct

[Jerin Jacob <jerinj@marvell.com>]

> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, February 18, 2025 7:58 PM
> To: Sunil Kumar Kori <skori@marvell.com>; Jerin Jacob <jerinj@marvell.com>
> Cc: dev@dpdk.org; fengchengwen@huawei.com; Kevin Laatz
> <kevin.laatz@intel.com>; Bruce Richardson <bruce.richardson@intel.com>;
> Tyler Retzlaff <roretzla@linux.microsoft.com>
> Subject: Re: [EXTERNAL] [PATCH v3 4/6] trace: support dumping binary inside a
> struct
> 
> On Wed, Feb 12, 2025 at 6: 14 AM Sunil Kumar Kori <skori@ marvell. com>
> wrote: > > > On Tue, Feb 11, 2025 at 9: 53 AM Sunil Kumar Kori
> <skori@ marvell. com> > > wrote: > > > > > > > diff --git
> a/lib/eal/common/eal_common_trace_ctf. c 
> On Wed, Feb 12, 2025 at 6:14 AM Sunil Kumar Kori <skori@marvell.com>
> wrote:
> >
> > > On Tue, Feb 11, 2025 at 9:53 AM Sunil Kumar Kori <skori@marvell.com>
> > > wrote:
> > > >
> > > > > diff --git a/lib/eal/common/eal_common_trace_ctf.c
> > > > > b/lib/eal/common/eal_common_trace_ctf.c
> > > > > index 6bc8bb9036..d9b307e076 100644
> > > > > --- a/lib/eal/common/eal_common_trace_ctf.c
> > > > > +++ b/lib/eal/common/eal_common_trace_ctf.c
> > > > > @@ -378,6 +378,9 @@ char *trace_metadata_fixup_field(const char
> > > *field)
> > > > >               "->",
> > > > >               "*",
> > > > >               " ",
> > > > > +             "&",
> > > > > +             "(",
> > > > > +             ")",
> > > > Adding brackets makes token names a bit complex. Same name is used
> > > > in metadata file to dump the traces to the user. With this complex
> > > > name, user might not understand the purpose of that information.
> > > >
> > > > For example, _conf_src_port_pcie_sizeof_uint64_t_ is created in
> > > > metafile and same will be dumped. But with this User might not get
> > > > that
> > > which information is provided.
> > >
> > > In practice, as there is no other documentation for a trace point
> > > arguments, a user needs to read the trace point definitions.
> > > So it seems trivial to me to link a variable name in the trace point
> > > emitter, and the metadata in the trace files.
> > >
> > > >
> > > > This is the reason; we followed the existing naming convention
> > > > which is user
> > > friendly.
> > >
> > > User friendly? I don't see how this is different with '.' and '->'.
> > In general, structure fields are given a proper name to represent the purpose.
> > When we use it directly in trace point using '.' or '->' then it remains a
> meaningful name.
> > Adding more tokens in name, is making them complex and deviating from
> there meaning.
> >
> > I am not saying that the mentioned support should not be there. I am
> > just trying to convey that If it is possible to make meaningful names, then that
> will be more helpful.
> 
> Hard to preserve such information given the limitations of the C parser (which
> seems to apply to the CTF format).
> I still think that interpretation of the metadata in the traces require looking at
> the source code, which means that the "readability"
> objection is weak.
> 
> Jerin, opinion please.

One side, it reduces the number of lines of code, leveraging all CTF syntax and other side it traces string becomes complex.
IMO, We can go with new patch scheme and document the new syntax so that someone parsing babetrace or tracecompass
can understand the meaning of _conf_src_port_pcie_sizeof_uint64_t_ (as example)



> 
> 
> --
> David Marchand