Add functions to get original file name and loop device info #58
Conversation
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
|
@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 |
I hope the change is ok now. LMK if something else isn't fit. Some context on why I want to contribute. |
src/lib.rs
Outdated
| 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; |
There was a problem hiding this comment.
Why not put this use in with the other collected uses from bindings on line 40-43?
| }; | ||
|
|
||
| Self::attach_with_loop_info(self, backing_file, info) | ||
| Self::attach_with_loop_info(self, backing_file, loop_info64::default()) |
There was a problem hiding this comment.
Please take this change and put it in a separate PR. It's a code improvement, but not part of your enhancement. If in a separate PR, I can merge it immediately.
There was a problem hiding this comment.
I re-merged this PR's branch with the #60'th. So, once it's merged, this change will disappear.
|
@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. |
skorobogatydmitry
force-pushed
the
sd/ld-names
branch
from
af56c22 to
35f7b94
Compare
| 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
There was a problem hiding this comment.
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
| // 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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| 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 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
LO_NAME_SIZE comes from C header and limits number of bytes, AFAIU. So the comparison should be right.
|
@skorobogatydmitry Please rebase and force push when you get the chance. |
skorobogatydmitry
force-pushed
the
sd/ld-names
branch
from
35f7b94 to
37146b5
Compare
Done. |
|
Build failures seem infrastructure related, we can ignore. |
WalkthroughIntroduces a new loname module to encode backing file paths into loop_info64, updates attachment flow to prefill lo_file_name/lo_crypt_name, and adds LoopDevice::original_path and LoopDevice::info accessors. Integration tests validate presence/absence of stored path for attach vs attach_fd and successful info retrieval. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant U as Caller
participant LD as LoopDevice
participant LN as loname::Name
participant OS as Kernel (ioctl)
U->>LD: attach_with_loop_info(backing_file, mut info)
LD->>LN: Name::from_path(backing_file)
LN-->>LD: Name (fixed-size bytes)
LD->>LD: Write lo_file_name / lo_crypt_name into info
LD->>OS: LOOP_SET_FD (open/attach backing)
OS-->>LD: ok / err
LD->>OS: LOOP_SET_STATUS64(info with names)
OS-->>LD: ok / err
LD-->>U: Result
note over LD: Later<br/>original_path() reads<br/>lo_file_name from info()
sequenceDiagram
autonumber
participant U as Caller
participant LD as LoopDevice
participant OS as Kernel (ioctl)
U->>LD: info()
LD->>OS: LOOP_GET_STATUS64
OS-->>LD: loop_info64
LD-->>U: loop_info64
U->>LD: original_path()
LD->>LD: info() -> lo_file_name
alt has non-empty name
LD-->>U: Some(PathBuf)
else empty or absent
LD-->>U: None
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Congratulations! One of the builds has completed. 🍾 You can install the built RPMs by following these steps:
Please note that the RPMs should be used only in a testing environment. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
src/loname.rs (1)
61-66: Test iter over zeroed array can use all() for brevityOptional 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 endRelying 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 wellMirror 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 defaultingSilently 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
📒 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_namePer 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;
| /// 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)) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
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.
| /// 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.
| /// Obtain loop_info64 struct for the loop device | ||
| /// # Return | ||
| /// Ok(loop_info64) - successfully obtained info | ||
| /// Err(std::io::Error) - error from ioctl | ||
| pub fn info(&self) -> Result<loop_info64, std::io::Error> { | ||
| let empty_info = Box::new(loop_info64::default()); | ||
| let fd = self.device.as_raw_fd(); | ||
|
|
||
| unsafe { | ||
| 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)) | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
Fix memory leak and invalid fd usage in info()
- Leak: Box is never freed on ioctl error.
- fd.as_raw_fd() on a RawFd is invalid.
- Use a stack-allocated struct and pass a mutable pointer.
Apply this diff:
- pub fn info(&self) -> Result<loop_info64, std::io::Error> {
- let empty_info = Box::new(loop_info64::default());
- let fd = self.device.as_raw_fd();
-
- unsafe {
- 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))
- }
- }
+ pub fn info(&self) -> Result<loop_info64, std::io::Error> {
+ let mut info = loop_info64::default();
+ let ret = unsafe {
+ libc::ioctl(
+ self.device.as_raw_fd() as c_int,
+ LOOP_GET_STATUS64 as IoctlRequest,
+ &mut info,
+ )
+ };
+ ioctl_to_error(ret).map(|_| info)
+ }🤖 Prompt for AI Agents
In src/lib.rs around lines 421-435, the info() function currently leaks memory
by boxing the struct and never freeing it on ioctl error, and incorrectly calls
as_raw_fd() on a RawFd; replace the boxed allocation with a stack-allocated
mutable loop_info64 (let mut info = loop_info64::default()), call libc::ioctl
with the raw fd (use self.device.as_raw_fd() once, no extra as_raw_fd() on an
fd), pass &mut info as a *mut loop_info64, check ioctl_to_error(ret_code) and on
success return Ok(info), ensuring no heap allocation and no leak on error.
| let mut data: [u8; 64] = [0; LO_NAME_SIZE as usize]; | ||
| for (idx, byte) in s.iter().enumerate() { | ||
| data[idx] = *byte; | ||
| } | ||
| Ok(Self(data)) |
There was a problem hiding this comment.
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.
| 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.
| #[allow(clippy::to_string_trait_impl)] | ||
| impl ToString for Name { | ||
| fn to_string(&self) -> String { | ||
| self.0 | ||
| .iter() | ||
| .filter_map(|ch| { | ||
| let ch: char = *ch as char; | ||
| if ch == '\0' { | ||
| None | ||
| } else { | ||
| Some(ch) | ||
| } | ||
| }) | ||
| .collect::<String>() | ||
| } | ||
| } |
There was a problem hiding this comment.
String conversion is lossy for non‑UTF‑8 paths; switch to Display and expose raw bytes
Casting each u8 to char and collecting into a String changes the byte sequence for values >= 0x80, breaking round‑trips for non‑UTF‑8 paths. Implement Display for human output and expose a bytes view for exact reconstruction.
Apply this diff:
-#[allow(clippy::to_string_trait_impl)]
-impl ToString for Name {
- fn to_string(&self) -> String {
- self.0
- .iter()
- .filter_map(|ch| {
- let ch: char = *ch as char;
- if ch == '\0' {
- None
- } else {
- Some(ch)
- }
- })
- .collect::<String>()
- }
-}
+impl std::fmt::Display for Name {
+ fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+ let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len());
+ let s = String::from_utf8_lossy(&self.0[..end]);
+ f.write_str(&s)
+ }
+}And add a bytes accessor to enable lossless OS string construction:
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 {
return Err(format!(
"too many bytes in the provided loop dev source file path: {}, max {LO_NAME_SIZE}",
s.len()
));
}
let mut data = [0u8; LO_NAME_SIZE as usize];
data[..s.len()].copy_from_slice(s);
Ok(Self(data))
}
+
+ /// Bytes up to the first NUL terminator.
+ pub fn as_bytes(&self) -> &[u8] {
+ let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len());
+ &self.0[..end]
+ }
}📝 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.
| #[allow(clippy::to_string_trait_impl)] | |
| impl ToString for Name { | |
| fn to_string(&self) -> String { | |
| self.0 | |
| .iter() | |
| .filter_map(|ch| { | |
| let ch: char = *ch as char; | |
| if ch == '\0' { | |
| None | |
| } else { | |
| Some(ch) | |
| } | |
| }) | |
| .collect::<String>() | |
| } | |
| } | |
| // Replace the lossy `ToString` impl with a `Display` impl. | |
| impl std::fmt::Display for Name { | |
| fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | |
| let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len()); | |
| let s = String::from_utf8_lossy(&self.0[..end]); | |
| f.write_str(&s) | |
| } | |
| } | |
| // In the `Name` impl, add a lossless bytes accessor. | |
| 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 { | |
| return Err(format!( | |
| "too many bytes in the provided loop dev source file path: {}, max {LO_NAME_SIZE}", | |
| s.len() | |
| )); | |
| } | |
| let mut data = [0u8; LO_NAME_SIZE as usize]; | |
| data[..s.len()].copy_from_slice(s); | |
| Ok(Self(data)) | |
| } | |
| /// Bytes up to the first NUL terminator. | |
| pub fn as_bytes(&self) -> &[u8] { | |
| let end = self.0.iter().position(|&b| b == 0).unwrap_or(self.0.len()); | |
| &self.0[..end] | |
| } | |
| } |
🤖 Prompt for AI Agents
In src/loname.rs around lines 37 to 52, replace the current lossy ToString impl
that casts u8s to char with a Display impl that formats the Name for humans
using String::from_utf8_lossy(&self.0) (so printing is safe) and remove the
clippy allow and the ToString impl; additionally add a public bytes accessor,
e.g. pub fn as_bytes(&self) -> &[u8] { &self.0 }, so callers can reconstruct
exact, lossless OS strings from the raw bytes.
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
LoopConfiginstead of calling attach, etcTesting
All tests worked but
add_a_loop_device.I don't think it's related to the change.
Summary by CodeRabbit