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

Jump to bottom

(exa PR) 1190: Fix device major minor numbers #13

Closed
wants to merge 7 commits into from
Closed

(exa PR) 1190: Fix device major minor numbers #13

wants to merge 7 commits into from

Conversation

cafkafk
Copy link
Member

@cafkafk cafkafk commented Jul 29, 2023 •
edited
Loading

SteveLauC and others added 3 commits May 2, 2023 22:08
@SteveLauC @cesarpastorini
feat: device number
a2cf45d
@SteveLauC @cesarpastorini
remove the type cast
b854e7d
@cesarpastorini
Use i64 to accomodate for device files' major and minor numbers
33fae8b 
This is meant as the final step in fixing ogham/exa#1126.

The different build targets of Rust libc's `major()` and `minor()`
functions have conflicting return types like `c_uint`, `c_int` and
`i32`, so an `i64` should be able to represent all of them.
@cafkafk cafkafk changed the title (exa PR) 1190 (exa PR) 1190: Fix device major minor numbers Jul 29, 2023
@cafkafk cafkafk self-assigned this Jul 29, 2023
cafkafk
cafkafk commented Jul 30, 2023
View reviewed changes
src/fs/file.rs Show resolved Hide resolved
cafkafk
cafkafk commented Jul 30, 2023
View reviewed changes
src/fs/fields.rs
Comment on lines +197 to +198
pub major: i64,
pub minor: i64,
Copy link
Member Author

Choose a reason for hiding this comment

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

Considering libc defines pub type c_uint = u32;, I don't see the reason for using i64, would this ever be negative? https://docs.rs/libc/latest/libc/type.c_uint.html

Copy link
Member Author

Choose a reason for hiding this comment

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

this might be to make space for this difference:

# Linux
pub type dev_t = u64;
pub fn major(dev: ::dev_t) -> ::c_uint;
pub fn minor(dev: ::dev_t) -> ::c_uint;

# macOS
pub type dev_t = i32;
pub fn major(dev: dev_t) -> i32;
pub fn minor(dev: dev_t) -> i32;

ogham/exa#1126 (comment)

Copy link

Choose a reason for hiding this comment

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

would this ever be negative?

For glibc, I guess it won't be.

Ref: how to interpret the dev_t type

Copy link
Member Author

Choose a reason for hiding this comment

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

would this ever be negative?

For glibc, I guess it won't be.

Ref: how to interpret the dev_t type

I guess the original intent might have been to have space enough for both pub fn major(dev: dev_t) -> i32; and pub fn major(dev: ::dev_t) -> ::c_uint; (u32)?

cafkafk added 3 commits July 30, 2023 06:34
@cafkafk
Merge branch 'main' into pr-1190
90563c2
@cafkafk
fix: use major-minor c_uint equivalent
b1e642c 
Signed-off-by: Christina Sørensen <christina@cafkafk.com>
@cafkafk
fix: bump to libc 0.2.147
21d93d4 
Signed-off-by: Christina Sørensen <christina@cafkafk.com>
@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

This was super helpful to understand what's going on for future reference: ogham/exa#1126 (comment)

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

In order to progress on this we'd need feedback from someone with an apple box

@cafkafk cafkafk marked this pull request as draft July 30, 2023 04:50
@SteveLauC
Copy link

In order to progress on this we'd need feedback from someone with an apple box

For macOS, I am not sure if the major can be negative, but the minor won't be, see:

https://github.com/rust-lang/libc/blob/28ab9b9e7bd04a5c5aca3f4d78583214f63d4002/src/unix/bsd/apple/mod.rs#L5186-L5196

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

For potential testers:

cargo run --release -- -l /dev/ > changes.txt; exa -l /dev/ > old.txt; diff changes.txt old.txt;

@cafkafk
Copy link
Member Author

cafkafk commented Jul 30, 2023

I found an apple laptop to test this on:

error[E0308]: mismatched types
--> src/fs/file.rs:366:39
|
366 | major: unsafe { major(device_ids) as i64 },
| ----- ^^^^^^^^^^ expected i32, found u64
| |
| arguments to this function are incorrect
|
note: function defined here
--> /Users/ces/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.147/src/unix/bsd/apple/mod.rs:5186:12
|
5186 | pub fn major(dev: dev_t) -> i32 {
| ^^^^^
help: you can convert a u64 to an i32 and panic if the converted value doesn't fit
|
366 | major: unsafe { major(device_ids.try_into().unwrap()) as i64 },
| ++++++++++++++++++++

error[E0308]: mismatched types
--> src/fs/file.rs:367:39
|
367 | minor: unsafe { minor(device_ids) as i64 },
| ----- ^^^^^^^^^^ expected i32, found u64
| |
| arguments to this function are incorrect
|
note: function defined here
--> /Users/ces/.cargo/registry/src/index.crates.io-6f17d22bba15001f/libc-0.2.147/src/unix/bsd/apple/mod.rs:5190:12
|
5190 | pub fn minor(dev: dev_t) -> i32 {
| ^^^^^
help: you can convert a u64 to an i32 and panic if the converted value doesn't fit
|
367 | minor: unsafe { minor(device_ids.try_into().unwrap()) as i64 },
| ++++++++++++++++++++

For more information about this error, try rustc --explain E0308.
error: could not compile eza (bin "eza") due to 2 previous errors

@cafkafk cafkafk added not ready for PRs that aren't finished os › macos labels Jul 30, 2023
@sbatial sbatial mentioned this pull request Jul 30, 2023
Closed
63 tasks
@cafkafk cafkafk added this to the exa pulls done milestone Jul 31, 2023
@cafkafk
Copy link
Member Author

cafkafk commented Aug 4, 2023

This needs an apple user to work on these changes.

@cafkafk cafkafk removed not ready for PRs that aren't finished looking for testers needs feedback labels Aug 4, 2023
@github-actions
Copy link

github-actions bot commented Sep 4, 2023

This pull request is stale because it has been open for 30 days with no activity.

@github-actions
Copy link

This pull request was closed because it has been inactive for 14 days since being marked as stale.

@github-actions github-actions bot closed this Sep 19, 2023
@cafkafk cafkafk mentioned this pull request Sep 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveLauC SteveLauC SteveLauC left review comments

Assignees

@cafkafk cafkafk

Labels
help wanted Extra attention is needed os › macos stale › pr
Projects
Eza Pull-Requests
Status: ✅ Done
Milestone
exa pulls
Development

Successfully merging this pull request may close these issues.
3 participants
@cafkafk @SteveLauC @cesarpastorini