mbox

[v5,0/1] devtools: add tracepoint check in checkpatch

Message ID 20230307120514.2774917-1-adwivedi@marvell.com (mailing list archive)
Headers

Message

Ankur Dwivedi March 7, 2023, 12:05 p.m. UTC
  This patch series adds a validation in checkpatch tool to check if
tracepoint is present in any new function added in ethdev, eventdev
cryptodev and mempool library.

v5:
 - Copied the build_map_changes function from check-symbol-change.sh to
   check-tracepoint.sh.
 - Added eventdev, cryptodev and mempool in libdir in check-tracepoint.sh.
 
v4:
 - Rebased on the recent next-net branch.
 - Refined logic to find function definition.
 - Updated year in the license in devtools/check-tracepoint.sh.
 - Removed cryptodev, added ethdev in libdir in
   devtools/check-tracepoint.sh. 

v3:
 - Split the v2 patch into 2 patches.
 - The file common-func.sh is renamed to build-symbol-map.sh.
 - Removed check-tracepoint.py file.
 - Code improvements in check-tracepoint.sh.

v2:
 - Add check for parent directory.

Ankur Dwivedi (1):
  devtools: add tracepoint check in checkpatch

 devtools/check-tracepoint.sh | 223 +++++++++++++++++++++++++++++++++++
 devtools/checkpatches.sh     |   9 ++
 devtools/trace-skiplist.txt  |   0
 3 files changed, 232 insertions(+)
 create mode 100755 devtools/check-tracepoint.sh
 create mode 100644 devtools/trace-skiplist.txt
  

Comments

Thomas Monjalon Nov. 28, 2023, 1:18 p.m. UTC | #1
07/03/2023 13:05, Ankur Dwivedi:
> This patch series adds a validation in checkpatch tool to check if
> tracepoint is present in any new function added in ethdev, eventdev
> cryptodev and mempool library.
> 
> v5:
>  - Copied the build_map_changes function from check-symbol-change.sh to
>    check-tracepoint.sh.
>  - Added eventdev, cryptodev and mempool in libdir in check-tracepoint.sh.

Why did you decide to copy the function in v5,
instead of having a common file usable by different scripts?
  
Ankur Dwivedi Nov. 28, 2023, 2:07 p.m. UTC | #2
>07/03/2023 13:05, Ankur Dwivedi:
>> This patch series adds a validation in checkpatch tool to check if
>> tracepoint is present in any new function added in ethdev, eventdev
>> cryptodev and mempool library.
>>
>> v5:
>>  - Copied the build_map_changes function from check-symbol-change.sh to
>>    check-tracepoint.sh.
>>  - Added eventdev, cryptodev and mempool in libdir in check-tracepoint.sh.
>
>Why did you decide to copy the function in v5, instead of having a common
>file usable by different scripts?
>
There was comments in v2 of the patch that common scripts may not work well and to keep the scripts specialized.
  
Thomas Monjalon Nov. 28, 2023, 3:55 p.m. UTC | #3
28/11/2023 15:07, Ankur Dwivedi:
> >07/03/2023 13:05, Ankur Dwivedi:
> >> This patch series adds a validation in checkpatch tool to check if
> >> tracepoint is present in any new function added in ethdev, eventdev
> >> cryptodev and mempool library.
> >>
> >> v5:
> >>  - Copied the build_map_changes function from check-symbol-change.sh to
> >>    check-tracepoint.sh.
> >>  - Added eventdev, cryptodev and mempool in libdir in check-tracepoint.sh.
> >
> >Why did you decide to copy the function in v5, instead of having a common
> >file usable by different scripts?
> >
> There was comments in v2 of the patch that common scripts may not work well and to keep the scripts specialized.

I meant you can have a common file specialized in symbols.
In general, you should reply, establish a discussion, so we share the same understanding.
  
Ankur Dwivedi Nov. 30, 2023, 5:56 a.m. UTC | #4
>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Tuesday, November 28, 2023 9:25 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>Subject: Re: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in
>checkpatch
>
>28/11/2023 15:07, Ankur Dwivedi:
>> >07/03/2023 13:05, Ankur Dwivedi:
>> >> This patch series adds a validation in checkpatch tool to check if
>> >> tracepoint is present in any new function added in ethdev, eventdev
>> >> cryptodev and mempool library.
>> >>
>> >> v5:
>> >>  - Copied the build_map_changes function from check-symbol-change.sh
>to
>> >>    check-tracepoint.sh.
>> >>  - Added eventdev, cryptodev and mempool in libdir in check-
>tracepoint.sh.
>> >
>> >Why did you decide to copy the function in v5, instead of having a
>> >common file usable by different scripts?
>> >
>> There was comments in v2 of the patch that common scripts may not work
>well and to keep the scripts specialized.
>
>I meant you can have a common file specialized in symbols.
The build_map_changes() (in devtools/check-symbol-change.sh) which is a common function can be moved to a new file named devtools/build-symbol-map.sh.
The build-symbol-map.sh can be included in check-symbol-change.sh and check-tracepoint.sh. 
Please let me know if this is fine.
>In general, you should reply, establish a discussion, so we share the same
>understanding.
>
  
Thomas Monjalon Nov. 30, 2023, 8:40 a.m. UTC | #5
30/11/2023 06:56, Ankur Dwivedi:
> From: Thomas Monjalon <thomas@monjalon.net>
> > 28/11/2023 15:07, Ankur Dwivedi:
> >> > 07/03/2023 13:05, Ankur Dwivedi:
> >> >> This patch series adds a validation in checkpatch tool to check if
> >> >> tracepoint is present in any new function added in ethdev, eventdev
> >> >> cryptodev and mempool library.
> >> >>
> >> >> v5:
> >> >>  - Copied the build_map_changes function from check-symbol-change.sh
> >to
> >> >>    check-tracepoint.sh.
> >> >>  - Added eventdev, cryptodev and mempool in libdir in check-
> >tracepoint.sh.
> >> >
> >> >Why did you decide to copy the function in v5, instead of having a
> >> >common file usable by different scripts?
> >> >
> >> There was comments in v2 of the patch that common scripts may not work
> >well and to keep the scripts specialized.
> >
> >I meant you can have a common file specialized in symbols.
> The build_map_changes() (in devtools/check-symbol-change.sh) which is a common function can be moved to a new file named devtools/build-symbol-map.sh.
> The build-symbol-map.sh can be included in check-symbol-change.sh and check-tracepoint.sh. 
> Please let me know if this is fine.

Yes
We can imagine moving more symbol map related funtions in this new file.
What about symbol-map-util.sh as filename?
  
Ankur Dwivedi Nov. 30, 2023, 1:16 p.m. UTC | #6
>-----Original Message-----
>From: Thomas Monjalon <thomas@monjalon.net>
>Sent: Thursday, November 30, 2023 2:11 PM
>To: Ankur Dwivedi <adwivedi@marvell.com>
>Cc: dev@dpdk.org; Jerin Jacob Kollanukkaran <jerinj@marvell.com>
>Subject: Re: [EXT] Re: [PATCH v5 0/1] devtools: add tracepoint check in
>checkpatch
>
>30/11/2023 06:56, Ankur Dwivedi:
>> From: Thomas Monjalon <thomas@monjalon.net>
>> > 28/11/2023 15:07, Ankur Dwivedi:
>> >> > 07/03/2023 13:05, Ankur Dwivedi:
>> >> >> This patch series adds a validation in checkpatch tool to check
>> >> >> if tracepoint is present in any new function added in ethdev,
>> >> >> eventdev cryptodev and mempool library.
>> >> >>
>> >> >> v5:
>> >> >>  - Copied the build_map_changes function from
>> >> >> check-symbol-change.sh
>> >to
>> >> >>    check-tracepoint.sh.
>> >> >>  - Added eventdev, cryptodev and mempool in libdir in check-
>> >tracepoint.sh.
>> >> >
>> >> >Why did you decide to copy the function in v5, instead of having a
>> >> >common file usable by different scripts?
>> >> >
>> >> There was comments in v2 of the patch that common scripts may not
>> >> work
>> >well and to keep the scripts specialized.
>> >
>> >I meant you can have a common file specialized in symbols.
>> The build_map_changes() (in devtools/check-symbol-change.sh) which is a
>common function can be moved to a new file named devtools/build-symbol-
>map.sh.
>> The build-symbol-map.sh can be included in check-symbol-change.sh and
>check-tracepoint.sh.
>> Please let me know if this is fine.
>
>Yes
>We can imagine moving more symbol map related funtions in this new file.
>What about symbol-map-util.sh as filename?
I am ok with this filename.
>