[PATCH] rust: bindings: rename const binding using sed

[PATCH] rust: bindings: rename const binding using sed

[Gary Guo]

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' $@
 $(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

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

[Martin Rodriguez Reboredo]

On 9/30/23 10: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>
> ---
> [...]
This one is a must have.

Reviewed-by: Martin Rodriguez Reboredo <yakoyoku@gmail.com>

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

[Alice Ryhl]

On 9/30/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.

Maybe it would make sense to use a less generic name for this sed 
replacement? E.g., maybe RUST_CONST_HELPER_ or something that's less 
likely to overlap with things that are not constants.

Alice

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

[Benno Lossin]

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
> 

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

[Andreas Hindborg (Samsung)]

Gary Guo <gary@garyguo.net> writes:

> 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>

Reviewed-by: Andreas Hindborg <a.hindborg@samsung.com>

> ---
>  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' $@
>  $(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).