linux-kernel - Re: [PATCH] rust: bindings: rename const binding using sed

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]

Date:   Tue, 17 Oct 2023 07:15:16 +0000
From:   Benno Lossin <benno.lossin@...ton.me>
To:     Gary Guo <gary@...yguo.net>, Miguel Ojeda <ojeda@...nel.org>,
        Alex Gaynor <alex.gaynor@...il.com>,
        Wedson Almeida Filho <wedsonaf@...il.com>,
        Boqun Feng <boqun.feng@...il.com>,
        Björn Roy Baron <bjorn3_gh@...tonmail.com>,
        Andreas Hindborg <a.hindborg@...sung.com>,
        Alice Ryhl <aliceryhl@...gle.com>,
        Martin Rodriguez Reboredo <yakoyoku@...il.com>,
        Vincenzo Palazzo <vincenzopalazzodev@...il.com>,
        Vlastimil Babka <vbabka@...e.cz>
Cc:     Wedson Almeida Filho <walmeida@...rosoft.com>,
        rust-for-linux@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] rust: bindings: rename const binding using sed

On 30.09.23 15:36, Gary Guo wrote:
> Current for consts that bindgen don't recognise, we define a helper
> constant with
> 
>      const <TYPE> BINDINGS_<NAME> = <NAME>;
> 
> in `bindings_helper.h` and then we put
> 
>      pub const <NAME>: <TYPE> = BINDINGS_<NAME>;
> 
> in `bindings/lib.rs`. This is fine that we currently only have 3
> constants that are defined this way, but is going to be more annoying
> when more constants are added since every new constant needs to be
> defined in two places.
> 
> This patch changes the way we define constant helpers to
> 
>      const <TYPE> RUST_BINDING_<NAME> = <NAME>;
> 
> and then use `sed` to postprocess Rust code by generated by bindgen to
> remove the distinct prefix, so user of the binding crate can refer to
> the name directly.
> 
> Signed-off-by: Gary Guo <gary@...yguo.net>
> ---
>   rust/Makefile                   | 2 ++
>   rust/bindings/bindings_helper.h | 6 +++---
>   rust/bindings/lib.rs            | 3 ---
>   rust/kernel/allocator.rs        | 2 +-
>   4 files changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/rust/Makefile b/rust/Makefile
> index 14d93cf60a95..20889302b172 100644
> --- a/rust/Makefile
> +++ b/rust/Makefile
> @@ -339,6 +339,8 @@ quiet_cmd_bindgen = BINDGEN $@
> 
>   $(obj)/bindings/bindings_generated.rs: private bindgen_target_flags = \
>       $(shell grep -v '^#\|^$$' $(srctree)/$(src)/bindgen_parameters)
> +$(obj)/bindings/bindings_generated.rs: private bindgen_target_extra = ; \
> +    sed -Ei 's/pub const RUST_BINDING_([a-zA-Z0-9_]*)/pub const \1/g' $@

Would it make sense to use `^pub const...`?

I also agree with Alice that we could use a less generic name as the
prefix.

With those things fixed:

Reviewed-by: Benno Lossin <benno.lossin@...ton.me>

-- 
Cheers,
Benno

>   $(obj)/bindings/bindings_generated.rs: $(src)/bindings/bindings_helper.h \
>       $(src)/bindgen_parameters FORCE
>   	$(call if_changed_dep,bindgen)
> diff --git a/rust/bindings/bindings_helper.h b/rust/bindings/bindings_helper.h
> index 85f013ed4ca4..c41eaab4ddb2 100644
> --- a/rust/bindings/bindings_helper.h
> +++ b/rust/bindings/bindings_helper.h
> @@ -15,6 +15,6 @@
>   #include <linux/workqueue.h>
> 
>   /* `bindgen` gets confused at certain things. */
> -const size_t BINDINGS_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
> -const gfp_t BINDINGS_GFP_KERNEL = GFP_KERNEL;
> -const gfp_t BINDINGS___GFP_ZERO = __GFP_ZERO;
> +const size_t RUST_BINDING_ARCH_SLAB_MINALIGN = ARCH_SLAB_MINALIGN;
> +const gfp_t RUST_BINDING_GFP_KERNEL = GFP_KERNEL;
> +const gfp_t RUST_BINDING___GFP_ZERO = __GFP_ZERO;
> diff --git a/rust/bindings/lib.rs b/rust/bindings/lib.rs
> index 9bcbea04dac3..40ddaee50d8b 100644
> --- a/rust/bindings/lib.rs
> +++ b/rust/bindings/lib.rs
> @@ -48,6 +48,3 @@ mod bindings_helper {
>   }
> 
>   pub use bindings_raw::*;
> -
> -pub const GFP_KERNEL: gfp_t = BINDINGS_GFP_KERNEL;
> -pub const __GFP_ZERO: gfp_t = BINDINGS___GFP_ZERO;
> diff --git a/rust/kernel/allocator.rs b/rust/kernel/allocator.rs
> index a8f3d5be1af1..4b057e837358 100644
> --- a/rust/kernel/allocator.rs
> +++ b/rust/kernel/allocator.rs
> @@ -21,7 +21,7 @@ unsafe fn krealloc_aligned(ptr: *mut u8, new_layout: Layout, flags: bindings::gf
> 
>       let mut size = layout.size();
> 
> -    if layout.align() > bindings::BINDINGS_ARCH_SLAB_MINALIGN {
> +    if layout.align() > bindings::ARCH_SLAB_MINALIGN {
>           // The alignment requirement exceeds the slab guarantee, thus try to enlarge the size
>           // to use the "power-of-two" size/alignment guarantee (see comments in `kmalloc()` for
>           // more information).
> --
> 2.40.1
>