Merge pull request #285 from tmuehlbacher/fix-pedantic-clippy-lints

Fix pedantic clippy lints

flake.nix

@@ -57,21 +57,29 @@
           ...
         }:
         let
+          inherit (builtins) readFile split;
+          inherit (lib.lists) findFirst;
+          inherit (lib.strings) hasPrefix removePrefix substring;
+
           cargoToml = builtins.fromTOML (builtins.readFile ./Cargo.toml);
           rustfmtToml = builtins.fromTOML (builtins.readFile ./rustfmt.toml);
 
           craneLib = crane.mkLib pkgs;
 
-          commit = lib.strings.substring 0 7 (builtins.readFile ./.bcachefs_revision);
+          libbcachefsCommit = substring 0 7 (builtins.readFile ./.bcachefs_revision);
+          makefileVersion = removePrefix "VERSION=" (
+            findFirst (line: hasPrefix "VERSION=" line) "VERSION=0.0.0" (split "\n" (readFile ./Makefile))
+          );
+          version = "${makefileVersion}+git-${libbcachefsCommit}";
 
           commonArgs = {
-            version = "git-${commit}";
+            inherit version;
             src = self;
 
             makeFlags = [
               "DESTDIR=${placeholder "out"}"
               "PREFIX="
-              "VERSION=${commit}"
+              "VERSION=${version}"
             ];
 
             dontStrip = true;
@@ -116,7 +124,7 @@
               installCheckPhase = ''
                 runHook preInstallCheck
 
-                test "$($out/bin/bcachefs version)" = "${commit}"
+                test "$($out/bin/bcachefs version)" = "${version}"
 
                 runHook postInstallCheck
               '';

src/bcachefs.rs

@@ -28,7 +28,7 @@ fn handle_c_command(mut argv: Vec<String>, symlink_cmd: Option<&str>) -> i32 {
     let argv: Vec<_> = argv.into_iter().map(|s| CString::new(s).unwrap()).collect();
     let mut argv = argv
         .into_iter()
-        .map(|s| Box::into_raw(s.into_boxed_c_str()) as *mut c_char)
+        .map(|s| Box::into_raw(s.into_boxed_c_str()).cast::<c_char>())
         .collect::<Box<[*mut c_char]>>();
     let argv = argv.as_mut_ptr();
 
@@ -63,7 +63,7 @@ fn handle_c_command(mut argv: Vec<String>, symlink_cmd: Option<&str>) -> i32 {
             "fusemount" => c::cmd_fusemount(argc, argv),
 
             _ => {
-                println!("Unknown command {}", cmd);
+                println!("Unknown command {cmd}");
                 c::bcachefs_usage();
                 1
             }

src/commands/list.rs

@@ -10,7 +10,7 @@ use clap::Parser;
 use log::error;
 use std::io::{stdout, IsTerminal};
 
-fn list_keys(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
+fn list_keys(fs: &Fs, opt: &Cli) -> anyhow::Result<()> {
     let trans = BtreeTrans::new(fs);
     let mut iter = BtreeIter::new(
         &trans,
@@ -38,7 +38,7 @@ fn list_keys(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
     Ok(())
 }
 
-fn list_btree_formats(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
+fn list_btree_formats(fs: &Fs, opt: &Cli) -> anyhow::Result<()> {
     let trans = BtreeTrans::new(fs);
     let mut iter = BtreeNodeIter::new(
         &trans,
@@ -61,7 +61,7 @@ fn list_btree_formats(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
     Ok(())
 }
 
-fn list_btree_nodes(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
+fn list_btree_nodes(fs: &Fs, opt: &Cli) -> anyhow::Result<()> {
     let trans = BtreeTrans::new(fs);
     let mut iter = BtreeNodeIter::new(
         &trans,
@@ -84,7 +84,7 @@ fn list_btree_nodes(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
     Ok(())
 }
 
-fn list_nodes_ondisk(fs: &Fs, opt: Cli) -> anyhow::Result<()> {
+fn list_nodes_ondisk(fs: &Fs, opt: &Cli) -> anyhow::Result<()> {
     let trans = BtreeTrans::new(fs);
     let mut iter = BtreeNodeIter::new(
         &trans,
@@ -157,8 +157,8 @@ pub struct Cli {
     devices: Vec<std::path::PathBuf>,
 }
 
-fn cmd_list_inner(opt: Cli) -> anyhow::Result<()> {
+fn cmd_list_inner(opt: &Cli) -> anyhow::Result<()> {
-    let mut fs_opts: bcachefs::bch_opts = Default::default();
+    let mut fs_opts = bcachefs::bch_opts::default();
 
     opt_set!(fs_opts, nochanges, 1);
     opt_set!(fs_opts, read_only, 1);
@@ -197,7 +197,7 @@ fn cmd_list_inner(opt: Cli) -> anyhow::Result<()> {
 pub fn 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) {
+    if let Err(e) = cmd_list_inner(&opt) {
         error!("Fatal error: {}", e);
         1
     } else {

src/commands/mod.rs

@@ -27,13 +27,27 @@ enum Subcommands {
     Subvolume(subvolume::Cli),
 }
 
+// FIXME: Can be removed after bumping MSRV >= 1.77 in favor of `c""` literals
 #[macro_export]
 macro_rules! c_str {
     ($lit:expr) => {
-        unsafe {
+        ::std::ffi::CStr::from_bytes_with_nul(concat!($lit, "\0").as_bytes())
-            std::ffi::CStr::from_ptr(concat!($lit, "\0").as_ptr() as *const std::os::raw::c_char)
+            .unwrap()
-                .to_bytes_with_nul()
+            .as_ptr()
-                .as_ptr() as *const std::os::raw::c_char
-        }
     };
 }
+
+#[cfg(test)]
+mod tests {
+    use std::ffi::CStr;
+
+    #[test]
+    fn check_cstr_macro() {
+        let literal = c_str!("hello");
+
+        assert_eq!(
+            literal,
+            CStr::from_bytes_with_nul(b"hello\0").unwrap().as_ptr()
+        );
+    }
+}

src/commands/mount.rs

@@ -1,9 +1,11 @@
 use std::{
     collections::HashMap,
-    ffi::{c_char, c_void, CString},
+    env,
+    ffi::CString,
+    fs,
     io::{stdout, IsTerminal},
     path::{Path, PathBuf},
-    {env, fs, str},
+    ptr, str,
 };
 
 use anyhow::{ensure, Result};
@@ -28,12 +30,10 @@ fn mount_inner(
     let fstype = CString::new(fstype)?;
 
     // convert to pointers for ffi
-    let src = src.as_c_str().to_bytes_with_nul().as_ptr() as *const c_char;
+    let src = src.as_ptr();
-    let target = target.as_c_str().to_bytes_with_nul().as_ptr() as *const c_char;
+    let target = target.as_ptr();
-    let data = data.as_ref().map_or(std::ptr::null(), |data| {
+    let data = data.map_or(ptr::null(), |data| data.as_ptr().cast());
-        data.as_c_str().to_bytes_with_nul().as_ptr() as *const c_void
+    let fstype = fstype.as_ptr();
-    });
-    let fstype = fstype.as_c_str().to_bytes_with_nul().as_ptr() as *const c_char;
 
     let ret = {
         info!("mounting filesystem");
@@ -49,7 +49,8 @@ fn mount_inner(
 /// Parse a comma-separated mount options and split out mountflags and filesystem
 /// specific options.
 fn parse_mount_options(options: impl AsRef<str>) -> (Option<String>, libc::c_ulong) {
-    use either::Either::*;
+    use either::Either::{Left, Right};
+
     debug!("parsing mount options: {}", options.as_ref());
     let (opts, flags) = options
         .as_ref()
@@ -66,10 +67,9 @@ fn parse_mount_options(options: impl AsRef<str>) -> (Option<String>, libc::c_ulo
             "relatime" => Left(libc::MS_RELATIME),
             "remount" => Left(libc::MS_REMOUNT),
             "ro" => Left(libc::MS_RDONLY),
-            "rw" => Left(0),
+            "rw" | "" => Left(0),
             "strictatime" => Left(libc::MS_STRICTATIME),
             "sync" => Left(libc::MS_SYNCHRONOUS),
-            "" => Left(0),
             o => Right(o),
         })
         .fold((Vec::new(), 0), |(mut opts, flags), next| match next {
@@ -127,7 +127,7 @@ fn udev_bcachefs_info() -> anyhow::Result<HashMap<String, Vec<String>>> {
 
     for m in udev
         .scan_devices()?
-        .filter(|dev| dev.is_initialized())
+        .filter(udev::Device::is_initialized)
         .map(|dev| device_property_map(&dev))
         .filter(|m| m.contains_key("ID_FS_UUID") && m.contains_key("DEVNAME"))
     {
@@ -140,11 +140,8 @@ fn udev_bcachefs_info() -> anyhow::Result<HashMap<String, Vec<String>>> {
     Ok(info)
 }
 
-fn get_super_blocks(
+fn get_super_blocks(uuid: Uuid, devices: &[String]) -> Vec<(PathBuf, bch_sb_handle)> {
-    uuid: Uuid,
+    devices
-    devices: &[String],
-) -> anyhow::Result<Vec<(PathBuf, bch_sb_handle)>> {
-    Ok(devices
         .iter()
         .filter_map(|dev| {
             read_super_silent(PathBuf::from(dev))
@@ -152,7 +149,7 @@ fn get_super_blocks(
                 .map(|sb| (PathBuf::from(dev), sb))
         })
         .filter(|(_, sb)| sb.sb().uuid() == uuid)
-        .collect::<Vec<_>>())
+        .collect::<Vec<_>>()
 }
 
 fn get_all_block_devnodes() -> anyhow::Result<Vec<String>> {
@@ -189,14 +186,14 @@ fn get_devices_by_uuid(
         }
     };
 
-    get_super_blocks(uuid, &devices)
+    Ok(get_super_blocks(uuid, &devices))
 }
 
 #[allow(clippy::type_complexity)]
 fn get_uuid_for_dev_node(
     udev_bcachefs: &HashMap<String, Vec<String>>,
-    device: &std::path::PathBuf,
+    device: impl AsRef<Path>,
-) -> anyhow::Result<(Option<Uuid>, Option<(PathBuf, bch_sb_handle)>)> {
+) -> Result<(Option<Uuid>, Option<(PathBuf, bch_sb_handle)>)> {
     let canonical = fs::canonicalize(device)?;
 
     if !udev_bcachefs.is_empty() {
@@ -264,11 +261,11 @@ pub struct Cli {
 
 fn devs_str_sbs_from_uuid(
     udev_info: &HashMap<String, Vec<String>>,
-    uuid: String,
+    uuid: &str,
 ) -> anyhow::Result<(String, Vec<bch_sb_handle>)> {
     debug!("enumerating devices with UUID {}", uuid);
 
-    let devs_sbs = Uuid::parse_str(&uuid).map(|uuid| get_devices_by_uuid(udev_info, uuid))??;
+    let devs_sbs = Uuid::parse_str(uuid).map(|uuid| get_devices_by_uuid(udev_info, uuid))??;
 
     let devs_str = devs_sbs
         .iter()
@@ -283,7 +280,7 @@ fn devs_str_sbs_from_uuid(
 
 fn devs_str_sbs_from_device(
     udev_info: &HashMap<String, Vec<String>>,
-    device: &std::path::PathBuf,
+    device: impl AsRef<Path>,
 ) -> anyhow::Result<(String, Vec<bch_sb_handle>)> {
     let (uuid, sb_info) = get_uuid_for_dev_node(udev_info, device)?;
 
@@ -300,10 +297,10 @@ fn devs_str_sbs_from_device(
                 let dev = path.into_os_string().into_string().unwrap();
                 Ok((dev, vec![sb]))
             } else {
-                devs_str_sbs_from_uuid(udev_info, uuid.to_string())
+                devs_str_sbs_from_uuid(udev_info, &uuid.to_string())
             }
         }
-        (Some(uuid), None) => devs_str_sbs_from_uuid(udev_info, uuid.to_string()),
+        (Some(uuid), None) => devs_str_sbs_from_uuid(udev_info, &uuid.to_string()),
         _ => Ok((String::new(), Vec::new())),
     }
 }
@@ -312,11 +309,9 @@ fn cmd_mount_inner(opt: Cli) -> Result<()> {
     // Grab the udev information once
     let udev_info = udev_bcachefs_info()?;
 
-    let (devices, sbs) = if opt.dev.starts_with("UUID=") {
+    let (devices, sbs) = if let Some(uuid) = opt.dev.strip_prefix("UUID=") {
-        let uuid = opt.dev.replacen("UUID=", "", 1);
         devs_str_sbs_from_uuid(&udev_info, uuid)?
-    } else if opt.dev.starts_with("OLD_BLKID_UUID=") {
+    } else if let Some(uuid) = opt.dev.strip_prefix("OLD_BLKID_UUID=") {
-        let uuid = opt.dev.replacen("OLD_BLKID_UUID=", "", 1);
         devs_str_sbs_from_uuid(&udev_info, uuid)?
     } else {
         // If the device string contains ":" we will assume the user knows the entire list.
@@ -324,16 +319,15 @@ fn cmd_mount_inner(opt: Cli) -> Result<()> {
         // only 1 of a number of devices which are part of the FS. This appears to be the case
         // when we get called during fstab mount processing and the fstab specifies a UUID.
         if opt.dev.contains(':') {
-            let mut block_devices_to_mount = Vec::new();
+            let sbs = opt
+                .dev
+                .split(':')
+                .map(read_super_silent)
+                .collect::<Result<Vec<_>>>()?;
 
-            for dev in opt.dev.split(':') {
+            (opt.dev, sbs)
-                let dev = PathBuf::from(dev);
-                block_devices_to_mount.push(read_super_silent(&dev)?);
-            }
-
-            (opt.dev, block_devices_to_mount)
         } else {
-            devs_str_sbs_from_device(&udev_info, &PathBuf::from(opt.dev))?
+            devs_str_sbs_from_device(&udev_info, Path::new(&opt.dev))?
         }
     };
 

src/commands/subvolume.rs

@@ -37,9 +37,9 @@ enum Subcommands {
 }
 
 pub fn subvolume(argv: Vec<String>) -> i32 {
-    let args = Cli::parse_from(argv);
+    let cli = Cli::parse_from(argv);
 
-    match args.subcommands {
+    match cli.subcommands {
         Subcommands::Create { targets } => {
             for target in targets {
                 if let Some(dirname) = target.parent() {

src/key.rs

@@ -4,7 +4,7 @@ use std::{
     io::{stdin, IsTerminal},
     mem,
     path::Path,
-    thread,
+    ptr, thread,
     time::Duration,
 };
 
@@ -66,7 +66,7 @@ impl KeyHandle {
         let bch_key_magic = BCH_KEY_MAGIC.as_bytes().read_u64::<LittleEndian>().unwrap();
 
         let crypt = sb.sb().crypt().unwrap();
-        let crypt_ptr = crypt as *const _ as *mut _;
+        let crypt_ptr = ptr::addr_of!(*crypt).cast_mut();
 
         let mut output: bch_key =
             unsafe { bcachefs::derive_passphrase(crypt_ptr, passphrase.get().as_ptr()) };
@@ -75,9 +75,9 @@ impl KeyHandle {
 
         let ret = unsafe {
             bch2_chacha_encrypt_key(
-                &mut output as *mut _,
+                ptr::addr_of_mut!(output),
                 sb.sb().nonce(),
-                &mut key as *mut _ as *mut _,
+                ptr::addr_of_mut!(key).cast(),
                 mem::size_of_val(&key),
             )
         };
@@ -93,7 +93,7 @@ impl KeyHandle {
             keyutils::add_key(
                 key_type,
                 key_name,
-                &output as *const _ as *const _,
+                ptr::addr_of!(output).cast(),
                 mem::size_of_val(&output),
                 keyutils::KEY_SPEC_USER_KEYRING,
             )
@@ -103,7 +103,7 @@ impl KeyHandle {
             info!("Found key in keyring");
             Ok(KeyHandle {
                 _uuid: sb.sb().uuid(),
-                _id:   key_id as c_long,
+                _id:   c_long::from(key_id),
             })
         } else {
             Err(anyhow!("failed to add key to keyring: {}", errno::errno()))

src/wrappers/handle.rs

@@ -1,4 +1,4 @@
-use std::path::Path;
+use std::{path::Path, ptr};
 
 use bch_bindgen::c::{
     bcache_fs_close, bcache_fs_open, bch_ioctl_subvolume, bchfs_handle, BCH_IOCTL_SUBVOLUME_CREATE,
@@ -42,7 +42,7 @@ pub enum BcachefsIoctlPayload {
 impl From<&BcachefsIoctlPayload> for *const libc::c_void {
     fn from(value: &BcachefsIoctlPayload) -> Self {
         match value {
-            BcachefsIoctlPayload::Subvolume(p) => p as *const _ as *const libc::c_void,
+            BcachefsIoctlPayload::Subvolume(p) => ptr::addr_of!(p).cast(),
         }
     }
 }