Add functions to get original file name and loop device info by skorobogatydmitry · Pull Request #58 · stratis-storage/loopdev-3

skorobogatydmitry · 2025-07-06T18:40:59Z

It could be useful to be able to get original file name and info of a loop device in case the original LoopDevice isn't available for any reason.

I have some more changes in mind like listing devices, applying LoopConfig instead of calling attach, etc

Testing

All tests worked but add_a_loop_device.
I don't think it's related to the change.

Summary by CodeRabbit

New Features
- Loop devices now expose the original backing file path when attached using a file path.
- Added a method to retrieve current loop device status/info.
- Note: When attached via file descriptor, the original path is not available.
Tests
- Added integration tests validating original path reporting (present/absent) and successful info retrieval across attachment methods.

packit-as-a-service · 2025-07-06T18:45:01Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo yum install -y dnf-plugins-core on RHEL 8
sudo dnf install -y dnf-plugins-core on Fedora
dnf copr enable packit/stratis-storage-loopdev-3-58
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

mulkieran · 2025-07-07T13:00:50Z

@skorobogatydmitry Thanks for PR. It looks like you hit some lints in your submitted code that you should fix. You should be able to force push. We have a Makefile target clippy for lints and also a fmt target for formatting. Best to run both of those.

skorobogatydmitry · 2025-07-08T18:47:38Z

@skorobogatydmitry Thanks for PR. It looks like you hit some lints in your submitted code that you should fix. You should be able to force push. We have a Makefile target clippy for lints and also a fmt target for formatting. Best to run both of those.

I hope the change is ok now. LMK if something else isn't fit.

Some context on why I want to contribute.
That's the most active fork of the original crate. I need this functionality in my pet-project and already have some code locally in it, but really like to submit it to an upstream.

mulkieran

Two preliminary requests...please put the requested code up in a separate PR and I'll merge that immediately, then resume reviewing this PR.

mulkieran · 2025-07-10T20:19:43Z

src/lib.rs

    loop_info64, LOOP_CLR_FD, LOOP_CTL_ADD, LOOP_CTL_GET_FREE, LOOP_SET_CAPACITY, LOOP_SET_FD,
    LOOP_SET_STATUS64, LO_FLAGS_AUTOCLEAR, LO_FLAGS_PARTSCAN, LO_FLAGS_READ_ONLY,
 };
+use bindings::LOOP_GET_STATUS64;


Why not put this use in with the other collected uses from bindings on line 40-43?

src/lib.rs

mulkieran · 2025-07-10T20:46:45Z

@skorobogatydmitry This crate is the official successor of the original loopdev crate (https://rustsec.org/advisories/RUSTSEC-2023-0088.html). We took it over because it was no longer compiling and the original author was unresponsive. However, since we are not the original author, review will take a little bit longer than with a code base we are familiar with. You will have to bear with us.

mulkieran · 2025-07-15T18:12:59Z

src/lib.rs

        let write_access = (info.lo_flags & LO_FLAGS_READ_ONLY) == 0;
+
+        // store backing file name in the info
+        let name = loname::Name::from_path(&backing_file).unwrap_or_default();


I understand the purpose of this but I'm unsure why you need the special Name struct for marshalling and unmarshalling. I feel that it would be better to use existing Path and PathBuf methods, as_os_str() or something. Can you tell me why the concern w/ null terminators in the to_string method? Thanks.

My concern was compatibility with C code.
I found that kernel actually takes care of the terminator here.

I did a brief check that name setup through as_os_str is correctly displayed by losetup -l, but that's not a reliable source, because ...

losetup actually leverages cat /sys/block/loop0/loop/backing_file on my system. It isn't limited at 64 chars and a way more straight-forward path.

I think I need to get rid of lo_file_name logic but keep info around for future use.

Do you think it's reasonable ?

Getting the full name from /sys/block/loop0/loop/backing_file isn't that simple, as it requires to know the loop0 part, which, in its turn, requires an info call. The only real advantage is that it allows to get the full name in all cases.
LMK if you want to see the version with as_os_str anyways

Personally I would be for removing some of the more complicated handling that @mulkieran mentioned. I can see something like a length check potentially being necessary, but I believe that if we're doing FFI, it's better to use standard ways of null-encoding the string. Custom logic with null handling has caused me a lot of pain in the past.

I see the point of removing the custom logic for handling \0, let me find a way to make it with OsString.
However, I would rather keep the naming helper separated from the rest of the code so it doesn't pollute the lib.rs, which is nearing the limit of a good code unit.

I ended-up with casting &[u8] -> CStr (because it's actually a CStr) -> OsStr (because it's needed to make a PathBuf) -> PathBuf.
So, there's no Tostring anymore, although the code is even more complicated.

mulkieran

Just one semantic question so far, still working...

mulkieran · 2025-07-16T15:58:50Z

src/lib.rs

+        // store backing file name in the info
+        let name = loname::Name::from_path(&backing_file).unwrap_or_default();
+        info.lo_file_name = name.0;
+        info.lo_crypt_name = name.0;


Would you consider only setting lo_file_name and not lo_crypt_name here? If not, are you using io_crypt_name? I've made some preliminary inquiries and it seems not entirely clear what it is for.

Would you consider only setting lo_file_name and not lo_crypt_name here?
Sure, will do.

I don't use lo_crypt_name, but I found that losetup sets both and they're semantically similar.

I'm personally okay with mimicking losetup if you are @mulkieran

mulkieran · 2025-07-21T21:19:50Z

src/loname.rs

+impl Name {
+    pub fn from_path<Y: AsRef<Path>>(path: Y) -> Result<Self, String> {
+        let s = path.as_ref().as_os_str().as_encoded_bytes();
+        if s.len() > LO_NAME_SIZE as usize {


I'm concerned that this use of .len() is not correct, because it means the bytes the implementation has allocated, not the actual length of the characters required to encode the path. I'm going to ask another person to look at this.

LO_NAME_SIZE comes from C header and limits number of bytes, AFAIU. So the comparison should be right.

Generally, I believe that .len() is correct. You may be thinking of .capacity().

mulkieran · 2025-07-28T20:53:18Z

@skorobogatydmitry Please rebase and force push when you get the chance.

skorobogatydmitry · 2025-08-04T08:26:41Z

@skorobogatydmitry Please rebase and force push when you get the chance.

Done.
It's not a good practice, IMO

mulkieran · 2025-08-26T02:24:51Z

Build failures seem infrastructure related, we can ignore.

coderabbitai

Actionable comments posted: 4

🧹 Nitpick comments (4)

src/loname.rs (1)
61-66: Test iter over zeroed array can use all() for brevity

Optional tidy: reduces noise and improves readability.

Apply this diff:
-        let name = Name::default();
-        for num in name.0 {
-            assert_eq!(0, num);
-        }
+        let name = Name::default();
+        assert!(name.0.iter().all(|&b| b == 0));
tests/integration_test.rs (2)
225-249: Deterministically detach the loop device at test end

Relying on the next test’s setup() to detach can hide ordering issues. Detach here to keep tests self-contained.

Apply this diff:
     assert!(ld0.info().is_ok());
+    ld0.detach()
+        .expect("should detach the backing file at the end of the test");
 }
251-271: Detach in the fd-based test as well

Mirror the previous test to avoid leaving state behind.

Apply this diff:
     assert!(ld0.original_path().is_none());
+    ld0.detach()
+        .expect("should detach the backing file at the end of the test");
 }
src/lib.rs (1)

251-259: Consider surfacing Name::from_path error instead of silently defaulting

Silently zeroing lo_file_name on overflow hides input errors and surprises callers.

Option: return io::Error::new(InvalidInput, "...") from attach_with_loop_info when Name::from_path fails, or at least log at trace/debug.

📜 Review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 859c9a0 and 4729bdf.

📒 Files selected for processing (3)

src/lib.rs (5 hunks)
src/loname.rs (1 hunks)
tests/integration_test.rs (2 hunks)

🧰 Additional context used

🧬 Code graph analysis (2)

tests/integration_test.rs (2)

src/lib.rs (5)

path (320-324)

open (93-100)

open (195-200)

original_path (332-341)

info (425-434)

tests/util/mod.rs (2)

setup (57-61)

create_backing_file (19-27)

src/lib.rs (1)

src/loname.rs (2)

from_path (21-34)

default (14-16)

🔇 Additional comments (1)

src/lib.rs (1)

255-259: Only set lo_file_name; drop lo_crypt_name

Per earlier review, set lo_file_name only. lo_crypt_name’s semantics are unclear and not needed here.

Apply this diff:

-        // store backing file name in the info
-        let name = loname::Name::from_path(&backing_file).unwrap_or_default();
-        info.lo_file_name = name.0;
-        info.lo_crypt_name = name.0;
+        // store backing file name in the info
+        let name = loname::Name::from_path(&backing_file).unwrap_or_default();
+        info.lo_file_name = name.0;

coderabbitai · 2025-09-25T08:21:13Z

src/lib.rs

+    /// Try to obtain the original file path used on mapping
+    /// The method ignores ioctl errors
+    ///
+    /// # Return
+    /// The path expected to be stored in the loop_info64.
+    /// If it's not there, method returns None, otherwise - the string stored
+    pub fn original_path(&self) -> Option<PathBuf> {
+        self.info().ok().and_then(|info| {
+            let name = Name(info.lo_file_name).to_string();
+            if name.is_empty() {
+                None
+            } else {
+                Some(PathBuf::from(name))
+            }
+        })
+    }


⚠️ Potential issue | 🟠 Major

Lossless original_path: build PathBuf from raw bytes, not a UTF‑8 String

Current code can mangle non‑UTF‑8 paths. Use the bytes up to the first NUL and construct an OsString via OsStringExt.

Apply this diff:

- pub fn original_path(&self) -> Option<PathBuf> { - self.info().ok().and_then(|info| { - let name = Name(info.lo_file_name).to_string(); - if name.is_empty() { - None - } else { - Some(PathBuf::from(name)) - } - }) - } + pub fn original_path(&self) -> Option<PathBuf> { + self.info().ok().and_then(|info| { + let raw = &info.lo_file_name; + let end = raw.iter().position(|&b| b == 0).unwrap_or(raw.len()); + if end == 0 { + None + } else { + Some(PathBuf::from( + std::os::unix::ffi::OsStringExt::from_vec(raw[..end].to_vec()), + )) + } + }) + }

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

/// Try to obtain the original file path used on mapping

/// The method ignores ioctl errors

///

/// # Return

/// The path expected to be stored in the loop_info64.

/// If it's not there, method returns None, otherwise - the string stored

pub fn original_path(&self) -> Option<PathBuf> {

self.info().ok().and_then(|info| {

let name = Name(info.lo_file_name).to_string();

if name.is_empty() {

None

} else {

Some(PathBuf::from(name))

}

})

}

pub fn original_path(&self) -> Option<PathBuf> {

self.info().ok().and_then(|info| {

let raw = &info.lo_file_name;

let end = raw.iter().position(|&b| b == 0).unwrap_or(raw.len());

if end == 0 {

None

} else {

Some(PathBuf::from(

std::os::unix::ffi::OsStringExt::from_vec(raw[..end].to_vec()),

))

}

})

}

🤖 Prompt for AI Agents

src/lib.rs around lines 326 to 341: current original_path converts the C-style file name to a UTF-8 String which can mangle non‑UTF‑8 file names; instead, extract the raw byte array from info.lo_file_name up to the first NUL byte, convert that byte slice into an OsString (using OsStringExt::from_vec on Unix), then build and return a PathBuf from that OsString (return None if the byte slice is empty). Ensure you avoid any UTF-8 String conversion so paths are preserved losslessly.

src/lib.rs

coderabbitai · 2025-09-25T08:21:14Z

src/loname.rs

+        let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize];
+        for (idx, byte) in s.iter().enumerate() {
+            data[idx] = *byte;
+        }
+        Ok(Self(data))


⚠️ Potential issue | 🟠 Major

Avoid hard-coding 64; use LO_NAME_SIZE and copy_from_slice

Hard-coding the array length risks mismatch if LO_NAME_SIZE changes. Also, use copy_from_slice for clarity and bounds safety (you already guard len).

Apply this diff:

- let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize]; - for (idx, byte) in s.iter().enumerate() { - data[idx] = *byte; - } + let mut data = [0u8; LO_NAME_SIZE as usize]; + data[..s.len()].copy_from_slice(s);

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change

let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize];

for (idx, byte) in s.iter().enumerate() {

data[idx] = *byte;

}

Ok(Self(data))

let mut data = [0u8; LO_NAME_SIZE as usize];

data[..s.len()].copy_from_slice(s);

Ok(Self(data))

🤖 Prompt for AI Agents

In src/loname.rs around lines 29 to 33, the code hard-codes the array length as 64 and manually copies bytes; change the array initialization to use LO_NAME_SIZE (e.g. let mut data: [u8; LO_NAME_SIZE as usize] = [0; LO_NAME_SIZE as usize];) and replace the for-loop with a bounds-safe copy_from_slice call (e.g. data[..s.len()].copy_from_slice(s)); keep the existing length guard and ensure any casts to usize use LO_NAME_SIZE as usize.

src/loname.rs

jbaublitz

@mulkieran I've provided my review. @skorobogatydmitry I apologize that it's taken me so long to get around to this. Are you interested in resuming work on this?

jbaublitz · 2026-02-16T23:56:30Z

src/loname.rs

+impl Name {
+    pub fn from_path<Y: AsRef<Path>>(path: Y) -> Result<Self, String> {
+        let s = path.as_ref().as_os_str().as_encoded_bytes();
+        if s.len() > LO_NAME_SIZE as usize {


Generally, I believe that .len() is correct. You may be thinking of .capacity().

jbaublitz · 2026-02-16T23:57:52Z

src/lib.rs

+            let ptr = Box::into_raw(empty_info);
+            let ret_code = libc::ioctl(fd.as_raw_fd(), LOOP_GET_STATUS64 as IoctlRequest, ptr);
+            ioctl_to_error(ret_code).map(|_| *Box::from_raw(ptr))


Is there any reason we're not just using .as_ptr() or .as_mut_ptr() here?

No particular reason, I just used what I knew. Will change.
I also this thank using as_mut_ptr() eliminates one of the leakages pointed out in the AI review.

It's in nightly: https://doc.rust-lang.org/std/boxed/struct.Box.html#method.as_mut_ptr
I assume it's not ok to use, as the crate has rust 1.74.0 pinned.

Okay, you're right. I believe it would still be better to use something like .as_ref() or .as_mut() as needed and then cast it using as to a pointer. I'm also a bit confused as to why we need the box at all. I'm not seeing a clear reason why we would need a heap allocation when at the end of the method, we copy the data out of the Box and pass it back by value as the return type. If we passed back the box I could maybe see why we might do that but as it stands, I think it would work just as well with a stack allocation.

Good point, actually. Let me get rid of Box-es.

jbaublitz · 2026-02-16T23:58:52Z

src/lib.rs

+        // store backing file name in the info
+        let name = loname::Name::from_path(&backing_file).unwrap_or_default();
+        info.lo_file_name = name.0;
+        info.lo_crypt_name = name.0;


I'm personally okay with mimicking losetup if you are @mulkieran

jbaublitz · 2026-02-17T00:02:21Z

src/lib.rs

        let write_access = (info.lo_flags & LO_FLAGS_READ_ONLY) == 0;
+
+        // store backing file name in the info
+        let name = loname::Name::from_path(&backing_file).unwrap_or_default();


Personally I would be for removing some of the more complicated handling that @mulkieran mentioned. I can see something like a length check potentially being necessary, but I believe that if we're doing FFI, it's better to use standard ways of null-encoding the string. Custom logic with null handling has caused me a lot of pain in the past.

skorobogatydmitry · 2026-02-18T13:57:55Z

I'm playing around with the way to produce a PathBuf from [u8;64]. Will change PR status after pushing the changes.
Thank you for looking at the code!

packit-as-a-service · 2026-02-20T11:45:06Z

Congratulations! One of the builds has completed. 🍾

You can install the built RPMs by following these steps:

sudo dnf install -y 'dnf*-command(copr)'
dnf copr enable packit/stratis-storage-loopdev-3-58-copr_pull
And now you can install the packages.

Please note that the RPMs should be used only in a testing environment.

- cleanup code

- state possible errors on conversion, but ignore them

skorobogatydmitry · 2026-02-20T12:05:48Z

src/loname.rs

+
+impl TryInto<PathBuf> for Name {
+    type Error = NameToPathBufError;
+    fn try_into(self) -> Result<PathBuf, Self::Error> {


I'm exposing possible errors here, but the outer scope supresses it, as I think it's a meaningful action on that level.

skorobogatydmitry · 2026-02-20T12:06:41Z

src/lib.rs

+    /// # Return
+    /// None - path field is empty, not a valid path or ioctl error was encountered
+    /// Some(PathBuf) - otherwise
+    pub fn original_path(&self) -> Option<PathBuf> {


I think that this has to be replaced by this:

self.info().ok().and_then(|info| { self.path() .and_then(|dev_loop_path| { dev_loop_path.file_name().and_then(|dev_name| { fs::read_to_string( PathBuf::from("/sys/block") .join(dev_name) .join("loop/backing_file"), ) .ok() }) }) .map(|string| PathBuf::from(string)) .or(Name(info.lo_file_name).try_into().ok()) })

but would like to see a thumb-up on this version

mulkieran assigned skorobogatydmitry Jul 7, 2025

mulkieran added this to 2025June Jul 7, 2025

mulkieran self-requested a review July 7, 2025 13:00

mulkieran moved this to In Progress in 2025June Jul 7, 2025

mulkieran added this to 2025July Jul 8, 2025

mulkieran removed this from 2025June Jul 8, 2025

mulkieran moved this to In Progress in 2025July Jul 8, 2025

mulkieran requested changes Jul 10, 2025

View reviewed changes

skorobogatydmitry mentioned this pull request Jul 11, 2025

Remove unused local variable: make construction inline #60

Merged

skorobogatydmitry force-pushed the sd/ld-names branch from af56c22 to 35f7b94 Compare July 11, 2025 06:53

skorobogatydmitry requested a review from mulkieran July 11, 2025 06:55

mulkieran requested changes Jul 15, 2025

View reviewed changes

mulkieran requested changes Jul 16, 2025

View reviewed changes

mulkieran reviewed Jul 21, 2025

View reviewed changes

decay added 4 commits August 4, 2025 11:25

add functions to get original file name and loop device info

24f3117

fix clippy problems

f500303

move import to the existing block

0048775

remove redundant change

37146b5

skorobogatydmitry force-pushed the sd/ld-names branch from 35f7b94 to 37146b5 Compare August 4, 2025 08:26

mulkieran removed this from 2025July Aug 8, 2025

mulkieran added this to 2025August Aug 8, 2025

mulkieran moved this to In Review in 2025August Aug 8, 2025

mulkieran added this to the 0.5.3 milestone Aug 13, 2025

coderabbitai bot reviewed Sep 25, 2025

View reviewed changes

mulkieran removed this from 2025September Oct 11, 2025

mulkieran added this to 2025October Oct 11, 2025

mulkieran moved this to In Review in 2025October Oct 11, 2025

mulkieran modified the milestones: 0.5.3, 0.5.4 Oct 31, 2025

mulkieran removed this from 2025October Nov 17, 2025

mulkieran added this to 2025November Nov 17, 2025

mulkieran moved this to In Review in 2025November Nov 17, 2025

mulkieran moved this from In Review to Pending in 2025November Dec 1, 2025

mulkieran moved this from Pending to In Progress (long term) in 2025November Dec 1, 2025

mulkieran added this to 2025December Dec 1, 2025

mulkieran removed this from 2025November Dec 1, 2025

mulkieran moved this to In Progress (long term) in 2025December Dec 1, 2025

mulkieran added this to 2026January Jan 2, 2026

mulkieran moved this to In Progress (long term) in 2026January Jan 2, 2026

mulkieran removed this from 2025December Jan 2, 2026

jbaublitz requested changes Feb 17, 2026

View reviewed changes

mulkieran removed this from 2026January Feb 17, 2026

skorobogatydmitry marked this pull request as draft February 18, 2026 13:56

Merge branch 'master' into sd/ld-names

0714ab2

skorobogatydmitry added 3 commits February 20, 2026 15:52

- get rid of heap allocation for loop_info64

7aeae6d

- cleanup code

- avoid custom conversion &[u8;64] -> PathBuf

38a1f6f

- state possible errors on conversion, but ignore them

remove the hardcoded name length

5e3aa8b

skorobogatydmitry commented Feb 20, 2026

View reviewed changes

skorobogatydmitry requested a review from jbaublitz February 20, 2026 12:06

skorobogatydmitry marked this pull request as ready for review February 20, 2026 12:07

Comments

Conversation

skorobogatydmitry commented Jul 6, 2025 • edited by coderabbitai bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Testing

Summary by CodeRabbit

Uh oh!

packit-as-a-service bot commented Jul 6, 2025

Uh oh!

mulkieran commented Jul 7, 2025

Uh oh!

skorobogatydmitry commented Jul 8, 2025

Uh oh!

mulkieran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mulkieran commented Jul 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skorobogatydmitry Feb 18, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mulkieran left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mulkieran commented Jul 28, 2025

Uh oh!

skorobogatydmitry commented Aug 4, 2025

Uh oh!

mulkieran commented Aug 26, 2025

Uh oh!

coderabbitai bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coderabbitai bot Sep 25, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jbaublitz left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skorobogatydmitry Feb 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skorobogatydmitry commented Jul 6, 2025 •

edited by coderabbitai bot

Loading

skorobogatydmitry Feb 18, 2026 •

edited

Loading

skorobogatydmitry Feb 17, 2026 •

edited

Loading