Misc clean fix

src/args.rs

@@ -64,10 +64,39 @@ pub(crate) struct GenAddArgs {
     #[clap(long, short = 'z', default_value_t = false)]
     pub zero_copy: bool,
 
+    /// Used to resolve relative paths for backing files.
+    /// `RefCell` is used to allow deferred initialization of this field
+    /// from an immutable `GenAddArgs` reference, which is necessary
+    /// because the current directory is saved after argument parsing.
     #[clap(skip)]
     start_dir: RefCell<Option<PathBuf>>,
 }
 
+/// Rounds `x` up to the nearest multiple of `y`.
+/// `y` must be a power of two.
+pub fn round_up<T>(x: T, y: T) -> T
+where
+    T: Copy + Into<u64> + std::convert::TryFrom<u64>, // support u32, u64 and try conversion back
+{
+    let x: u64 = x.into();
+    let y: u64 = y.into();
+    assert!(y.is_power_of_two(), "y isn\'t power_of_2");
+    let result = (x + y - 1) & !(y - 1);
+
+    T::try_from(result).unwrap_or_else(|_| {
+        panic!("Overflow occurred during conversion from u64 to the original type");
+    })
+}
+
+/// Checks if `input` is a multiple of `base`, and that the result of
+/// `input / base` is a power of two.
+/// `base` must be a power of two.
+pub fn is_power2_of(input: u64, base: u64) -> bool {
+    assert!(base.is_power_of_two());
+
+    input % base == 0 && (input / base).is_power_of_two()
+}
+
 impl GenAddArgs {
     pub fn apply_block_size(&self, dev: &mut UblkDev) {
         if let Some(bs) = self.logical_block_size {
@@ -85,33 +114,7 @@ impl GenAddArgs {
             dev.tgt.params.basic.attrs &= !libublk::sys::UBLK_ATTR_READ_ONLY;
         }
     }
-}
 
-use std::convert::TryFrom;
-
-pub fn round_up<T>(x: T, y: T) -> T
-where
-    T: Copy + Into<u64> + TryFrom<u64>, // support u32, u64 and try conversion back
-{
-    let x: u64 = x.into();
-    let y: u64 = y.into();
-    assert!(y & (y - 1) == 0, "y isn't power_of_2");
-    let result = (x + y - 1) & !(y - 1);
-
-    T::try_from(result).unwrap_or_else(|_| {
-        panic!("Overflow occurred during conversion from u64 to the original type");
-    })
-}
-
-pub fn is_power2_of(input: u64, base: u64) -> bool {
-    assert!((base & (base - 1)) == 0);
-
-    let quotient = input / base;
-    quotient > 0 && (quotient & (quotient - 1)) == 0
-}
-
-impl GenAddArgs {
-    /// Return shared memory os id
     pub fn get_start_dir(&self) -> Option<PathBuf> {
         let dir = self.start_dir.borrow();
 
@@ -142,11 +145,10 @@ impl GenAddArgs {
         name: &'static str,
         dev_flags: UblkFlags,
     ) -> anyhow::Result<UblkCtrl> {
-        let mut ctrl_flags = if self.user_recovery {
-            libublk::sys::UBLK_F_USER_RECOVERY
-        } else {
-            0
-        };
+        let mut ctrl_flags = 0;
+        if self.user_recovery {
+            ctrl_flags |= libublk::sys::UBLK_F_USER_RECOVERY;
+        }
 
         if name == "zoned" {
             ctrl_flags |= libublk::sys::UBLK_F_USER_COPY | libublk::sys::UBLK_F_ZONED;
@@ -174,7 +176,7 @@ impl GenAddArgs {
         }
 
         match self.logical_block_size {
-            None | Some(512) | Some(1024) | Some(2048) | Some(4096) => {}
+            None | Some(512) | Some(1024) | Some(2048) | Some(4096) => {} // No-op
             _ => {
                 anyhow::bail!("invalid logical block size");
             }
@@ -199,15 +201,14 @@ impl GenAddArgs {
 
         Ok(libublk::ctrl::UblkCtrlBuilder::default()
             .name(name)
-            .depth(self.depth.try_into().unwrap())
-            .nr_queues(self.queue.try_into().unwrap())
+            .depth(self.depth.try_into()?)
+            .nr_queues(self.queue.try_into()?)
             .id(self.number)
             .ctrl_flags(ctrl_flags.into())
             .ctrl_target_flags(gen_flags)
             .dev_flags(dev_flags)
             .io_buf_bytes(buf_size as u32)
-            .build()
-            .unwrap())
+            .build()?)
     }
 }
 

src/loop.rs

@@ -1,3 +1,4 @@
+use anyhow::Context;
 use io_uring::{opcode, squeue, types};
 use libublk::io::{UblkDev, UblkIOCtx, UblkQueue};
 use libublk::uring_async::ublk_wait_and_handle_ios;
@@ -50,7 +51,6 @@ impl LoopTgt {
     }
 }
 
-#[inline]
 fn __lo_prep_submit_io_cmd(iod: &libublk::sys::ublksrv_io_desc) -> i32 {
     let op = iod.op_flags & 0xff;
 
@@ -90,7 +90,6 @@ fn lo_make_discard_sqe(op: u32, flags: u32, off: u64, bytes: u32) -> io_uring::s
         .flags(squeue::Flags::FIXED_FILE)
 }
 
-#[inline]
 fn __lo_make_io_sqe_zc(
     iod: &libublk::sys::ublksrv_io_desc,
     buf_index: u16,
@@ -123,7 +122,6 @@ fn __lo_make_io_sqe_zc(
     }
 }
 
-#[inline]
 fn __lo_make_io_sqe(
     iod: &libublk::sys::ublksrv_io_desc,
     buf_addr: *mut u8,
@@ -147,7 +145,6 @@ fn __lo_make_io_sqe(
     }
 }
 
-#[inline]
 async fn lo_handle_io_cmd_async(q: &UblkQueue<'_>, tag: u16, buf_addr: *mut u8, zc: bool) -> i32 {
     let iod = q.get_iod(tag);
     let res = __lo_prep_submit_io_cmd(iod);
@@ -335,43 +332,38 @@ pub(crate) fn ublk_add_loop(
     opt: Option<LoopArgs>,
     comm_rc: &Arc<crate::DevIdComm>,
 ) -> anyhow::Result<i32> {
-    let (file, dio, ro, aa, no_discard) = match opt {
-        Some(ref o) => (
+    let (file, dio, ro, aa, no_discard) = if let Some(ref o) = opt {
+        (
             o.gen_arg.build_abs_path(o.file.clone()),
             !o.buffered_io,
             o.gen_arg.read_only,
             o.async_await,
             o.no_discard,
-        ),
-        None => {
-            let mut p: libublk::sys::ublk_params = Default::default();
-            ctrl.get_params(&mut p)?;
-
-            match ctrl.get_target_data_from_json() {
-                Some(val) => {
-                    let lo = &val["loop"];
-                    let tgt_data: Result<LoJson, _> = serde_json::from_value(lo.clone());
-
-                    match tgt_data {
-                        Ok(t) => (
-                            PathBuf::from(t.back_file_path.as_str()),
-                            t.direct_io != 0,
-                            (p.basic.attrs & libublk::sys::UBLK_ATTR_READ_ONLY) != 0,
-                            t.async_await,
-                            t.no_discard,
-                        ),
-                        Err(_) => return Err(anyhow::anyhow!("wrong json data")),
-                    }
-                }
-                None => return Err(anyhow::anyhow!("not get json data")),
-            }
-        }
+        )
+    } else {
+        let mut p: libublk::sys::ublk_params = Default::default();
+        ctrl.get_params(&mut p)?;
+
+        let val = ctrl
+            .get_target_data_from_json()
+            .ok_or_else(|| anyhow::anyhow!("not get json data"))?;
+        let lo = &val["loop"];
+        let tgt_data: LoJson =
+            serde_json::from_value(lo.clone()).map_err(|e| anyhow::anyhow!(e))?;
+
+        (
+            PathBuf::from(tgt_data.back_file_path.as_str()),
+            tgt_data.direct_io != 0,
+            (p.basic.attrs & libublk::sys::UBLK_ATTR_READ_ONLY) != 0,
+            tgt_data.async_await,
+            tgt_data.no_discard,
+        )
     };
 
     let file_path = format!("{}", file.as_path().display());
     let json = LoJson {
         back_file_path: file_path,
-        direct_io: i32::from(dio),
+        direct_io: dio.into(),
         async_await: aa,
         no_discard,
     };
@@ -381,7 +373,7 @@ pub(crate) fn ublk_add_loop(
             .read(true)
             .write(!ro)
             .open(&file)
-            .unwrap(),
+            .with_context(|| format!("failed to open backing file {:?}", file))?,
     );
 
     if (ctrl.dev_info().flags & (libublk::sys::UBLK_F_USER_COPY as u64)) != 0 {

src/main.rs

@@ -205,8 +205,8 @@ fn ublk_add(opt: args::AddCommands) -> anyhow::Result<i32> {
         ublk_add_worker(opt, &comm)
     } else {
         let daemonize = daemonize::Daemonize::new()
-            .stdout(daemonize::Stdio::keep())
-            .stderr(daemonize::Stdio::keep());
+            .stdout(daemonize::Stdio::devnull())
+            .stderr(daemonize::Stdio::devnull());
 
         match daemonize.execute() {
             daemonize::Outcome::Child(Ok(_)) => match ublk_add_worker(opt, &comm) {
@@ -263,8 +263,8 @@ fn ublk_recover_work(opt: args::UblkArgs) -> anyhow::Result<i32> {
 
 fn ublk_recover(opt: args::UblkArgs) -> anyhow::Result<i32> {
     let daemonize = daemonize::Daemonize::new()
-        .stdout(daemonize::Stdio::keep())
-        .stderr(daemonize::Stdio::keep());
+        .stdout(daemonize::Stdio::devnull())
+        .stderr(daemonize::Stdio::devnull());
 
     let id = opt.number;
     if id < 0 {

src/null.rs

@@ -21,8 +21,14 @@ pub(crate) struct NullAddArgs {
 #[inline]
 fn get_io_cmd_result(q: &UblkQueue, tag: u16) -> i32 {
     let iod = q.get_iod(tag);
+    let op = iod.op_flags & 0xff;
 
-    (iod.nr_sectors << 9) as i32
+    match op {
+        libublk::sys::UBLK_IO_OP_READ | libublk::sys::UBLK_IO_OP_WRITE => {
+            (iod.nr_sectors << 9) as i32
+        }
+        _ => 0,
+    }
 }
 
 #[inline]
@@ -104,19 +110,24 @@ pub(crate) fn ublk_add_null(
 
     let tgt_init = |dev: &mut UblkDev| {
         dev.set_default_params(size);
+        let p = &mut dev.tgt.params;
+
+        p.types |= libublk::sys::UBLK_PARAM_TYPE_DISCARD;
+        p.discard.max_discard_sectors = 2 << 30 >> 9;
+        p.discard.discard_granularity = 1 << p.basic.physical_bs_shift;
+        p.discard.max_discard_segments = 1;
+
         if let Some(ref o) = opt {
             o.gen_arg.apply_block_size(dev);
             o.gen_arg.apply_read_only(dev);
         }
         Ok(())
     };
 
-    let aa = {
-        if let Some(ref o) = opt {
-            o.async_await
-        } else {
-            false
-        }
+    let aa = if let Some(ref o) = opt {
+        o.async_await
+    } else {
+        false
     };
 
     let q_handler = move |qid, dev: &_| {

tests/basic.rs

@@ -29,6 +29,15 @@ mod integration {
         }
     }
 
+    fn has_blkdiscard() -> bool {
+        Command::new("blkdiscard")
+            .arg("--version")
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .status()
+            .is_ok()
+    }
+
     fn support_zoned() -> bool {
         match UblkCtrl::get_features() {
             Some(f) => {
@@ -672,13 +681,7 @@ mod integration {
             return;
         }
 
-        let has_blkdiscard = Command::new("blkdiscard")
-            .arg("--version")
-            .stdout(Stdio::null())
-            .stderr(Stdio::null())
-            .status()
-            .is_ok();
-        if !has_blkdiscard {
+        if !has_blkdiscard() {
             return;
         }
 
@@ -747,4 +750,56 @@ mod integration {
             assert!(cmp_status.success());
         });
     }
+
+    #[test]
+    fn test_ublk_null_discard() {
+        if !support_ublk() {
+            return;
+        }
+
+        if !has_blkdiscard() {
+            return;
+        }
+
+        let ctrl = run_rublk_add_dev(["add", "null"].to_vec());
+        let dev_path = ctrl.get_bdev_path();
+
+        let res = Command::new("blkdiscard")
+            .args([&dev_path])
+            .stdout(Stdio::null())
+            .stderr(Stdio::null())
+            .status()
+            .expect("blkdiscard failed");
+        assert!(res.success());
+
+        run_rublk_del_dev(ctrl, false);
+    }
+
+    #[test]
+    fn test_rublk_add_no_hang() {
+        if !support_ublk() {
+            return;
+        }
+
+        let tgt_dir = get_curr_bin_dir().unwrap();
+        let rublk_path = tgt_dir.join("rublk");
+
+        let output = Command::new(rublk_path)
+            .args(["add", "null"])
+            .output()
+            .expect("Failed to execute rublk add null");
+
+        assert!(output.status.success());
+
+        let stdout = String::from_utf8_lossy(&output.stdout);
+        let id_regx = regex::Regex::new(r"dev id (\d+)").unwrap();
+        let id: i32 = id_regx
+            .captures(&stdout)
+            .and_then(|c| c.get(1))
+            .and_then(|m| m.as_str().parse().ok())
+            .expect("Failed to parse device ID");
+
+        let ctrl = UblkCtrl::new_simple(id).unwrap();
+        run_rublk_del_dev(ctrl, false);
+    }
 }