Hello Andre,
On Tue, Dec 31, 2024 at 7:39 PM Andre Muezerie
<andre...@linux.microsoft.com> wrote:
>
> Ensure __rte_packed_begin and __rte_packed_end show up in pairs
> when checking patches.
>
> Signed-off-by: Andre Muezerie <andre...@linux.microsoft.com>
> Acked-by: Tyler Retzlaff <roret...@linux.microsoft.com>
> ---
> devtools/checkpatches.sh | 43 ++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 43 insertions(+)
>
> diff --git a/devtools/checkpatches.sh b/devtools/checkpatches.sh
> index 4a8591be22..7868f5e522 100755
> --- a/devtools/checkpatches.sh
> +++ b/devtools/checkpatches.sh
> @@ -202,6 +202,14 @@ check_forbidden_additions() { # <patch>
> -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> "$1" || res=1
>
> + # forbid use of __rte_packed_begin with enums
> + awk -v FOLDERS='lib drivers app examples' \
> + -v EXPRESSIONS='enum.*__rte_packed_begin' \
> + -v RET_ON_FAIL=1 \
> + -v MESSAGE='Using __rte_packed_begin with enum is not
> allowed' \
> + -f $(dirname $(readlink -f $0))/check-forbidden-tokens.awk \
> + "$1" || res=1
> +
> # forbid use of experimental build flag except in examples
> awk -v FOLDERS='lib drivers app' \
> -v EXPRESSIONS='-DALLOW_EXPERIMENTAL_API
> allow_experimental_apis' \
> @@ -362,6 +370,33 @@ check_aligned_attributes() { # <patch>
> return $res
> }
>
> +check_packed_attributes() { # <patch>
> + res=0
> +
> + if [ $(grep -E '^\+.*__rte_packed_begin' "$1" | \
> + grep -vE '\<struct[[:space:]]*__rte_packed_begin\>' |
> \
> + grep -vE '\<union[[:space:]]*__rte_packed_begin\>' | \
> + grep -vE
> '\<__rte_cache_aligned[[:space:]]*__rte_packed_begin\>' | \
> + grep -vE
> '\<__rte_cache_min_aligned[[:space:]]*__rte_packed_begin\>' | \
> + grep -vE
> '\<__rte_aligned\(.*\)[[:space:]]*__rte_packed_begin\>' | \
> + wc -l) != 0 ]; then
> + echo "Please use __rte_packed_begin only after struct,
> union," \
> + " __rte_cache_aligned, __rte_cache_min_aligned or
> __rte_aligned."
> + res=1
> + fi
This part lgtm.
> +
> + begin_count=$(grep '__rte_packed_begin' "$1" | \
> + wc -l)
> + end_count=$(grep '__rte_packed_end' "$1" | \
> + wc -l)
> + if [ $begin_count != $end_count ]; then
> + echo "__rte_packed_begin and __rte_packed_end mismatch. They
> should always be used in pairs."
> + res=1
> + fi
This part is problematic.
The check is applied on a patch: a __rte_packed_* token (let's imagine
__rte_packed_begin) may be involved without an associated change on
its counterpart token (__rte_packed_end).
Let me show an example:
$ git show
commit 655cfe433ab6f4fb8c92fec44ea5e1f689055201 (HEAD -> pack_v8_dma)
Author: David Marchand <david.march...@redhat.com>
Date: Tue Jan 7 15:13:26 2025 +0100
plop
plop
Signed-off-by: David Marchand <david.march...@redhat.com>
diff --git a/lib/net/rte_ip6.h b/lib/net/rte_ip6.h
index 92558a124a..686c12220b 100644
--- a/lib/net/rte_ip6.h
+++ b/lib/net/rte_ip6.h
@@ -461,7 +461,7 @@ rte_ether_mcast_from_ipv6(struct rte_ether_addr
*mac, const struct rte_ipv6_addr
/**
* IPv6 Header
*/
-struct __rte_aligned(2) __rte_packed_begin rte_ipv6_hdr {
+struct __rte_aligned(2) __rte_packed_begin rte_ipv6_hdr /* A useful
comment, isn't it? */ {
union {
rte_be32_t vtc_flow; /**< IP version, traffic
class & flow label. */
__extension__
$ ./devtools/checkpatches.sh -n1
### [PATCH] plop
__rte_packed_begin and __rte_packed_end should always be used in pairs.
0/1 valid patch
I don't think there is a way to accurately catch wrongly paired tokens.
The only good way is having most extensive Windows builds in the CI, isn't it?
--
David Marchand
