Re: [EXT] Re: [PATCH v5 1/1] devtools: add tracepoint check in checkpatch

[Thomas Monjalon <thomas@monjalon.net>]

21/08/2023 16:46, Morten Brørup:
> > From: Ankur Dwivedi [mailto:adwivedi@marvell.com]
> > Sent: Monday, 21 August 2023 15.54
> > 
> > >From: Stephen Hemminger <stephen@networkplumber.org>
> > >Sent: Thursday, May 18, 2023 9:04 PM
> > >
> > >----------------------------------------------------------------------
> > >On Thu, 18 May 2023 13:45:29 +0000
> > >Ankur Dwivedi <adwivedi@marvell.com> wrote:
> > >
> > >> >From: Ankur Dwivedi <adwivedi@marvell.com>
> > >> >Sent: Tuesday, March 7, 2023 5:35 PM
> > >> >
> > >> >This patch adds a validation in checkpatch tool, to check if a
> > >> >tracepoint is present in any new function added in cryptodev,
> > ethdev,
> > >> >eventdev and mempool library.
> > >> >
> > >> >In this patch, the build_map_changes function is copied from
> > >> >check-symbol- change.sh to check-tracepoint.sh. The
> > >> >check-tracepoint.sh script uses build_map_changes function to create
> > a
> > >map of functions.
> > >> >In the map, the newly added functions, added in the experimental
> > >> >section are identified and their definition are checked for the
> > >> >presence of tracepoint. The checkpatch return error if the
> > tracepoint is not
> > >present.
> > >> >
> > >> >For functions for which trace is not needed, they can be added to
> > >> >devtools/trace-skiplist.txt file. The above tracepoint check will be
> > >> >skipped for them.
> > >> >
> > >> >Signed-off-by: Ankur Dwivedi <adwivedi@marvell.com>
> > >
> > >Given the amount of string processing, it would be more readable in
> > python.
> > >That is not a show stopper, just a suggestion.
> > 
> > Hi Thomas,
> > 
> > Please let me know if the shell script in this patch is fine or would a
> > python implementation would be more preferable.

In general, I wonder how much the check is useful compared to the complexity.


> The bigger question is: Do we really want to change tracepoints in functions from opt-in to opt-out?

Yes that's the question: should traces be mandatory in some libs?

> In my opinion, opt-in for trace is more appropriate.

There was some work to add traces everywhere in few libs, so why not maintaining this state?
I don't really like adding a skip list as one more burden for future authors.


> Nonetheless, having a tool to check for tracepoint presence might still be useful for library reviewers and maintainers. And such a tool might be useful for any library, not just the few libraries suggested by this patch.