linux-kernel - Re: [PATCH][RFC] kernel.h: provide array iterator

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <b6af11b9-d697-59ec-6acc-80f0657a3e11@prevas.dk>
Date:   Fri, 16 Mar 2018 16:27:28 +0100
From:   Rasmus Villemoes <rasmus.villemoes@...vas.dk>
To:     Kieran Bingham <kieran.bingham@...asonboard.com>,
        linux-kernel@...r.kernel.org
Cc:     Laurent Pinchart <laurent.pinchart@...asonboard.com>,
        linux-media@...r.kernel.org,
        Andrew Morton <akpm@...ux-foundation.org>,
        Ingo Molnar <mingo@...nel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Kees Cook <keescook@...omium.org>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        Masahiro Yamada <yamada.masahiro@...ionext.com>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        Randy Dunlap <rdunlap@...radead.org>,
        Ian Abbott <abbotti@....co.uk>
Subject: Re: [PATCH][RFC] kernel.h: provide array iterator

On 2018-03-15 11:00, Kieran Bingham wrote:
> Simplify array iteration with a helper to iterate each entry in an array.
> Utilise the existing ARRAY_SIZE macro to identify the length of the array
> and pointer arithmetic to process each item as a for loop.
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@...asonboard.com>
> ---
>  include/linux/kernel.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> The use of static arrays to store data is a common use case throughout the
> kernel. Along with that is the obvious need to iterate that data.
> 
> In fact there are just shy of 5000 instances of iterating a static array:
> 	git grep "for .*ARRAY_SIZE" | wc -l
> 	4943
> 
> When working on the UVC driver - I found that I needed to split one such
> iteration into two parts, and at the same time felt that this could be
> refactored to be cleaner / easier to read. 

About that, it would be helpful if you first converted to the new
iterator, so that one can more easily see they are equivalent. And then
split in two, adding the flush_workqueue call. Or do it the other way
around. But please don't mix the two in one patch, especially not if
it's supposed to act as an example of how to use the new helper.

> I do however worry that this simple short patch might not be desired or could
> also be heavily bikeshedded due to it's potential wide spread use (though
> perhaps that would be a good thing to have more users) ...  but here it is,
> along with an example usage below which is part of a separate series.

I think it can be useful, and it does have the must_be_array protection
built in, so code doesn't silently break if one changes from a
fixed-size allocation to e.g. a kmalloc-based one. Just don't attempt a
tree-wide mass conversion, but obviously starting to make use of it when
refactoring code anyway is fine.

And now, the bikeshedding you expected :)

> diff --git a/include/linux/kernel.h b/include/linux/kernel.h
> index ce51455e2adf..95d7dae248b7 100644
> --- a/include/linux/kernel.h
> +++ b/include/linux/kernel.h
> @@ -70,6 +70,16 @@
>   */
>  #define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0]) + __must_be_array(arr))
>  
> +/**
> + * for_each_array_element - Iterate all items in an array
> + * @elem: pointer of array type for iteration cursor

Hm, "pointer of array type" sounds wrong; it's not a "pointer to array".
But "pointer of array elements' type" is clumsy. Maybe just "@elem:
iteration cursor" is clear enough.

> + * @array: array to be iterated
> + */
> +#define for_each_array_element(elem, array) \
> +	for (elem = &(array)[0]; \
> +	     elem < &(array)[ARRAY_SIZE(array)]; \
> +	     ++elem)
> +

Please parenthesize elem as well.

Rasmus