[PATCH TOOLS 0/2] convert main() from C to Rust

[PATCH TOOLS 0/2] convert main() from C to Rust

[Thomas Bertschinger]

This converts the top-level main() of the bcachefs command line tool
from C to Rust.

The first patch does all the work of implementing a new main(), updating
the build process, and making things look for the executable in its new
location which is now rust-src/target/release/bcachefs.

The second patch removes the Library crate from the Rust package. This
was needed previously to allow the C program to link in the Rust
functions, but that doesn't happen anymore.

It seems likely that a bcachefs-tools library will be needed in the
future; for example see this request [1] on GitHub. This library will
have a different external-focused API as opposed to the previous library
that provided an internal API for the bcachefs tool itself. Considering
Rust's structure of one library crate per package, removing the current
library makes room for that requested library.

I tried to update everything to account for the new location of the
bcachefs executable, but let me know if anyone sees something I missed.
I could not figure out how this is set for the Debian package so it's
possible I missed something there...

[1] https://github.com/koverstreet/bcachefs-tools/issues/188


Thomas Bertschinger (2):
  convert main() from C to Rust
  remove library from bcachefs-tools Rust package

 Makefile                                      |  21 +--
 bcachefs.c                                    | 127 +-----------------
 cmds.h                                        |   6 +-
 rust-src/Cargo.lock                           |   2 +-
 rust-src/Cargo.toml                           |   7 +-
 rust-src/bch_bindgen/build.rs                 |   6 +
 .../bch_bindgen/src/libbcachefs_wrapper.h     |   2 +
 rust-src/build.rs                             |  21 +++
 rust-src/src/bcachefs.rs                      | 120 +++++++++++++++++
 rust-src/src/cmd_main.rs                      |  34 -----
 .../src/{ => commands}/cmd_completions.rs     |   3 +-
 rust-src/src/{ => commands}/cmd_list.rs       |   3 +-
 rust-src/src/{ => commands}/cmd_mount.rs      |  11 +-
 rust-src/src/{ => commands}/logger.rs         |   0
 rust-src/src/commands/mod.rs                  |  31 +++++
 rust-src/src/lib.rs                           |  54 --------
 tests/util.py                                 |   2 +-
 17 files changed, 214 insertions(+), 236 deletions(-)
 create mode 100644 rust-src/build.rs
 create mode 100644 rust-src/src/bcachefs.rs
 delete mode 100644 rust-src/src/cmd_main.rs
 rename rust-src/src/{ => commands}/cmd_completions.rs (84%)
 rename rust-src/src/{ => commands}/cmd_list.rs (98%)
 rename rust-src/src/{ => commands}/cmd_mount.rs (94%)
 rename rust-src/src/{ => commands}/logger.rs (100%)
 create mode 100644 rust-src/src/commands/mod.rs
 delete mode 100644 rust-src/src/lib.rs

-- 
2.43.0

[PATCH TOOLS 1/2] convert main() from C to Rust

[Thomas Bertschinger]

This moves the main() function from C to Rust.

Running the bcachefs tool out of the development tree is now:

$ ./rust-src/target/release/bcachefs command
or
$ cargo run --profile release --manifest-path rust-src/Cargo.toml --command

instead of:
$ ./bcachefs command

Building and installing is still:
$ make && make install

Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
 Makefile                                      |  21 +--
 bcachefs.c                                    | 127 +-----------------
 cmds.h                                        |   6 +-
 rust-src/Cargo.lock                           |   2 +-
 rust-src/Cargo.toml                           |   8 +-
 rust-src/bch_bindgen/build.rs                 |   6 +
 .../bch_bindgen/src/libbcachefs_wrapper.h     |   2 +
 rust-src/build.rs                             |  21 +++
 rust-src/src/bcachefs.rs                      | 107 +++++++++++++++
 rust-src/src/cmd_completions.rs               |   3 +-
 rust-src/src/cmd_list.rs                      |   3 +-
 rust-src/src/cmd_main.rs                      |  34 -----
 rust-src/src/cmd_mount.rs                     |  11 +-
 rust-src/src/lib.rs                           |  13 --
 tests/util.py                                 |   2 +-
 15 files changed, 172 insertions(+), 194 deletions(-)
 create mode 100644 rust-src/build.rs
 create mode 100644 rust-src/src/bcachefs.rs
 delete mode 100644 rust-src/src/cmd_main.rs

diff --git a/Makefile b/Makefile
index a38f074..d283c7b 100644
--- a/Makefile
+++ b/Makefile
@@ -77,6 +77,7 @@ PKGCONFIG_LIBS="blkid uuid liburcu libsodium zlib liblz4 libzstd libudev libkeyu
 ifdef BCACHEFS_FUSE
 	PKGCONFIG_LIBS+="fuse3 >= 3.7"
 	CFLAGS+=-DBCACHEFS_FUSE
+	export RUSTFLAGS=--cfg fuse
 endif
 
 PKGCONFIG_CFLAGS:=$(shell $(PKG_CONFIG) --cflags $(PKGCONFIG_LIBS))
@@ -172,25 +173,15 @@ OBJS:=$(SRCS:.c=.o)
 	$(Q)$(CC) $(CPPFLAGS) $(CFLAGS) -c -o $@ $<
 
 BCACHEFS_DEPS=libbcachefs.a
+RUST_SRCS:=$(shell find rust-src/src rust-src/bch_bindgen/src -type f -iname '*.rs')
 
-ifndef NO_RUST
-	BCACHEFS_DEPS+=rust-src/target/release/libbcachefs_rust.a
-else
-	CFLAGS+=-DBCACHEFS_NO_RUST
-endif
-
-bcachefs: $(BCACHEFS_DEPS)
-	@echo "    [LD]     $@"
-	$(Q)$(CC) $(LDFLAGS) -Wl,--whole-archive $+ $(LOADLIBES) -Wl,--no-whole-archive $(LDLIBS) -o $@
+bcachefs: $(BCACHEFS_DEPS) $(RUST_SRCS)
+	$(CARGO_BUILD)
 
 libbcachefs.a: $(filter-out ./tests/%.o, $(OBJS))
 	@echo "    [AR]     $@"
 	$(Q)ar -rc $@ $+
 
-RUST_SRCS:=$(shell find rust-src/src rust-src/bch_bindgen/src -type f -iname '*.rs')
-rust-src/target/release/libbcachefs_rust.a: $(RUST_SRCS)
-	$(CARGO_BUILD)
-
 tests/test_helper: $(filter ./tests/%.o, $(OBJS))
 	@echo "    [LD]     $@"
 	$(Q)$(CC) $(LDFLAGS) $+ $(LOADLIBES) $(LDLIBS) -o $@
@@ -210,7 +201,7 @@ cmd_version.o : .version
 install: INITRAMFS_HOOK=$(INITRAMFS_DIR)/hooks/bcachefs
 install: INITRAMFS_SCRIPT=$(INITRAMFS_DIR)/scripts/local-premount/bcachefs
 install: bcachefs $(optional_install)
-	$(INSTALL) -m0755 -D bcachefs      -t $(DESTDIR)$(ROOT_SBINDIR)
+	$(INSTALL) -m0755 -D rust-src/target/release/bcachefs -t $(DESTDIR)$(ROOT_SBINDIR)
 	$(INSTALL) -m0644 -D bcachefs.8    -t $(DESTDIR)$(PREFIX)/share/man/man8/
 	$(INSTALL) -m0755 -D initramfs/script $(DESTDIR)$(INITRAMFS_SCRIPT)
 	$(INSTALL) -m0755 -D initramfs/hook   $(DESTDIR)$(INITRAMFS_HOOK)
@@ -233,7 +224,7 @@ install_systemd: $(systemd_services) $(systemd_libexecfiles)
 .PHONY: clean
 clean:
 	@echo "Cleaning all"
-	$(Q)$(RM) bcachefs libbcachefs.a tests/test_helper .version *.tar.xz $(OBJS) $(DEPS) $(DOCGENERATED)
+	$(Q)$(RM) libbcachefs.a tests/test_helper .version *.tar.xz $(OBJS) $(DEPS) $(DOCGENERATED)
 	$(Q)$(CARGO_CLEAN)
 	$(Q)$(RM) -f $(built_scripts)
 
diff --git a/bcachefs.c b/bcachefs.c
index 7ca79ad..70af2c3 100644
--- a/bcachefs.c
+++ b/bcachefs.c
@@ -25,7 +25,7 @@
 
 #include "cmds.h"
 
-static void usage(void)
+void bcachefs_usage(void)
 {
 	puts("bcachefs - tool for managing bcachefs filesystems\n"
 	     "usage: bcachefs <command> [<args>]\n"
@@ -36,11 +36,9 @@ static void usage(void)
 	     "  set-option               Set a filesystem option\n"
 	     "  reset-counters           Reset all counters on an unmounted device\n"
 	     "\n"
-#ifndef BCACHEFS_NO_RUST
 	     "Mount:\n"
 	     "  mount                    Mount a filesystem\n"
 	     "\n"
-#endif
 	     "Repair:\n"
 	     "  fsck                     Check an existing filesystem for errors\n"
 	     "\n"
@@ -89,18 +87,14 @@ static void usage(void)
 	     "Debug:\n"
 	     "These commands work on offline, unmounted filesystems\n"
 	     "  dump                     Dump filesystem metadata to a qcow2 image\n"
-#ifndef BCACHEFS_NO_RUST
 	     "  list                     List filesystem metadata in textual form\n"
-#endif
 	     "  list_journal             List contents of journal\n"
 	     "\n"
 	     "FUSE:\n"
 	     "  fusemount                Mount a filesystem via FUSE\n"
 	     "\n"
 	     "Miscellaneous:\n"
-#ifndef BCACHEFS_NO_RUST
          "  completions              Generate shell completions\n"
-#endif
 	     "  version                  Display the version of the invoked bcachefs tool\n");
 }
 
@@ -115,12 +109,12 @@ static char *pop_cmd(int *argc, char *argv[])
 	return cmd;
 }
 
-static int fs_cmds(int argc, char *argv[])
+int fs_cmds(int argc, char *argv[])
 {
 	char *cmd = pop_cmd(&argc, argv);
 
 	if (argc < 1) {
-		usage();
+		bcachefs_usage();
 		exit(EXIT_FAILURE);
 	}
 	if (!strcmp(cmd, "usage"))
@@ -129,7 +123,7 @@ static int fs_cmds(int argc, char *argv[])
 	return 0;
 }
 
-static int device_cmds(int argc, char *argv[])
+int device_cmds(int argc, char *argv[])
 {
 	char *cmd = pop_cmd(&argc, argv);
 
@@ -155,7 +149,7 @@ static int device_cmds(int argc, char *argv[])
 	return 0;
 }
 
-static int data_cmds(int argc, char *argv[])
+int data_cmds(int argc, char *argv[])
 {
 	char *cmd = pop_cmd(&argc, argv);
 
@@ -169,7 +163,7 @@ static int data_cmds(int argc, char *argv[])
 	return 0;
 }
 
-static int subvolume_cmds(int argc, char *argv[])
+int subvolume_cmds(int argc, char *argv[])
 {
 	char *cmd = pop_cmd(&argc, argv);
 	if (argc < 1)
@@ -183,112 +177,3 @@ static int subvolume_cmds(int argc, char *argv[])
 
 	return 0;
 }
-
-int main(int argc, char *argv[])
-{
-	raid_init();
-
-	char *full_cmd = argv[0];
-
-	/* Are we being called via a symlink? */
-
-	if (strstr(full_cmd, "mkfs"))
-		return cmd_format(argc, argv);
-
-	if (strstr(full_cmd, "fsck"))
-		return cmd_fsck(argc, argv);
-
-#ifdef BCACHEFS_FUSE
-	if (strstr(full_cmd, "mount.fuse"))
-		return cmd_fusemount(argc, argv);
-#endif
-
-#ifndef BCACHEFS_NO_RUST
-	if (strstr(full_cmd, "mount"))
-		return rust_main(argc, argv, "mount");
-#endif
-
-	setvbuf(stdout, NULL, _IOLBF, 0);
-
-	char *cmd = pop_cmd(&argc, argv);
-	if (!cmd) {
-		puts("missing command\n");
-		goto usage;
-	}
-
-	/* these subcommands display usage when argc < 2 */
-	if (!strcmp(cmd, "device"))
-		return device_cmds(argc, argv);
-	if (!strcmp(cmd, "fs"))
-		return fs_cmds(argc, argv);
-	if (!strcmp(cmd, "data"))
-		return data_cmds(argc, argv);
-	if (!strcmp(cmd, "subvolume"))
-		return subvolume_cmds(argc, argv);
-	if (!strcmp(cmd, "format"))
-		return cmd_format(argc, argv);
-	if (!strcmp(cmd, "fsck"))
-		return cmd_fsck(argc, argv);
-	if (!strcmp(cmd, "version"))
-		return cmd_version(argc, argv);
-	if (!strcmp(cmd, "show-super"))
-		return cmd_show_super(argc, argv);
-	if (!strcmp(cmd, "set-option"))
-		return cmd_set_option(argc, argv);
-	if (!strcmp(cmd, "reset-counters"))
-		return cmd_reset_counters(argc, argv);
-
-#if 0
-	if (!strcmp(cmd, "assemble"))
-		return cmd_assemble(argc, argv);
-	if (!strcmp(cmd, "incremental"))
-		return cmd_incremental(argc, argv);
-	if (!strcmp(cmd, "run"))
-		return cmd_run(argc, argv);
-	if (!strcmp(cmd, "stop"))
-		return cmd_stop(argc, argv);
-#endif
-
-	if (!strcmp(cmd, "unlock"))
-		return cmd_unlock(argc, argv);
-	if (!strcmp(cmd, "set-passphrase"))
-		return cmd_set_passphrase(argc, argv);
-	if (!strcmp(cmd, "remove-passphrase"))
-		return cmd_remove_passphrase(argc, argv);
-
-	if (!strcmp(cmd, "migrate"))
-		return cmd_migrate(argc, argv);
-	if (!strcmp(cmd, "migrate-superblock"))
-		return cmd_migrate_superblock(argc, argv);
-
-	if (!strcmp(cmd, "dump"))
-		return cmd_dump(argc, argv);
-	if (!strcmp(cmd, "list_journal"))
-		return cmd_list_journal(argc, argv);
-	if (!strcmp(cmd, "kill_btree_node"))
-		return cmd_kill_btree_node(argc, argv);
-
-	if (!strcmp(cmd, "setattr"))
-		return cmd_setattr(argc, argv);
-#ifndef BCACHEFS_NO_RUST
-	if (!strcmp(cmd, "list") ||
-	    !strcmp(cmd, "mount") ||
-	    !strcmp(cmd, "completions"))
-		return rust_main(argc, argv, cmd);
-#endif
-
-#ifdef BCACHEFS_FUSE
-	if (!strcmp(cmd, "fusemount"))
-		return cmd_fusemount(argc, argv);
-#endif
-
-	if (!strcmp(cmd, "--help")) {
-		usage();
-		return 0;
-	}
-
-	printf("Unknown command %s\n", cmd);
-usage:
-	usage();
-	exit(EXIT_FAILURE);
-}
diff --git a/cmds.h b/cmds.h
index 88b03f3..64267dc 100644
--- a/cmds.h
+++ b/cmds.h
@@ -54,6 +54,10 @@ int cmd_subvolume_snapshot(int argc, char *argv[]);
 
 int cmd_fusemount(int argc, char *argv[]);
 
-int rust_main(int argc, char *argv[], char *cmd);
+void bcachefs_usage(void);
+int device_cmds(int argc, char *argv[]);
+int fs_cmds(int argc, char *argv[]);
+int data_cmds(int argc, char *argv[]);
+int subvolume_cmds(int argc, char *argv[]);
 
 #endif /* _CMDS_H */
diff --git a/rust-src/Cargo.lock b/rust-src/Cargo.lock
index 091f760..62eaa3d 100644
--- a/rust-src/Cargo.lock
+++ b/rust-src/Cargo.lock
@@ -84,7 +84,7 @@ source = "registry+https://github.com/rust-lang/crates.io-index"
 checksum = "d468802bab17cbc0cc575e9b053f41e72aa36bfa6b7f55e3529ffa43161b97fa"
 
 [[package]]
-name = "bcachefs-rust"
+name = "bcachefs-tools"
 version = "0.3.1"
 dependencies = [
  "anyhow",
diff --git a/rust-src/Cargo.toml b/rust-src/Cargo.toml
index 84107d4..1e96f85 100644
--- a/rust-src/Cargo.toml
+++ b/rust-src/Cargo.toml
@@ -1,12 +1,16 @@
 [package]
-name = "bcachefs-rust"
+name = "bcachefs-tools"
 version = "0.3.1"
 authors = ["Yuxuan Shui <yshuiv7@gmail.com>", "Kayla Firestack <dev@kaylafire.me>", "Kent Overstreet <kent.overstreet@linux.dev>" ]
 edition = "2021"
 rust-version = "1.65"
 
+[[bin]]
+name = "bcachefs"
+path = "src/bcachefs.rs"
+
 [lib]
-crate-type = ["staticlib"]
+name = "bcachefs"
 
 [dependencies]
 atty = "0.2.14"
diff --git a/rust-src/bch_bindgen/build.rs b/rust-src/bch_bindgen/build.rs
index 79337a0..d9805a8 100644
--- a/rust-src/bch_bindgen/build.rs
+++ b/rust-src/bch_bindgen/build.rs
@@ -10,6 +10,8 @@ impl bindgen::callbacks::ParseCallbacks for Fix753 {
 fn main() {
     use std::path::PathBuf;
 
+    println!("cargo:rerun-if-changed=src/libbcachefs_wrapper.h");
+
     let out_dir: PathBuf = std::env::var_os("OUT_DIR")
         .expect("ENV Var 'OUT_DIR' Expected")
         .into();
@@ -44,6 +46,10 @@ fn main() {
         .default_enum_style(bindgen::EnumVariation::Rust {
             non_exhaustive: true,
         })
+        .allowlist_function("bcachefs_usage")
+        .allowlist_function("raid_init")
+        .allowlist_function("cmd_.*")
+        .allowlist_function(".*_cmds")
         .allowlist_function(".*bch2_.*")
         .allowlist_function("bio_.*")
         .allowlist_function("derive_passphrase")
diff --git a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
index e68de66..5fb4261 100644
--- a/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
+++ b/rust-src/bch_bindgen/src/libbcachefs_wrapper.h
@@ -11,6 +11,8 @@
 #include "../crypto.h"
 #include "../include/linux/bio.h"
 #include "../include/linux/blkdev.h"
+#include "../cmds.h"
+#include "../raid/raid.h"
 
 
 #define MARK_FIX_753(req_name) const blk_mode_t Fix753_##req_name = req_name;
diff --git a/rust-src/build.rs b/rust-src/build.rs
new file mode 100644
index 0000000..e4662bd
--- /dev/null
+++ b/rust-src/build.rs
@@ -0,0 +1,21 @@
+fn main() {
+    println!("cargo:rustc-link-search=..");
+    println!("cargo:rerun-if-changed=../libbcachefs.a");
+    println!("cargo:rustc-link-lib=static:+whole-archive=bcachefs");
+
+    println!("cargo:rustc-link-lib=urcu");
+    println!("cargo:rustc-link-lib=zstd");
+    println!("cargo:rustc-link-lib=blkid");
+    println!("cargo:rustc-link-lib=uuid");
+    println!("cargo:rustc-link-lib=sodium");
+    println!("cargo:rustc-link-lib=z");
+    println!("cargo:rustc-link-lib=lz4");
+    println!("cargo:rustc-link-lib=zstd");
+    println!("cargo:rustc-link-lib=udev");
+    println!("cargo:rustc-link-lib=keyutils");
+    println!("cargo:rustc-link-lib=aio");
+
+    if std::env::var("BCACHEFS_FUSE").is_ok() {
+        println!("cargo:rustc-link-lib=fuse3");
+    }
+}
diff --git a/rust-src/src/bcachefs.rs b/rust-src/src/bcachefs.rs
new file mode 100644
index 0000000..ed5fd1d
--- /dev/null
+++ b/rust-src/src/bcachefs.rs
@@ -0,0 +1,107 @@
+use std::ffi::CString;
+
+use bcachefs::cmd_completions::cmd_completions;
+use bcachefs::cmd_list::cmd_list;
+use bcachefs::cmd_mount::cmd_mount;
+use bcachefs::logger::SimpleLogger;
+use bch_bindgen::c;
+
+fn handle_c_command(args: Vec<String>, symlink_cmd: Option<&str>) -> i32 {
+    let mut argv: Vec<_> = args.clone();
+
+    let cmd = match symlink_cmd {
+        Some(s) => s.to_string(),
+        None => argv.remove(1),
+    };
+
+    let argc: i32 = argv.len().try_into().unwrap();
+
+    let argv: Vec<_> = argv
+        .iter()
+        .map(|s| CString::new(s.as_str()).unwrap())
+        .collect();
+    let argv: Vec<_> = argv.iter().map(|s| s.as_ptr()).collect();
+    let argv = argv.as_ptr() as *mut *mut i8;
+
+    // The C functions will mutate argv. It shouldn't be used after this block.
+    unsafe {
+        match cmd.as_str() {
+            "--help" => {
+                c::bcachefs_usage();
+                0
+            },
+            "data" => c::data_cmds(argc, argv),
+            "device" => c::device_cmds(argc, argv),
+            "dump" => c::cmd_dump(argc, argv),
+            "format" => c::cmd_format(argc, argv),
+            "fs" => c::fs_cmds(argc, argv),
+            "fsck" => c::cmd_fsck(argc, argv),
+            "list_journal" => c::cmd_list_journal(argc, argv),
+            "kill_btree_node" => c::cmd_kill_btree_node(argc, argv),
+            "migrate" => c::cmd_migrate(argc, argv),
+            "migrate-superblock" => c::cmd_migrate_superblock(argc, argv),
+            "mkfs" => c::cmd_format(argc, argv),
+            "remove-passphrase" => c::cmd_remove_passphrase(argc, argv),
+            "reset-counters" => c::cmd_reset_counters(argc, argv),
+            "set-option" => c::cmd_set_option(argc, argv),
+            "set-passphrase" => c::cmd_set_passphrase(argc, argv),
+            "setattr" => c::cmd_setattr(argc, argv),
+            "show-super" => c::cmd_show_super(argc, argv),
+            "subvolume" => c::subvolume_cmds(argc, argv),
+            "unlock" => c::cmd_unlock(argc, argv),
+            "version" => c::cmd_version(argc, argv),
+
+            #[cfg(fuse)]
+            "fusemount" => c::cmd_fusemount(argc, argv),
+
+            _ => {
+                println!("Unknown command {}", cmd);
+                c::bcachefs_usage();
+                1
+            }
+        }
+    }
+}
+
+fn main() {
+    let args: Vec<String> = std::env::args().collect();
+
+    let symlink_cmd: Option<&str> = if args[0].contains("mkfs") {
+        Some("mkfs")
+    } else if args[0].contains("fsck") {
+        Some("fsck")
+    } else if args[0].contains("mount.fuse") {
+        Some("fusemount")
+    } else if args[0].contains("mount") {
+        Some("mount")
+    } else {
+        None
+    };
+
+    if symlink_cmd.is_none() && args.len() < 2 {
+        println!("missing command");
+        unsafe { c::bcachefs_usage() };
+        std::process::exit(1);
+    }
+
+    unsafe { c::raid_init() };
+
+    log::set_boxed_logger(Box::new(SimpleLogger)).unwrap();
+    log::set_max_level(log::LevelFilter::Warn);
+
+    let cmd = match symlink_cmd {
+        Some(s) => s,
+        None => args[1].as_str(),
+    };
+
+    let ret = match cmd {
+        "completions" => cmd_completions(args[1..].to_vec()),
+        "list" => cmd_list(args[1..].to_vec()),
+        "mount" => cmd_mount(args, symlink_cmd),
+        _ => handle_c_command(args, symlink_cmd),
+    };
+
+    if ret != 0 {
+        std::process::exit(1);
+    }
+}
diff --git a/rust-src/src/cmd_completions.rs b/rust-src/src/cmd_completions.rs
index 3e839fe..53cdd64 100644
--- a/rust-src/src/cmd_completions.rs
+++ b/rust-src/src/cmd_completions.rs
@@ -1,6 +1,5 @@
 use clap::{Command, CommandFactory, Parser};
 use clap_complete::{generate, Generator, Shell};
-use std::ffi::{c_int, OsStr};
 use std::io;
 
 /// Generate shell completions
@@ -13,7 +12,7 @@ fn print_completions<G: Generator>(gen: G, cmd: &mut Command) {
     generate(gen, cmd, cmd.get_name().to_string(), &mut io::stdout());
 }
 
-pub fn cmd_completions(argv: Vec<&OsStr>) -> c_int {
+pub fn cmd_completions(argv: Vec<String>) -> i32 {
     let cli = Cli::parse_from(argv);
     print_completions(cli.shell, &mut super::Cli::command());
     0
diff --git a/rust-src/src/cmd_list.rs b/rust-src/src/cmd_list.rs
index fa9c2f2..cb35291 100644
--- a/rust-src/src/cmd_list.rs
+++ b/rust-src/src/cmd_list.rs
@@ -9,7 +9,6 @@ use bch_bindgen::btree::BtreeIter;
 use bch_bindgen::btree::BtreeNodeIter;
 use bch_bindgen::btree::BtreeIterFlags;
 use clap::{Parser};
-use std::ffi::{c_int, OsStr};
 
 fn list_keys(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
     let trans = BtreeTrans::new(fs);
@@ -157,7 +156,7 @@ fn cmd_list_inner(opt: Cli) -> anyhow::Result<()> {
     }
 }
 
-pub fn cmd_list(argv: Vec<&OsStr>) -> c_int {
+pub fn cmd_list(argv: Vec<String>) -> i32 {
     let opt = Cli::parse_from(argv);
     colored::control::set_override(opt.colorize);
     if let Err(e) = cmd_list_inner(opt) {
diff --git a/rust-src/src/cmd_main.rs b/rust-src/src/cmd_main.rs
deleted file mode 100644
index baedc85..0000000
--- a/rust-src/src/cmd_main.rs
+++ /dev/null
@@ -1,34 +0,0 @@
-use log::{error, LevelFilter};
-use std::ffi::{CStr, c_int, c_char};
-use crate::transform_c_args;
-use crate::logger::SimpleLogger;
-use crate::cmd_completions::cmd_completions;
-use crate::cmd_list::cmd_list;
-use crate::cmd_mount::cmd_mount;
-
-#[no_mangle]
-pub extern "C" fn rust_main(argc: c_int, argv: *const *const c_char, cmd: *const c_char) -> c_int {
-    transform_c_args!(argv, argc, argv);
-
-    log::set_boxed_logger(Box::new(SimpleLogger)).unwrap();
-    log::set_max_level(LevelFilter::Warn);
-
-    let cmd: &CStr = unsafe { CStr::from_ptr(cmd) };
-    let cmd = match cmd.to_str() {
-        Ok(c) => c,
-        Err(e) => {
-            error!("could not parse command: {}", e);
-            return 1;
-        }
-    };
-
-    match cmd {
-        "completions" => cmd_completions(argv),
-        "list" => cmd_list(argv),
-        "mount" => cmd_mount(argv),
-        _ => {
-            error!("unknown command: {}", cmd);
-            1
-        }
-    }
-}
diff --git a/rust-src/src/cmd_mount.rs b/rust-src/src/cmd_mount.rs
index 4f6021c..3b8b01f 100644
--- a/rust-src/src/cmd_mount.rs
+++ b/rust-src/src/cmd_mount.rs
@@ -6,7 +6,7 @@ use uuid::Uuid;
 use std::path::PathBuf;
 use crate::key;
 use crate::key::KeyLocation;
-use std::ffi::{CString, c_int, c_char, c_void, OsStr};
+use std::ffi::{CString, c_char, c_void};
 use std::os::unix::ffi::OsStrExt;
 
 fn mount_inner(
@@ -223,7 +223,14 @@ fn cmd_mount_inner(opt: Cli) -> anyhow::Result<()> {
     Ok(())
 }
 
-pub fn cmd_mount(argv: Vec<&OsStr>) -> c_int {
+pub fn cmd_mount(mut argv: Vec<String>, symlink_cmd: Option<&str>) -> i32 {
+    // If the bcachefs tool is being called as "bcachefs mount dev ..." (as opposed to via a
+    // symlink like "/usr/sbin/mount.bcachefs dev ...", then we need to pop the 0th argument
+    // ("bcachefs") since the CLI parser here expects the device at position 1.
+    if symlink_cmd.is_none() {
+        argv.remove(0);
+    }
+
     let opt = Cli::parse_from(argv);
 
     // @TODO : more granular log levels via mount option
diff --git a/rust-src/src/lib.rs b/rust-src/src/lib.rs
index 026cca4..f8b508d 100644
--- a/rust-src/src/lib.rs
+++ b/rust-src/src/lib.rs
@@ -2,7 +2,6 @@ use clap::Subcommand;
 
 pub mod key;
 pub mod logger;
-pub mod cmd_main;
 pub mod cmd_mount;
 pub mod cmd_list;
 pub mod cmd_completions;
@@ -32,18 +31,6 @@ macro_rules! c_str {
     };
 }
 
-#[macro_export]
-macro_rules! transform_c_args {
-    ($var:ident, $argc:expr, $argv:expr) => {
-        // TODO: `OsStr::from_bytes` only exists on *nix
-        use ::std::os::unix::ffi::OsStrExt;
-        let $var: Vec<_> = (0..$argc)
-        .map(|i| unsafe { ::std::ffi::CStr::from_ptr(*$argv.add(i as usize)) })
-        .map(|i| ::std::ffi::OsStr::from_bytes(i.to_bytes()))
-        .collect();
-    };
-}
-
 #[derive(Debug)]
 struct ErrnoError(errno::Errno);
 impl std::fmt::Display for ErrnoError {
diff --git a/tests/util.py b/tests/util.py
index 00314f4..3ec38b2 100644
--- a/tests/util.py
+++ b/tests/util.py
@@ -11,7 +11,7 @@ import time
 from pathlib import Path
 
 BASE_PATH= os.path.dirname(__file__)
-BCH_PATH = os.path.abspath(os.path.join(BASE_PATH, '..', 'bcachefs'))
+BCH_PATH = os.path.abspath(os.path.join(BASE_PATH, '../rust-src/target/release', 'bcachefs'))
 VALGRIND_PATH= os.path.abspath(os.path.join(BASE_PATH,
     'valgrind-suppressions.txt'))
 
-- 
2.43.0

[PATCH TOOLS 2/2] remove library from bcachefs-tools Rust package

[Thomas Bertschinger]

When bcachefs was a C program that had some functions implemented in
Rust, it was necessary to make a static library containing the Rust
functions for the C program to link.

Now that bcachefs is a Rust program, that library is no longer needed.
Instead, the Rust executable links in libbachefs.a.

This patch updates the crate structure to reflect that. The command
functions are moved into their own module.

There could be a need to create a "libbachefs-tools" library in the
future that exposes an API for bcachefs functionality to other
userspace programs. That will be a different, external API as opposed to
the previous library functions which were an internal API for the
bcachefs tool itself.

Signed-off-by: Thomas Bertschinger <tahbertschinger@gmail.com>
---
 rust-src/Cargo.toml                           |  3 ---
 rust-src/src/bcachefs.rs                      | 21 +++++++++++++++----
 .../src/{ => commands}/cmd_completions.rs     |  0
 rust-src/src/{ => commands}/cmd_list.rs       |  0
 rust-src/src/{ => commands}/cmd_mount.rs      |  0
 rust-src/src/{ => commands}/logger.rs         |  0
 rust-src/src/{lib.rs => commands/mod.rs}      | 10 ---------
 7 files changed, 17 insertions(+), 17 deletions(-)
 rename rust-src/src/{ => commands}/cmd_completions.rs (100%)
 rename rust-src/src/{ => commands}/cmd_list.rs (100%)
 rename rust-src/src/{ => commands}/cmd_mount.rs (100%)
 rename rust-src/src/{ => commands}/logger.rs (100%)
 rename rust-src/src/{lib.rs => commands/mod.rs} (72%)

diff --git a/rust-src/Cargo.toml b/rust-src/Cargo.toml
index 1e96f85..f50523b 100644
--- a/rust-src/Cargo.toml
+++ b/rust-src/Cargo.toml
@@ -9,9 +9,6 @@ rust-version = "1.65"
 name = "bcachefs"
 path = "src/bcachefs.rs"
 
-[lib]
-name = "bcachefs"
-
 [dependencies]
 atty = "0.2.14"
 log = { version = "0.4", features = ["std"] }
diff --git a/rust-src/src/bcachefs.rs b/rust-src/src/bcachefs.rs
index ed5fd1d..0a76a92 100644
--- a/rust-src/src/bcachefs.rs
+++ b/rust-src/src/bcachefs.rs
@@ -1,11 +1,24 @@
+mod commands;
+mod key;
+
 use std::ffi::CString;
 
-use bcachefs::cmd_completions::cmd_completions;
-use bcachefs::cmd_list::cmd_list;
-use bcachefs::cmd_mount::cmd_mount;
-use bcachefs::logger::SimpleLogger;
+use commands::cmd_completions::cmd_completions;
+use commands::cmd_list::cmd_list;
+use commands::cmd_mount::cmd_mount;
+use commands::logger::SimpleLogger;
 use bch_bindgen::c;
 
+#[derive(Debug)]
+pub struct ErrnoError(pub errno::Errno);
+impl std::fmt::Display for ErrnoError {
+    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
+        self.0.fmt(f)
+    }
+}
+
+impl std::error::Error for ErrnoError {}
+
 fn handle_c_command(args: Vec<String>, symlink_cmd: Option<&str>) -> i32 {
     let mut argv: Vec<_> = args.clone();
 
diff --git a/rust-src/src/cmd_completions.rs b/rust-src/src/commands/cmd_completions.rs
similarity index 100%
rename from rust-src/src/cmd_completions.rs
rename to rust-src/src/commands/cmd_completions.rs
diff --git a/rust-src/src/cmd_list.rs b/rust-src/src/commands/cmd_list.rs
similarity index 100%
rename from rust-src/src/cmd_list.rs
rename to rust-src/src/commands/cmd_list.rs
diff --git a/rust-src/src/cmd_mount.rs b/rust-src/src/commands/cmd_mount.rs
similarity index 100%
rename from rust-src/src/cmd_mount.rs
rename to rust-src/src/commands/cmd_mount.rs
diff --git a/rust-src/src/logger.rs b/rust-src/src/commands/logger.rs
similarity index 100%
rename from rust-src/src/logger.rs
rename to rust-src/src/commands/logger.rs
diff --git a/rust-src/src/lib.rs b/rust-src/src/commands/mod.rs
similarity index 72%
rename from rust-src/src/lib.rs
rename to rust-src/src/commands/mod.rs
index f8b508d..e05a084 100644
--- a/rust-src/src/lib.rs
+++ b/rust-src/src/commands/mod.rs
@@ -1,6 +1,5 @@
 use clap::Subcommand;
 
-pub mod key;
 pub mod logger;
 pub mod cmd_mount;
 pub mod cmd_list;
@@ -30,12 +29,3 @@ macro_rules! c_str {
         }
     };
 }
-
-#[derive(Debug)]
-struct ErrnoError(errno::Errno);
-impl std::fmt::Display for ErrnoError {
-    fn fmt(&self, f: &mut std::fmt::Formatter) -> Result<(), std::fmt::Error> {
-        self.0.fmt(f)
-    }
-}
-impl std::error::Error for ErrnoError {}
-- 
2.43.0

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

On Sun, Jan 14, 2024 at 10:24:49PM -0700, Thomas Bertschinger wrote:
> This converts the top-level main() of the bcachefs command line tool
> from C to Rust.
> 
> The first patch does all the work of implementing a new main(), updating
> the build process, and making things look for the executable in its new
> location which is now rust-src/target/release/bcachefs.
> 
> The second patch removes the Library crate from the Rust package. This
> was needed previously to allow the C program to link in the Rust
> functions, but that doesn't happen anymore.
> 
> It seems likely that a bcachefs-tools library will be needed in the
> future; for example see this request [1] on GitHub. This library will
> have a different external-focused API as opposed to the previous library
> that provided an internal API for the bcachefs tool itself. Considering
> Rust's structure of one library crate per package, removing the current
> library makes room for that requested library.
> 
> I tried to update everything to account for the new location of the
> bcachefs executable, but let me know if anyone sees something I missed.
> I could not figure out how this is set for the Debian package so it's
> possible I missed something there...
> 
> [1] https://github.com/koverstreet/bcachefs-tools/issues/188

Nice work :)

I think there's some more rearraging we could do - it'd be cleaner if we
moved the C sources out of the top level directory, perhaps to c_src,
and then moved everything in rust-src/ to the toplevel.

Top level directory is getting a bit crowded, and then the new bcachefs
binary won't be quite so out of the way; I frequently build and run
without installing when I'm debugging.

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

On Sun, Jan 14, 2024 at 10:24:49PM -0700, Thomas Bertschinger wrote:
> This converts the top-level main() of the bcachefs command line tool
> from C to Rust.
> 
> The first patch does all the work of implementing a new main(), updating
> the build process, and making things look for the executable in its new
> location which is now rust-src/target/release/bcachefs.
> 
> The second patch removes the Library crate from the Rust package. This
> was needed previously to allow the C program to link in the Rust
> functions, but that doesn't happen anymore.
> 
> It seems likely that a bcachefs-tools library will be needed in the
> future; for example see this request [1] on GitHub. This library will
> have a different external-focused API as opposed to the previous library
> that provided an internal API for the bcachefs tool itself. Considering
> Rust's structure of one library crate per package, removing the current
> library makes room for that requested library.
> 
> I tried to update everything to account for the new location of the
> bcachefs executable, but let me know if anyone sees something I missed.
> I could not figure out how this is set for the Debian package so it's
> possible I missed something there...
> 
> [1] https://github.com/koverstreet/bcachefs-tools/issues/188

We've got a new CI failure; it looks like perhaps the MSRV environment
variable isn't being set correctly in the test environment. Can you take
a look?

https://github.com/koverstreet/bcachefs-tools/actions/runs/7532280737/job/20502558560

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Thomas Bertschinger]

On Mon, Jan 15, 2024 at 12:32:23PM -0500, Kent Overstreet wrote:
> I think there's some more rearraging we could do - it'd be cleaner if we
> moved the C sources out of the top level directory, perhaps to c_src,
> and then moved everything in rust-src/ to the toplevel.
> 
> Top level directory is getting a bit crowded, and then the new bcachefs
> binary won't be quite so out of the way; I frequently build and run
> without installing when I'm debugging.

I agree this makes sense. I'll work on a v2 that does this.

Thoughts on keeping Make as the top-level build tool vs. using Cargo? I
am still fairly new to Rust/Cargo so I'm not sure how easy it would be
to replicate all of the Makefile targets in Cargo. It does appear to be
possible to use Cargo to invoke gcc to build the C library.

I can see three approaches:
- keep Make as the primary build tool
- hybrid: use Cargo for source code building but keep the Makefile around
  for the supplemental targets like "make doc", "make rpm", etc.
- use Cargo for everything (not sure if this is easy / possible, but I
  could look into it if desired)

I'm leaning towards keeping Make as the primary build tool for now and
perhaps a future patch can move towards using Cargo. But other opinions
are welcome.

- Thomas Bertschinger

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Thomas Bertschinger]

On Mon, Jan 15, 2024 at 12:48:28PM -0500, Kent Overstreet wrote:
> We've got a new CI failure; it looks like perhaps the MSRV environment
> variable isn't being set correctly in the test environment. Can you take
> a look?
> 
> https://github.com/koverstreet/bcachefs-tools/actions/runs/7532280737/job/20502558560

Sure, I'll take a look.

- Thomas Bertschinger

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> On Mon, Jan 15, 2024 at 12:32:23PM -0500, Kent Overstreet wrote:
> > I think there's some more rearraging we could do - it'd be cleaner if we
> > moved the C sources out of the top level directory, perhaps to c_src,
> > and then moved everything in rust-src/ to the toplevel.
> > 
> > Top level directory is getting a bit crowded, and then the new bcachefs
> > binary won't be quite so out of the way; I frequently build and run
> > without installing when I'm debugging.
> 
> I agree this makes sense. I'll work on a v2 that does this.
> 
> Thoughts on keeping Make as the top-level build tool vs. using Cargo? I
> am still fairly new to Rust/Cargo so I'm not sure how easy it would be
> to replicate all of the Makefile targets in Cargo. It does appear to be
> possible to use Cargo to invoke gcc to build the C library.
#
> I can see three approaches:
> - keep Make as the primary build tool
> - hybrid: use Cargo for source code building but keep the Makefile around
>   for the supplemental targets like "make doc", "make rpm", etc.
> - use Cargo for everything (not sure if this is easy / possible, but I
>   could look into it if desired)
> 
> I'm leaning towards keeping Make as the primary build tool for now and
> perhaps a future patch can move towards using Cargo. But other opinions
> are welcome.

Let's hold off on that for now.

I'm planning on pullling the Rust code in bch_bindgen into the kernel
soon, and I want to see how that interacts with the kernel build system.
If there's potential for commonality with the kernel build system, we
might want to take advantage of that.

(Also, if you're interested in that project, let me know! Getting that
ready for the next merge window is a high priority item, but I have a
lot of high priority items at the moment... :)

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Thomas Bertschinger]

On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > I'm leaning towards keeping Make as the primary build tool for now and
> > perhaps a future patch can move towards using Cargo. But other opinions
> > are welcome.
> 
> Let's hold off on that for now.
> 
> I'm planning on pullling the Rust code in bch_bindgen into the kernel
> soon, and I want to see how that interacts with the kernel build system.
> If there's potential for commonality with the kernel build system, we
> might want to take advantage of that.
> 
> (Also, if you're interested in that project, let me know! Getting that
> ready for the next merge window is a high priority item, but I have a
> lot of high priority items at the moment... :)

Sure, I would definitely be interested in helping out with that. I don't
want to make any promises regarding timeline, since this combines two
worlds--Rust, and Linux Kernel development--that are both still very
new to me. But I can certainly give it an honest try :) as long as you,
or someone else experienced, can help with any questions I might run
into.

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

On Mon, Jan 15, 2024 at 12:10:22PM -0700, Thomas Bertschinger wrote:
> On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> > On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > > I'm leaning towards keeping Make as the primary build tool for now and
> > > perhaps a future patch can move towards using Cargo. But other opinions
> > > are welcome.
> > 
> > Let's hold off on that for now.
> > 
> > I'm planning on pullling the Rust code in bch_bindgen into the kernel
> > soon, and I want to see how that interacts with the kernel build system.
> > If there's potential for commonality with the kernel build system, we
> > might want to take advantage of that.
> > 
> > (Also, if you're interested in that project, let me know! Getting that
> > ready for the next merge window is a high priority item, but I have a
> > lot of high priority items at the moment... :)
> 
> Sure, I would definitely be interested in helping out with that. I don't
> want to make any promises regarding timeline, since this combines two
> worlds--Rust, and Linux Kernel development--that are both still very
> new to me. But I can certainly give it an honest try :) as long as you,
> or someone else experienced, can help with any questions I might run
> into.

Of course :)

Join the IRC channel - irc.oftc.net#bcache, we've got quite a few people
acquainted with Rust there.

The main thing that needs to be sorted out is that we require a patched
version of bindgen (since rustc can't yet handle types that are both
packed and aligned); we need to talk to the rust-for-linux people about
whether they'll be ok with switching the kernel to the patched bindgen
until we can get a proper fix into rustc.

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Trevor Gross]

On Mon, Jan 15, 2024 at 1:22 PM Kent Overstreet
<kent.overstreet@linux.dev> wrote:
>
> On Mon, Jan 15, 2024 at 12:10:22PM -0700, Thomas Bertschinger wrote:
> > On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> > > On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > > > I'm leaning towards keeping Make as the primary build tool for now and
> > > > perhaps a future patch can move towards using Cargo. But other opinions
> > > > are welcome.
> > >
> > > Let's hold off on that for now.
> > >
> > > I'm planning on pullling the Rust code in bch_bindgen into the kernel
> > > soon, and I want to see how that interacts with the kernel build system.
> > > If there's potential for commonality with the kernel build system, we
> > > might want to take advantage of that.
> > >
> > > (Also, if you're interested in that project, let me know! Getting that
> > > ready for the next merge window is a high priority item, but I have a
> > > lot of high priority items at the moment... :)
> >
> > Sure, I would definitely be interested in helping out with that. I don't
> > want to make any promises regarding timeline, since this combines two
> > worlds--Rust, and Linux Kernel development--that are both still very
> > new to me. But I can certainly give it an honest try :) as long as you,
> > or someone else experienced, can help with any questions I might run
> > into.
>
> Of course :)
>
> Join the IRC channel - irc.oftc.net#bcache, we've got quite a few people
> acquainted with Rust there.
>
> The main thing that needs to be sorted out is that we require a patched
> version of bindgen (since rustc can't yet handle types that are both
> packed and aligned); we need to talk to the rust-for-linux people about
> whether they'll be ok with switching the kernel to the patched bindgen
> until we can get a proper fix into rustc.
>

Do you have a link to the patches needed? We are wondering if this is
something that could be upstreamed.

Miguel found [1] which seems to be the rustc fix but I am curious how
you work around this.

[1]: https://github.com/rust-lang/rust/issues/59154#issuecomment-1899563996

Re: [PATCH TOOLS 0/2] convert main() from C to Rust

[Kent Overstreet]

On Fri, Jan 19, 2024 at 01:05:17PM -0600, Trevor Gross wrote:
> On Mon, Jan 15, 2024 at 1:22 PM Kent Overstreet
> <kent.overstreet@linux.dev> wrote:
> >
> > On Mon, Jan 15, 2024 at 12:10:22PM -0700, Thomas Bertschinger wrote:
> > > On Mon, Jan 15, 2024 at 01:45:22PM -0500, Kent Overstreet wrote:
> > > > On Mon, Jan 15, 2024 at 10:55:09AM -0700, Thomas Bertschinger wrote:
> > > > > I'm leaning towards keeping Make as the primary build tool for now and
> > > > > perhaps a future patch can move towards using Cargo. But other opinions
> > > > > are welcome.
> > > >
> > > > Let's hold off on that for now.
> > > >
> > > > I'm planning on pullling the Rust code in bch_bindgen into the kernel
> > > > soon, and I want to see how that interacts with the kernel build system.
> > > > If there's potential for commonality with the kernel build system, we
> > > > might want to take advantage of that.
> > > >
> > > > (Also, if you're interested in that project, let me know! Getting that
> > > > ready for the next merge window is a high priority item, but I have a
> > > > lot of high priority items at the moment... :)
> > >
> > > Sure, I would definitely be interested in helping out with that. I don't
> > > want to make any promises regarding timeline, since this combines two
> > > worlds--Rust, and Linux Kernel development--that are both still very
> > > new to me. But I can certainly give it an honest try :) as long as you,
> > > or someone else experienced, can help with any questions I might run
> > > into.
> >
> > Of course :)
> >
> > Join the IRC channel - irc.oftc.net#bcache, we've got quite a few people
> > acquainted with Rust there.
> >
> > The main thing that needs to be sorted out is that we require a patched
> > version of bindgen (since rustc can't yet handle types that are both
> > packed and aligned); we need to talk to the rust-for-linux people about
> > whether they'll be ok with switching the kernel to the patched bindgen
> > until we can get a proper fix into rustc.
> >
> 
> Do you have a link to the patches needed? We are wondering if this is
> something that could be upstreamed.

The current workaround is to just drop #[repr(align)] if the type is
both packed and aligned - but that leads to rustc and gcc disagreeing on
the memory layout of certain types; it only works as long as we're not
mutating types where this matters from rust code.

packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Thomas Bertschinger]

On Fri, Jan 19, 2024 at 04:37:51PM -0500, Kent Overstreet wrote:
> On Fri, Jan 19, 2024 at 01:05:17PM -0600, Trevor Gross wrote:
> > On Mon, Jan 15, 2024 at 1:22 PM Kent Overstreet
> > <kent.overstreet@linux.dev> wrote:
> > > The main thing that needs to be sorted out is that we require a patched
> > > version of bindgen (since rustc can't yet handle types that are both
> > > packed and aligned); we need to talk to the rust-for-linux people about
> > > whether they'll be ok with switching the kernel to the patched bindgen
> > > until we can get a proper fix into rustc.
> > 
> > Do you have a link to the patches needed? We are wondering if this is
> > something that could be upstreamed.
> 
> The current workaround is to just drop #[repr(align)] if the type is
> both packed and aligned - but that leads to rustc and gcc disagreeing on
> the memory layout of certain types; it only works as long as we're not
> mutating types where this matters from rust code.

I made a script to compare the size and alignment of bcachefs structs
in C vs. in Rust generated by the patched, lossy bindgen. All sizes
were the same, but the following types had different alignment:

bkey			(8 in C, 4 in Rust)
bch_extent_crc32	(8 in C, 4 in Rust)
bch_extent_ptr		(8 in C, 1 in Rust)

I didn't explicitly compare the offsets of all struct members between
Rust and C as I couldn't come up with a trivial way to automate this.
However, I think that if the C struct is packed, and the Rust and C
structs have the same size, you can conclude that all member offsets
are the same. This is because Rust could not increase the offset of a
member (or else the Rust size would be larger), it could not decrease
the offset of a member (because it's already packed), and it could not
reorder the members (because #[repr(C)] prohibits this in Rust).

Given the above logic, I think it's safe to conclude that if a struct
is packed in C, and the size and alignment are the same between C and
Rust, then they are fully ABI compatible. Does anyone see an error in
my reasoning?


Taking a look at struct bkey, the alignment difference arises from a
semantic difference between gcc's __attribute__((packed, aligned(N)))
and rustc's #[repr(C, packed(N))]. In Rust, 

> By specifying this attribute, the alignment of the struct would be
> the smaller of the specified packing and the default alignment of the
> struct. [1]

We can get the same alignment for struct bkey in Rust by modifying
the code from bindgen to use align(8) instead of packed(8). On my
x86_64 Linux system, this results in a struct with the same size,
alignment, and offsets of all members as the original C struct. This
works because the struct happens to be packed already, so the
#[repr(packed)] attribute is superfluous. I can't say if this works in
any other environment with different architecture, endianness, etc. But
introducing an automated test for comparing the size and alignment of
bindgen-generated structs could help with validating other environments.


In general, given a C structure like

struct A {
	...
} __packed __aligned(N);

I think a Rust structure can be created with the same size, alignment,
and member offsets in the following cases: 

(1) #[repr(packed(N))] will have the same layout if N is <= the
    structure's default alignment and all members with a default
    alignment >= N naturally have an offset that is a multiple of N.
    Also, no member can have an alignment attribute as rustc forbids
    this [2].

(2) #[repr(align(N))] will have the same layout if the structure's size
    without the packed attribute is equal to the sum of the size of all
    its members (i.e., it is "naturally packed"), and the structure's
    default alignment is <= N.

(I haven't formally proven these rules and I welcome counterexamples if
someone can spot an error in my logic.)

An example of a struct that I think the current rustc cannot represent:

struct rust_cannot_represent_this {
	u8 a;
	u32 b;
} __packed __aligned(2);

Luckily, I do not think bcachefs has any structs like this.


TL;DR -- even without a rustc fix to support packed and aligned structs,
I think it is possible in many cases to create Rust structs with the
same memory layout, although it may take some manual effort. Perhaps
bindgen could be enhanced with awareness of rule (2) from above, to
automatically cover more cases than it does now?

Of course, a fix to rustc is the best option. It would avoid the need
for error-prone manual work. Specifying both #[repr(packed, align(N))]
also communicates semantic intent more than specifying just one even
when the result is the same. But we could be waiting a while for that
fix to arrive.

[1] https://rust-lang.github.io/rfcs/1399-repr-pack.html
[2] https://github.com/rust-lang/rust/issues/100743

- Thomas Bertschinger

Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Kent Overstreet]

On Sun, Jan 21, 2024 at 09:11:24AM -0700, Thomas Bertschinger wrote:
> On Fri, Jan 19, 2024 at 04:37:51PM -0500, Kent Overstreet wrote:
> > On Fri, Jan 19, 2024 at 01:05:17PM -0600, Trevor Gross wrote:
> > > On Mon, Jan 15, 2024 at 1:22 PM Kent Overstreet
> > > <kent.overstreet@linux.dev> wrote:
> > > > The main thing that needs to be sorted out is that we require a patched
> > > > version of bindgen (since rustc can't yet handle types that are both
> > > > packed and aligned); we need to talk to the rust-for-linux people about
> > > > whether they'll be ok with switching the kernel to the patched bindgen
> > > > until we can get a proper fix into rustc.
> > > 
> > > Do you have a link to the patches needed? We are wondering if this is
> > > something that could be upstreamed.
> > 
> > The current workaround is to just drop #[repr(align)] if the type is
> > both packed and aligned - but that leads to rustc and gcc disagreeing on
> > the memory layout of certain types; it only works as long as we're not
> > mutating types where this matters from rust code.
> 
> I made a script to compare the size and alignment of bcachefs structs
> in C vs. in Rust generated by the patched, lossy bindgen. All sizes
> were the same, but the following types had different alignment:
> 
> bkey			(8 in C, 4 in Rust)
> bch_extent_crc32	(8 in C, 4 in Rust)
> bch_extent_ptr		(8 in C, 1 in Rust)

That right there is really good news. If we can add that script to the
tests in bcachefs-tools, we'll already be in better shape than we were.

I wonder if it would be possible to upstream that check into bindgen.

Also, note: the differing alignment isn't a correctness issue, but it is
definitely a performance issue on arm - the compiler will then have to
generate code to handle misaligned loads and stores.

> I didn't explicitly compare the offsets of all struct members between
> Rust and C as I couldn't come up with a trivial way to automate this.
> However, I think that if the C struct is packed, and the Rust and C
> structs have the same size, you can conclude that all member offsets
> are the same. This is because Rust could not increase the offset of a
> member (or else the Rust size would be larger), it could not decrease
> the offset of a member (because it's already packed), and it could not
> reorder the members (because #[repr(C)] prohibits this in Rust).
> 
> Given the above logic, I think it's safe to conclude that if a struct
> is packed in C, and the size and alignment are the same between C and
> Rust, then they are fully ABI compatible. Does anyone see an error in
> my reasoning?

Agreed. It'd be great to have an easy way of checking member offsets as
well, like we were discussing on IRC, but I don't think it's necessary.

> Taking a look at struct bkey, the alignment difference arises from a
> semantic difference between gcc's __attribute__((packed, aligned(N)))
> and rustc's #[repr(C, packed(N))]. In Rust, 
> 
> > By specifying this attribute, the alignment of the struct would be
> > the smaller of the specified packing and the default alignment of the
> > struct. [1]
> 
> We can get the same alignment for struct bkey in Rust by modifying
> the code from bindgen to use align(8) instead of packed(8). On my
> x86_64 Linux system, this results in a struct with the same size,
> alignment, and offsets of all members as the original C struct. This
> works because the struct happens to be packed already, so the
> #[repr(packed)] attribute is superfluous. I can't say if this works in
> any other environment with different architecture, endianness, etc. But
> introducing an automated test for comparing the size and alignment of
> bindgen-generated structs could help with validating other environments.
> 
> 
> In general, given a C structure like
> 
> struct A {
> 	...
> } __packed __aligned(N);
> 
> I think a Rust structure can be created with the same size, alignment,
> and member offsets in the following cases: 
> 
> (1) #[repr(packed(N))] will have the same layout if N is <= the
>     structure's default alignment and all members with a default
>     alignment >= N naturally have an offset that is a multiple of N.
>     Also, no member can have an alignment attribute as rustc forbids
>     this [2].
> 
> (2) #[repr(align(N))] will have the same layout if the structure's size
>     without the packed attribute is equal to the sum of the size of all
>     its members (i.e., it is "naturally packed"), and the structure's
>     default alignment is <= N.
> 
> (I haven't formally proven these rules and I welcome counterexamples if
> someone can spot an error in my logic.)

That feels right to me; and if we can get your test committed I won't be
too concerned about getting a formal proof :)

> An example of a struct that I think the current rustc cannot represent:
> 
> struct rust_cannot_represent_this {
> 	u8 a;
> 	u32 b;
> } __packed __aligned(2);
> 
> Luckily, I do not think bcachefs has any structs like this.

*nod*

> TL;DR -- even without a rustc fix to support packed and aligned structs,
> I think it is possible in many cases to create Rust structs with the
> same memory layout, although it may take some manual effort. Perhaps
> bindgen could be enhanced with awareness of rule (2) from above, to
> automatically cover more cases than it does now?
> 
> Of course, a fix to rustc is the best option. It would avoid the need
> for error-prone manual work. Specifying both #[repr(packed, align(N))]
> also communicates semantic intent more than specifying just one even
> when the result is the same. But we could be waiting a while for that
> fix to arrive.
> 
> [1] https://rust-lang.github.io/rfcs/1399-repr-pack.html
> [2] https://github.com/rust-lang/rust/issues/100743

Thanks for the writeup, this really helps.

Agreed that if at all possible we should aim for better support from
rustc, and forwarding this to the rustc people might help get this the
necessary attention before we dive in with patching bindgen.

Another thought - the interface between bindgen and rustc feels a bit
fragile here, since the C attributes and Rust reprs don't exactly match.
Can we come up with something better?

I wonder if we could get support in rustc for specifying the offset of
each member directly?

Also - you tracked down the difference between microsoft and gcc
__attribute__((align)), let me try to recap (and tell me if I get it
wrong): with gcc, __align() works like any naturally aligned type, but
Microsoft's version overrides __packed on a containing structure.

I think there's a strong argument to be made that Microsoft is the weird
one here and Rustc should just provide the gcc behaviour when mixing
packed and align...

Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Thomas Bertschinger]

On Sun, Jan 21, 2024 at 01:19:24PM -0500, Kent Overstreet wrote:
> On Sun, Jan 21, 2024 at 09:11:24AM -0700, Thomas Bertschinger wrote:
> > I made a script to compare the size and alignment of bcachefs structs
> > in C vs. in Rust generated by the patched, lossy bindgen. All sizes
> > were the same, but the following types had different alignment:
> > 
> That right there is really good news. If we can add that script to the
> tests in bcachefs-tools, we'll already be in better shape than we were.
> 
> I wonder if it would be possible to upstream that check into bindgen.

I did this with an awk script that grabs the structs from the bcachefs
C headers and outputs code in Rust and C to dump the sizes and
alignments. I then manually added calls to the generated functions into
the bcachefs utility and diffed the output. I'll try to get this into a
form more suitable for an automated test, and hopefully submit a patch
to bcachefs-tools soon...

Enhancing bindgen to do something like this automatically also sounds
like a good idea but it will take me longer to figure out how to do
that. I think it could also be good to incorporate explicit member
offset checks with this. Handling that with an awk script or similar
seems challenging to me with how bindgen mangles bitfield names.

> Also - you tracked down the difference between microsoft and gcc
> __attribute__((align)), let me try to recap (and tell me if I get it
> wrong): with gcc, __align() works like any naturally aligned type, but
> Microsoft's version overrides __packed on a containing structure.
> 
> I think there's a strong argument to be made that Microsoft is the weird
> one here and Rustc should just provide the gcc behaviour when mixing
> packed and align...

I don't have access to a Microsoft environment to test anything, so
everything that I think I know about Microsoft semantics is based on what
I've read on various rust-lang threads, like [1][2]

With that disclaimer, from what I can tell it seems like Microsoft's
semantics are strictly less expressive than gcc's. I think gcc can
represent any struct that Microsoft can (adding padding by hand
may be necessary in some cases, but it's always possible), whereas
there are structures representable in gcc that cannot be represented
with the Microsoft semantics as presented in the cited rust-lang
threads. (Given that I haven't tested the MS compiler, I could be
totally wrong about this.)

[1] https://github.com/rust-lang/rust/issues/59154#issuecomment-476408300
[2] https://github.com/rust-lang/rust/issues/33158

- Thomas Bertschinger

Re: packed, aligned structs in bcachefs (was: Re: [PATCH TOOLS 0/2] convert main() from C to Rust)

[Thomas Bertschinger]

On Sun, Jan 21, 2024 at 12:32:53PM -0700, Thomas Bertschinger wrote:
> On Sun, Jan 21, 2024 at 01:19:24PM -0500, Kent Overstreet wrote:
> > On Sun, Jan 21, 2024 at 09:11:24AM -0700, Thomas Bertschinger wrote:
> > > I made a script to compare the size and alignment of bcachefs structs
> > > in C vs. in Rust generated by the patched, lossy bindgen. All sizes
> > > were the same, but the following types had different alignment:
> > > 
> > That right there is really good news. If we can add that script to the
> > tests in bcachefs-tools, we'll already be in better shape than we were.
> > 
> > I wonder if it would be possible to upstream that check into bindgen.
> 
> I did this with an awk script that grabs the structs from the bcachefs
> C headers and outputs code in Rust and C to dump the sizes and
> alignments. I then manually added calls to the generated functions into
> the bcachefs utility and diffed the output. I'll try to get this into a
> form more suitable for an automated test, and hopefully submit a patch
> to bcachefs-tools soon...

So after looking into bindgen more... it turns out that bindgen already
does this! All you have to do is run "cargo test" from the
bcachefs-tools/bch_bindgen/ directory.

Reading the documentation helps :)

The bindgen tests basically agree with my findings, that struct bkey
gets the wrong alignment (4 instead of 8). There's one other problem
that follows from this, btree_iter.k (a struct bkey) gets the wrong
offset.

Manually adjusting struct bkey to use #[repr(align(8))] as described
previously fixes both of these problems.

The other problem types mentioned earlier, bch_extent_crc32 and
bch_extent_ptr, are handled manually outside of bindgen so they don't
show up in the bindgen test. However, they can be moved under bindgen
control and fixed with the same technique, using align(8) instead of
packed(8).

This works immediately for bch_extent_crc32 but bch_extent_ptr is a
little trickier. It is a member of struct btree_node which has a
"packed" attribute so we run into rustc disallowing embedding aligned
types within packed types. Luckily, replacing the packed(8) attribute
on struct btree_node with align(8) appears unproblematic.

Given all that, I think it should be possible to move forward with an
unpatched bindgen as long as we're OK with treating a few types 
specially. We can move all of the above types under bindgen control (so
that they are covered by the automatic tests) and post-process the
bindgen output, as suggested in this comment [1], and use align(8)
instead of packed(8) for these types. 

What do you think?

[1] https://github.com/koverstreet/bcachefs-tools/issues/202#issuecomment-1886962791

- Thomas Bertschinger