Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change major/minor format into version format #123

Closed
wants to merge 1 commit into from
+21 −35
Closed

Change major/minor format into version format #123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions doc/metadata.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,8 +90,7 @@

// chr
struct {
dev_t major;
dev_t minor;
dev_t version;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's call this device_number instead of version (here and in the subsequent places):
Quoting Linux device drivers

The combined device number (the major and minor numbers concatenated together) resides in the field i_rdev of the inode structure.

},

// dir
Expand All @@ -102,8 +101,7 @@
// blk; do we even want these? seems like maybe not since they're
// system specific.
struct {
dev_t major;
dev_t minor;
version;
},

// reg
Expand Down
8 changes: 4 additions & 4 deletions puzzlefs-lib/src/extractor.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -97,11 +97,11 @@ pub fn extract_rootfs(oci_dir: &str, tag: &str, extract_dir: &str) -> anyhow::Re
InodeMode::Fifo => {
mkfifo(&path, Mode::S_IRWXU)?;
}
InodeMode::Chr { major, minor } => {
mknod(&path, SFlag::S_IFCHR, Mode::S_IRWXU, makedev(major, minor))?;
InodeMode::Chr { version } => {
mknod(&path, SFlag::S_IFCHR, Mode::S_IRWXU, makedev(version, 0))?;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks the extraction code.
I created a directory with a device node (major number: 1, minor number: 5):

$ sudo mknod foobar c 1 5
$ ls -l
total 0
crw-r--r-- 1 root root 1, 5 Jan  4 21:12 foobar

Using the existing puzzlefs version, the major and minor numbers are preserved:

$ target/debug/puzzlefs build ~/work/cisco/test-puzzlefs/device_number /tmp/puzzle2 device
$ sudo target/debug/puzzlefs extract /tmp/puzzle device here
$ ls -l here
total 0
crw-r--r-- 1 root root 1, 5 Jan  4 21:24 foobar

With your version, the major and minor numbers are corrupted after extraction:

$ target/debug/puzzlefs build ~/work/cisco/test-puzzlefs/device_number /tmp/puzzle2 device
$ sudo target/debug/puzzlefs extract /tmp/puzzle2 device here2
$ ls -l here2
total 0
crw-r--r-- 1 root root 261, 0 Jan  4 21:30 foobar

You should use the stat::major and stat::minor functions from the nix crate to extract them from the combined device_number and pass them to makedev.

Same observation for the InodeMode::Blk case below.

}
InodeMode::Blk { major, minor } => {
mknod(&path, SFlag::S_IFBLK, Mode::S_IRWXU, makedev(major, minor))?;
InodeMode::Blk { version } => {
mknod(&path, SFlag::S_IFBLK, Mode::S_IRWXU, makedev(version, 0))?;
}
InodeMode::Lnk => {
let target = dir_entry.inode.symlink_target()?;
Expand Down
6 changes: 2 additions & 4 deletions puzzlefs-lib/src/format/metadata.capnp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,7 @@
@0x84ae5e6e88b7cbb7;

struct Chr {
major@0: UInt64;
minor@1: UInt64;
version@0: UInt64;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since you're making backwards incompatible changes to the capnproto schema, please also increment the puzzlefs image manifest version.

}

struct DirEntry {
Expand All @@ -16,8 +15,7 @@ struct Dir {
}

struct Blk {
major@0: UInt64;
minor@1: UInt64;
version@0: UInt64;
}

struct FileChunk {
Expand Down
36 changes: 13 additions & 23 deletions puzzlefs-lib/src/format/types.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
use capnp::{message, serialize};
use memmap2::{Mmap, MmapOptions};
use nix::errno::Errno;
use nix::sys::stat;
use std::backtrace::Backtrace;
use std::collections::BTreeMap;
use std::convert::{TryFrom, TryInto};
Expand Down Expand Up @@ -243,10 +242,7 @@ mod tests {
},
Inode {
ino: 65343,
mode: InodeMode::Chr {
major: 64,
minor: 65536,
},
mode: InodeMode::Chr { version: 64 },
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's use stat::makedev here to preserve the original values for the major and minor versions (i.e. 64 and 65536, respectively).

uid: 10,
gid: 10000,
permissions: DEFAULT_DIRECTORY_PERMISSIONS,
Expand Down Expand Up @@ -372,18 +368,16 @@ impl Inode {
let mode = if file_type.is_fifo() {
InodeMode::Fifo
} else if file_type.is_char_device() {
let major = stat::major(md.rdev());
let minor = stat::minor(md.rdev());
InodeMode::Chr { major, minor }
let version = md.rdev();
InodeMode::Chr { version }
} else if file_type.is_dir() {
return Err(io::Error::new(
io::ErrorKind::Other,
format!("{ino} is a dir"),
));
} else if file_type.is_block_device() {
let major = stat::major(md.rdev());
let minor = stat::minor(md.rdev());
InodeMode::Blk { major, minor }
let version = md.rdev();
InodeMode::Blk { version }
} else if file_type.is_file() {
return Err(io::Error::new(
io::ErrorKind::Other,
Expand Down Expand Up @@ -480,9 +474,9 @@ impl Inode {
pub enum InodeMode {
Unknown,
Fifo,
Chr { major: u64, minor: u64 },
Chr { version: u64 },
Dir { dir_list: DirList },
Blk { major: u64, minor: u64 },
Blk { version: u64 },
File { chunks: Vec<FileChunk> },
Lnk,
Sock,
Expand All @@ -500,15 +494,13 @@ impl InodeMode {
Ok(crate::metadata_capnp::inode::mode::Chr(reader)) => {
let r = reader?;
Ok(InodeMode::Chr {
major: r.get_major(),
minor: r.get_minor(),
version: r.get_version(),
})
}
Ok(crate::metadata_capnp::inode::mode::Blk(reader)) => {
let r = reader?;
Ok(InodeMode::Blk {
major: r.get_major(),
minor: r.get_minor(),
version: r.get_version(),
})
}
Ok(crate::metadata_capnp::inode::mode::File(reader)) => {
Expand Down Expand Up @@ -554,10 +546,9 @@ impl InodeMode {
match &self {
Self::Unknown => builder.set_unknown(()),
Self::Fifo => builder.set_fifo(()),
Self::Chr { major, minor } => {
Self::Chr { version } => {
let mut chr_builder = builder.reborrow().init_chr();
chr_builder.set_minor(*minor);
chr_builder.set_major(*major);
chr_builder.set_version(*version);
}
Self::Dir { dir_list } => {
let mut dir_builder = builder.reborrow().init_dir();
Expand All @@ -572,10 +563,9 @@ impl InodeMode {
dir_entry_builder.set_name(&entry.name);
}
}
Self::Blk { major, minor } => {
Self::Blk { version } => {
let mut blk_builder = builder.reborrow().init_blk();
blk_builder.set_minor(*minor);
blk_builder.set_major(*major);
blk_builder.set_version(*version);
}
Self::File { chunks } => {
let chunks_len = chunks.len().try_into()?;
Expand Down
Loading