[PATCH] chore(rust): add opt_get! and expose some FMODE_* as Rust const

[PATCH] chore(rust): add opt_get! and expose some FMODE_* as Rust const

[TruongSinh Tran-Nguyen]

In an effort to rewrite `bch2_read_super` from C to Rust,
it is neccessary to have `opt_get!` macro defined, and some
FMODE_* consts (defined as macro in `include/linux/blkdev.h`)
defined as Rust const.

Bindgen is currently unable to exapnd C functional macro [1],
this this commit use the workaround as introduced in [2].

[1] https://github.com/rust-lang/rust-bindgen/issues/753
[2] https://github.com/rust-lang/rust-bindgen/issues/753#issuecomment-608546390

Signed-off-by: TruongSinh Tran-Nguyen <i@truongsinh.pro>
---
 rust-src/bch_bindgen/build.rs                 | 11 +++++++
 .../bch_bindgen/src/libbcachefs_wrapper.h     |  7 +++++
 rust-src/bch_bindgen/src/opts.rs              | 30 +++++++++++++++++--
 3 files changed, 46 insertions(+), 2 deletions(-)

diff --git a/rust-src/bch_bindgen/build.rs b/rust-src/bch_bindgen/build.rs
index f426316..92ec3ce 100644
--- a/rust-src/bch_bindgen/build.rs
+++ b/rust-src/bch_bindgen/build.rs
@@ -1,3 +1,12 @@
+
+#[derive(Debug)]
+pub struct Fix753 {}
+impl bindgen::callbacks::ParseCallbacks for Fix753 {
+    fn item_name(&self, original_item_name: &str) -> Option<String> {
+        Some(original_item_name.trim_start_matches("Fix753_").to_owned())
+    }
+}
+
 fn main() {
     use std::path::PathBuf;
 
@@ -49,6 +58,7 @@ fn main() {
         .blocklist_type("srcu_struct")
         .allowlist_var("BCH_.*")
         .allowlist_var("KEY_SPEC_.*")
+        .allowlist_var("Fix753_FMODE_.*")
         .allowlist_var("bch.*")
         .allowlist_var("__BTREE_ITER.*")
         .allowlist_var("BTREE_ITER.*")
@@ -69,6 +79,7 @@ fn main() {
         .no_partialeq("bkey")
         .no_partialeq("bpos")
         .generate_inline_functions(true)
+        .parse_callbacks(Box::new(Fix753 {}))
         .generate()
         .expect("BindGen Generation Failiure: [libbcachefs_wrapper]");
     bindings
diff --git a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
index d1ebf4b..e7bcfcf 100644
--- a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
+++ b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
@@ -10,4 +10,11 @@
 #include "../libbcachefs.h"
 #include "../crypto.h"
 #include "../include/linux/bio.h"
+#include "../include/linux/blkdev.h"
 
+
+#define MARK_FIX_753(req_name) const fmode_t Fix753_##req_name = req_name;
+
+MARK_FIX_753(FMODE_READ);
+MARK_FIX_753(FMODE_WRITE);
+MARK_FIX_753(FMODE_EXCL);
\ No newline at end of file
diff --git a/rust-src/bch_bindgen/src/opts.rs b/rust-src/bch_bindgen/src/opts.rs
index e226199..d38d469 100644
--- a/rust-src/bch_bindgen/src/opts.rs
+++ b/rust-src/bch_bindgen/src/opts.rs
@@ -3,7 +3,33 @@ macro_rules! opt_set {
     ($opts:ident, $n:ident, $v:expr) => {
         bch_bindgen::paste! {
             $opts.$n = $v;
-            $opts.[<set_ $n _defined>](1);
+            $opts.[<set_ $n _defined>](1)
         }
-    }
+    };
+}
+
+#[macro_export]
+macro_rules! opt_defined {
+    ($opts:ident, $n:ident) => {
+        bch_bindgen::paste! {
+            $opts.[< $n _defined>]()
+        }
+    };
+}
+
+#[macro_export]
+macro_rules! opt_get {
+    ($opts:ident, $n:ident) => {
+        if bch_bindgen::opt_defined!($opts, $n) == 0 {
+            bch_bindgen::paste! {
+                unsafe {
+                    bch_bindgen::bcachefs::bch2_opts_default.$n
+                }
+            }
+        } else {
+            bch_bindgen::paste! {
+                $opts.$n
+            }
+        }
+    };
 }
-- 
2.30.2

Re: [PATCH] chore(rust): add opt_get! and expose some FMODE_* as Rust const

[Kent Overstreet]

On Thu, Apr 27, 2023 at 11:02:00AM -0700, TruongSinh Tran-Nguyen wrote:
> In an effort to rewrite `bch2_read_super` from C to Rust,
> it is neccessary to have `opt_get!` macro defined, and some
> FMODE_* consts (defined as macro in `include/linux/blkdev.h`)
> defined as Rust const.
> 
> Bindgen is currently unable to exapnd C functional macro [1],
> this this commit use the workaround as introduced in [2].
> 
> [1] https://github.com/rust-lang/rust-bindgen/issues/753
> [2] https://github.com/rust-lang/rust-bindgen/issues/753#issuecomment-608546390
> 
> Signed-off-by: TruongSinh Tran-Nguyen <i@truongsinh.pro>
> ---
>  rust-src/bch_bindgen/build.rs                 | 11 +++++++
>  .../bch_bindgen/src/libbcachefs_wrapper.h     |  7 +++++
>  rust-src/bch_bindgen/src/opts.rs              | 30 +++++++++++++++++--
>  3 files changed, 46 insertions(+), 2 deletions(-)
> 
> diff --git a/rust-src/bch_bindgen/build.rs b/rust-src/bch_bindgen/build.rs
> index f426316..92ec3ce 100644
> --- a/rust-src/bch_bindgen/build.rs
> +++ b/rust-src/bch_bindgen/build.rs
> @@ -1,3 +1,12 @@
> +
> +#[derive(Debug)]
> +pub struct Fix753 {}
> +impl bindgen::callbacks::ParseCallbacks for Fix753 {
> +    fn item_name(&self, original_item_name: &str) -> Option<String> {
> +        Some(original_item_name.trim_start_matches("Fix753_").to_owned())
> +    }
> +}
> +
>  fn main() {
>      use std::path::PathBuf;
>  
> @@ -49,6 +58,7 @@ fn main() {
>          .blocklist_type("srcu_struct")
>          .allowlist_var("BCH_.*")
>          .allowlist_var("KEY_SPEC_.*")
> +        .allowlist_var("Fix753_FMODE_.*")
>          .allowlist_var("bch.*")
>          .allowlist_var("__BTREE_ITER.*")
>          .allowlist_var("BTREE_ITER.*")
> @@ -69,6 +79,7 @@ fn main() {
>          .no_partialeq("bkey")
>          .no_partialeq("bpos")
>          .generate_inline_functions(true)
> +        .parse_callbacks(Box::new(Fix753 {}))
>          .generate()
>          .expect("BindGen Generation Failiure: [libbcachefs_wrapper]");
>      bindings
> diff --git a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
> index d1ebf4b..e7bcfcf 100644
> --- a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
> +++ b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
> @@ -10,4 +10,11 @@
>  #include "../libbcachefs.h"
>  #include "../crypto.h"
>  #include "../include/linux/bio.h"
> +#include "../include/linux/blkdev.h"
>  
> +
> +#define MARK_FIX_753(req_name) const fmode_t Fix753_##req_name = req_name;

Do we actually need to include Fix753_ in the constants we define? Why
not shadow the macro names?

> +
> +MARK_FIX_753(FMODE_READ);
> +MARK_FIX_753(FMODE_WRITE);
> +MARK_FIX_753(FMODE_EXCL);
> \ No newline at end of file
> diff --git a/rust-src/bch_bindgen/src/opts.rs b/rust-src/bch_bindgen/src/opts.rs
> index e226199..d38d469 100644
> --- a/rust-src/bch_bindgen/src/opts.rs
> +++ b/rust-src/bch_bindgen/src/opts.rs
> @@ -3,7 +3,33 @@ macro_rules! opt_set {
>      ($opts:ident, $n:ident, $v:expr) => {
>          bch_bindgen::paste! {
>              $opts.$n = $v;
> -            $opts.[<set_ $n _defined>](1);
> +            $opts.[<set_ $n _defined>](1)
>          }
> -    }
> +    };
> +}
> +
> +#[macro_export]
> +macro_rules! opt_defined {
> +    ($opts:ident, $n:ident) => {
> +        bch_bindgen::paste! {
> +            $opts.[< $n _defined>]()
> +        }
> +    };
> +}
> +
> +#[macro_export]
> +macro_rules! opt_get {
> +    ($opts:ident, $n:ident) => {
> +        if bch_bindgen::opt_defined!($opts, $n) == 0 {
> +            bch_bindgen::paste! {
> +                unsafe {
> +                    bch_bindgen::bcachefs::bch2_opts_default.$n
> +                }
> +            }
> +        } else {
> +            bch_bindgen::paste! {
> +                $opts.$n
> +            }
> +        }
> +    };

This is fine, it's a direct translation of the C code.

At some point we ought to do a proper Rust version of the opts struct
that uses Option<>. (We'd actually want two different versions, one that
has every option wrapped in Option<> and one doesn't, since e.g. c->opts
doesn't have unset options.)

Re: [PATCH] chore(rust): add opt_get! and expose some FMODE_* as Rust const

[TruongSinh Tran-Nguyen]

On Thu, 27 Apr 2023 at 13:16, Kent Overstreet <kent.overstreet@linux.dev> wrote:
>
> On Thu, Apr 27, 2023 at 11:02:00AM -0700, TruongSinh Tran-Nguyen wrote:
> > In an effort to rewrite `bch2_read_super` from C to Rust,
> > it is neccessary to have `opt_get!` macro defined, and some
> > FMODE_* consts (defined as macro in `include/linux/blkdev.h`)
> > defined as Rust const.
> >
> > Bindgen is currently unable to exapnd C functional macro [1],
> > this this commit use the workaround as introduced in [2].
> >
> > [1] https://github.com/rust-lang/rust-bindgen/issues/753
> > [2] https://github.com/rust-lang/rust-bindgen/issues/753#issuecomment-608546390
> >
> > Signed-off-by: TruongSinh Tran-Nguyen <i@truongsinh.pro>
> > ---
> >  rust-src/bch_bindgen/build.rs                 | 11 +++++++
> >  .../bch_bindgen/src/libbcachefs_wrapper.h     |  7 +++++
> >  rust-src/bch_bindgen/src/opts.rs              | 30 +++++++++++++++++--
> >  3 files changed, 46 insertions(+), 2 deletions(-)
> >
> > diff --git a/rust-src/bch_bindgen/build.rs b/rust-src/bch_bindgen/build.rs
> > index f426316..92ec3ce 100644
> > --- a/rust-src/bch_bindgen/build.rs
> > +++ b/rust-src/bch_bindgen/build.rs
> > @@ -1,3 +1,12 @@
> > +
> > +#[derive(Debug)]
> > +pub struct Fix753 {}
> > +impl bindgen::callbacks::ParseCallbacks for Fix753 {
> > +    fn item_name(&self, original_item_name: &str) -> Option<String> {
> > +        Some(original_item_name.trim_start_matches("Fix753_").to_owned())
> > +    }
> > +}
> > +
> >  fn main() {
> >      use std::path::PathBuf;
> >
> > @@ -49,6 +58,7 @@ fn main() {
> >          .blocklist_type("srcu_struct")
> >          .allowlist_var("BCH_.*")
> >          .allowlist_var("KEY_SPEC_.*")
> > +        .allowlist_var("Fix753_FMODE_.*")
> >          .allowlist_var("bch.*")
> >          .allowlist_var("__BTREE_ITER.*")
> >          .allowlist_var("BTREE_ITER.*")
> > @@ -69,6 +79,7 @@ fn main() {
> >          .no_partialeq("bkey")
> >          .no_partialeq("bpos")
> >          .generate_inline_functions(true)
> > +        .parse_callbacks(Box::new(Fix753 {}))
> >          .generate()
> >          .expect("BindGen Generation Failiure: [libbcachefs_wrapper]");
> >      bindings
> > diff --git a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
> > index d1ebf4b..e7bcfcf 100644
> > --- a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
> > +++ b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
> > @@ -10,4 +10,11 @@
> >  #include "../libbcachefs.h"
> >  #include "../crypto.h"
> >  #include "../include/linux/bio.h"
> > +#include "../include/linux/blkdev.h"
> >
> > +
> > +#define MARK_FIX_753(req_name) const fmode_t Fix753_##req_name = req_name;
>
> Do we actually need to include Fix753_ in the constants we define? Why
> not shadow the macro names?
>
> > +
> > +MARK_FIX_753(FMODE_READ);
> > +MARK_FIX_753(FMODE_WRITE);
> > +MARK_FIX_753(FMODE_EXCL);
> > \ No newline at end of file
> > diff --git a/rust-src/bch_bindgen/src/opts.rs b/rust-src/bch_bindgen/src/opts.rs
> > index e226199..d38d469 100644
> > --- a/rust-src/bch_bindgen/src/opts.rs
> > +++ b/rust-src/bch_bindgen/src/opts.rs
> > @@ -3,7 +3,33 @@ macro_rules! opt_set {
> >      ($opts:ident, $n:ident, $v:expr) => {
> >          bch_bindgen::paste! {
> >              $opts.$n = $v;
> > -            $opts.[<set_ $n _defined>](1);
> > +            $opts.[<set_ $n _defined>](1)
> >          }
> > -    }
> > +    };
> > +}
> > +
> > +#[macro_export]
> > +macro_rules! opt_defined {
> > +    ($opts:ident, $n:ident) => {
> > +        bch_bindgen::paste! {
> > +            $opts.[< $n _defined>]()
> > +        }
> > +    };
> > +}
> > +
> > +#[macro_export]
> > +macro_rules! opt_get {
> > +    ($opts:ident, $n:ident) => {
> > +        if bch_bindgen::opt_defined!($opts, $n) == 0 {
> > +            bch_bindgen::paste! {
> > +                unsafe {
> > +                    bch_bindgen::bcachefs::bch2_opts_default.$n
> > +                }
> > +            }
> > +        } else {
> > +            bch_bindgen::paste! {
> > +                $opts.$n
> > +            }
> > +        }
> > +    };
>
> This is fine, it's a direct translation of the C code.
>
> At some point we ought to do a proper Rust version of the opts struct
> that uses Option<>. (We'd actually want two different versions, one that
> has every option wrapped in Option<> and one doesn't, since e.g. c->opts
> doesn't have unset options.)

> Do we actually need to include Fix753_ in the constants we define? Why
not shadow the macro names?

The prefix is during the process only, but it will be stripped out by
`.parse_callbacks(Box::new(Fix753 {}))`. In other words, in Rust,
it would be used like this:

```
sb.mode &= !FMODE_WRITE;
```

As for the reason we need `Fix753_` prefix even though it would not make it
to the final generated `bcachefs.rs`, I guess it is rust community convention
to have the workaround while waiting for the create to have the fix, and when
it is fixed, remove it.

Re: [PATCH] chore(rust): add opt_get! and expose some FMODE_* as Rust const

[Kent Overstreet]

On Thu, Apr 27, 2023 at 02:17:15PM -0700, TruongSinh Tran-Nguyen wrote:
> The prefix is during the process only, but it will be stripped out by
> `.parse_callbacks(Box::new(Fix753 {}))`. In other words, in Rust,
> it would be used like this:
> 
> ```
> sb.mode &= !FMODE_WRITE;
> ```

Weird, but ok.