Pna/special files support#2537
Conversation
Add support for archiving block devices, character devices, and FIFOs (named pipes) to PNA archives, following the tar pattern of storing device metadata only. Library changes: - Extend DataKind enum with BlockDevice, CharDevice, Fifo variants - Add dNUM chunk type for storing device major/minor numbers - Add DeviceNumbers struct with serialization support - Add EntryBuilder::new_block_device(), new_char_device(), new_fifo() - Update NormalEntry to parse/write dNUM chunks CLI changes: - Detect device files using FileTypeExt on Unix - Create device entries with correct major/minor numbers - Display device files in list command (b/c/p type indicators) - Skip device extraction with warning (mknod support to be added later) 🤖 Generated with [Claude Code](https://claude.com/claude-code)
Add Unix device file extraction support using mknod/mkfifo: - Add mknod_block(), mknod_char(), mkfifo() helper functions - Extract block devices with mknod(S_IFBLK, dev) - Extract character devices with mknod(S_IFCHR, dev) - Extract FIFOs with mkfifo() - Preserve permissions from archive metadata - Log warnings for failures (e.g., missing root privileges) - Skip gracefully on non-Unix platforms
Test FIFO and device node (block/char) round-trip archiving: - fifo_roundtrip: creates FIFO, archives, extracts, verifies type - device_nodes_roundtrip: creates block/char devices, verifies major/minor Tests gracefully skip if mknod fails (containerized environments).
Refactor device file support to follow PNA's design pattern for reference-based entries. Device numbers are now stored in FDAT (like symlink targets) instead of a separate dNUM ancillary chunk. Changes: - Remove dNUM chunk type from ChunkType - EntryBuilder::new_block_device/new_char_device now return io::Result and write device numbers to FDAT using WriteOptions::store() - Remove device_numbers field from NormalEntry struct - Replace device_numbers() with read_device_numbers() -> io::Result<Option<DeviceNumbers>> - Update CLI to use new API This aligns device file handling with the symlink/hardlink pattern where reference data is stored in FDAT rather than ancillary chunks.
The read_device_numbers() method now accepts a ReadOption parameter so it can pass the password when reading encrypted device entries.
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Comment |
Summary of ChangesHello @ChanTsune, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly expands the utility's capabilities by enabling the proper handling of Unix special file types. It allows users to archive and extract block devices, character devices, and named pipes, preserving their essential metadata and functionality. This enhancement is crucial for maintaining file system integrity and completeness when creating backups or transferring system-level data on Unix-based platforms. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for special file types on Unix systems, including block devices, character devices, and FIFOs. The changes are comprehensive, touching the CLI commands for creation, extraction, listing, and diffing, as well as the core library to handle these new entry types. New tests have been added to verify the functionality.
My review focuses on improving maintainability by reducing code duplication and enhancing error handling. I've identified a few areas where the code can be refactored for better clarity and robustness.
| DataKind::BlockDevice => { | ||
| #[cfg(unix)] | ||
| { | ||
| let Some(dev) = item.read_device_numbers(ReadOptions::with_password(password))? | ||
| else { | ||
| log::warn!( | ||
| "Skipping block device without device numbers: {}", | ||
| path.display() | ||
| ); | ||
| return Ok(()); | ||
| }; | ||
| let mode = item | ||
| .metadata() | ||
| .permission() | ||
| .map(|p| p.permissions() as u32) | ||
| .unwrap_or(0o666); | ||
| if remove_existing { | ||
| utils::io::ignore_not_found(utils::fs::remove_path(&path))?; | ||
| } | ||
| if let Err(e) = utils::fs::mknod_block(&path, dev.major(), dev.minor(), mode) { | ||
| log::warn!( | ||
| "Failed to create block device {} (major={}, minor={}): {} (may require root)", | ||
| path.display(), | ||
| dev.major(), | ||
| dev.minor(), | ||
| e | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| log::warn!( | ||
| "Skipping block device (not supported on this platform): {}", | ||
| path.display() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| DataKind::CharDevice => { | ||
| #[cfg(unix)] | ||
| { | ||
| let Some(dev) = item.read_device_numbers(ReadOptions::with_password(password))? | ||
| else { | ||
| log::warn!( | ||
| "Skipping character device without device numbers: {}", | ||
| path.display() | ||
| ); | ||
| return Ok(()); | ||
| }; | ||
| let mode = item | ||
| .metadata() | ||
| .permission() | ||
| .map(|p| p.permissions() as u32) | ||
| .unwrap_or(0o666); | ||
| if remove_existing { | ||
| utils::io::ignore_not_found(utils::fs::remove_path(&path))?; | ||
| } | ||
| if let Err(e) = utils::fs::mknod_char(&path, dev.major(), dev.minor(), mode) { | ||
| log::warn!( | ||
| "Failed to create character device {} (major={}, minor={}): {} (may require root)", | ||
| path.display(), | ||
| dev.major(), | ||
| dev.minor(), | ||
| e | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| } | ||
| #[cfg(not(unix))] | ||
| { | ||
| log::warn!( | ||
| "Skipping character device (not supported on this platform): {}", | ||
| path.display() | ||
| ); | ||
| return Ok(()); | ||
| } | ||
| } |
There was a problem hiding this comment.
The logic for handling DataKind::BlockDevice and DataKind::CharDevice is nearly identical, leading to significant code duplication. This can be refactored into a helper function to improve maintainability. The helper function could take a boolean flag to distinguish between block and character devices and encapsulate the common logic for reading device numbers, checking permissions, and creating the device node.
| use nix::sys::stat::{self, SFlag}; | ||
| let dev = encode_rdev(major, minor); | ||
| let mode = stat::Mode::from_bits_truncate(mode); | ||
| stat::mknod(path.as_ref(), SFlag::S_IFBLK, mode, dev).map_err(io::Error::other) |
There was a problem hiding this comment.
Using io::Error::other to wrap nix::Error can obscure the original error kind. Since nix::Error implements From<nix::Error> for io::Error, you can use Into::into to perform an idiomatic conversion, which preserves more error information. This also applies to mknod_char and mkfifo.
| stat::mknod(path.as_ref(), SFlag::S_IFBLK, mode, dev).map_err(io::Error::other) | |
| stat::mknod(path.as_ref(), SFlag::S_IFBLK, mode, dev).map_err(Into::into) |
| use nix::sys::stat::{self, SFlag}; | ||
| let dev = encode_rdev(major, minor); | ||
| let mode = stat::Mode::from_bits_truncate(mode); | ||
| stat::mknod(path.as_ref(), SFlag::S_IFCHR, mode, dev).map_err(io::Error::other) |
There was a problem hiding this comment.
Using io::Error::other to wrap nix::Error can obscure the original error kind. Since nix::Error implements From<nix::Error> for io::Error, you can use Into::into to perform an idiomatic conversion, which preserves more error information.
| stat::mknod(path.as_ref(), SFlag::S_IFCHR, mode, dev).map_err(io::Error::other) | |
| stat::mknod(path.as_ref(), SFlag::S_IFCHR, mode, dev).map_err(Into::into) |
| use nix::sys::stat::Mode; | ||
| use nix::unistd; | ||
| let mode = Mode::from_bits_truncate(mode); | ||
| unistd::mkfifo(path.as_ref(), mode).map_err(io::Error::other) |
There was a problem hiding this comment.
Using io::Error::other to wrap nix::Error can obscure the original error kind. Since nix::Error implements From<nix::Error> for io::Error, you can use Into::into to perform an idiomatic conversion, which preserves more error information.
| unistd::mkfifo(path.as_ref(), mode).map_err(io::Error::other) | |
| unistd::mkfifo(path.as_ref(), mode).map_err(Into::into) |
| pub fn new_block_device(name: EntryName, major: u32, minor: u32) -> io::Result<Self> { | ||
| let option = WriteOptions::store(); | ||
| let context = get_writer_context(option)?; | ||
| let mut writer = get_writer(FlattenWriter::new(), &context)?; | ||
| writer.write_all(&DeviceNumbers::new(major, minor).to_bytes())?; | ||
| let (iv, phsf) = match context.cipher { | ||
| None => (None, None), | ||
| Some(WriteCipher { context: c, .. }) => (Some(c.iv), Some(c.phsf)), | ||
| }; | ||
| Ok(Self { | ||
| data: Some(writer), | ||
| iv, | ||
| phsf, | ||
| ..Self::new(EntryHeader::for_block_device(name)) | ||
| }) | ||
| } | ||
|
|
||
| /// Creates a new character device entry with the given name and device numbers. | ||
| /// | ||
| /// # Arguments | ||
| /// | ||
| /// * `name` - The name of the entry to create. | ||
| /// * `major` - The major device number. | ||
| /// * `minor` - The minor device number. | ||
| /// | ||
| /// # Returns | ||
| /// | ||
| /// A new [EntryBuilder]. | ||
| /// | ||
| /// # Errors | ||
| /// | ||
| /// Returns an error if initialization fails. | ||
| /// | ||
| /// # Examples | ||
| /// ``` | ||
| /// use libpna::{EntryBuilder, EntryName}; | ||
| /// | ||
| /// // Create a character device entry (e.g., /dev/null with major=1, minor=3) | ||
| /// let builder = EntryBuilder::new_char_device( | ||
| /// EntryName::try_from("dev/null").unwrap(), | ||
| /// 1, | ||
| /// 3, | ||
| /// ).unwrap(); | ||
| /// let entry = builder.build().unwrap(); | ||
| /// ``` | ||
| #[inline] | ||
| pub fn new_char_device(name: EntryName, major: u32, minor: u32) -> io::Result<Self> { | ||
| let option = WriteOptions::store(); | ||
| let context = get_writer_context(option)?; | ||
| let mut writer = get_writer(FlattenWriter::new(), &context)?; | ||
| writer.write_all(&DeviceNumbers::new(major, minor).to_bytes())?; | ||
| let (iv, phsf) = match context.cipher { | ||
| None => (None, None), | ||
| Some(WriteCipher { context: c, .. }) => (Some(c.iv), Some(c.phsf)), | ||
| }; | ||
| Ok(Self { | ||
| data: Some(writer), | ||
| iv, | ||
| phsf, | ||
| ..Self::new(EntryHeader::for_char_device(name)) | ||
| }) | ||
| } |
There was a problem hiding this comment.
The functions new_block_device and new_char_device are almost identical, leading to code duplication. You can refactor this by creating a private helper function that takes a boolean flag to distinguish between block and character devices. This would centralize the logic for creating device entries and improve maintainability.
github-actions
Bot
added
cli
1 participant
No description provided.