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.

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
Closed

Change major/minor format into version format #123

wants to merge 1 commit into from

Conversation

jklott
Copy link

@jklott jklott commented Jan 2, 2024

Addresses Issue here: #10

This PR replaces major/minor versionining to be a single version variable. u64 should be plenty for both rather than using 2 u64.

Copy link
Collaborator

@ariel-miculas ariel-miculas left a comment

Choose a reason for hiding this comment

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

A few changes are needed, but otherwise looks ok. @hallyn could you also take a look?

@@ -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.

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.

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).

@@ -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.

@hallyn
Copy link
Contributor

hallyn commented Jan 4, 2024

@jklott was this sign-off:

Signed-off-by: Poxxy <[email protected]>

intentional?

@jklott jklott closed this Jan 4, 2024
@jklott jklott deleted the sign-off branch January 4, 2024 20:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants