rust-for-linux.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Benno Lossin <benno.lossin@proton.me>
To: "Gary Guo" <gary@garyguo.net>, "Miguel Ojeda" <ojeda@kernel.org>,
	"Alex Gaynor" <alex.gaynor@gmail.com>,
	"Wedson Almeida Filho" <wedsonaf@gmail.com>,
	"Boqun Feng" <boqun.feng@gmail.com>,
	"Björn Roy Baron" <bjorn3_gh@protonmail.com>,
	"Andreas Hindborg" <a.hindborg@samsung.com>,
	"Alice Ryhl" <aliceryhl@google.com>,
	"Martin Rodriguez Reboredo" <yakoyoku@gmail.com>,
	"Vincenzo Palazzo" <vincenzopalazzodev@gmail.com>,
	"Vlastimil Babka" <vbabka@suse.cz>
Cc: Wedson Almeida Filho <walmeida@microsoft.com>,
	rust-for-linux@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rust: bindings: rename const binding using sed
Date: Tue, 17 Oct 2023 07:15:16 +0000	[thread overview]
Message-ID: <f9e1a738-43f5-4ca6-9593-1b92053101ff@proton.me> (raw)
In-Reply-To: <20230930133704.13313-1-gary@garyguo.net>

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@garyguo.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@proton.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
> 



  parent reply	other threads:[~2023-10-17  7:15 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-09-30 13:36 [PATCH] rust: bindings: rename const binding using sed Gary Guo
2023-09-30 22:41 ` Martin Rodriguez Reboredo
2023-10-01 12:12 ` Alice Ryhl
2023-10-17  7:15 ` Benno Lossin [this message]
2023-10-20 10:51 ` Andreas Hindborg (Samsung)

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f9e1a738-43f5-4ca6-9593-1b92053101ff@proton.me \
    --to=benno.lossin@proton.me \
    --cc=a.hindborg@samsung.com \
    --cc=alex.gaynor@gmail.com \
    --cc=aliceryhl@google.com \
    --cc=bjorn3_gh@protonmail.com \
    --cc=boqun.feng@gmail.com \
    --cc=gary@garyguo.net \
    --cc=linux-kernel@vger.kernel.org \
    --cc=ojeda@kernel.org \
    --cc=rust-for-linux@vger.kernel.org \
    --cc=vbabka@suse.cz \
    --cc=vincenzopalazzodev@gmail.com \
    --cc=walmeida@microsoft.com \
    --cc=wedsonaf@gmail.com \
    --cc=yakoyoku@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).