Skip to content

df: detect over-mounted device #4316

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
Closed
wants to merge 10 commits into from
Closed

df: detect over-mounted device #4316

wants to merge 10 commits into from

Conversation

ctsk
Copy link
Contributor

@ctsk ctsk commented Feb 1, 2023

Addresses #3970 - Still needs some cleanup around the error handling. Feedback appreciated!

I added Filesystem::from_mount to ensure only valid (i.e. non-overmounted) Filesystem structs are created. I guess the next step for that would be making Filesystem::new unavailable outside of filesystem.rs

@sylvestre
Copy link
Contributor

what are the two binaries? (they are quite big btw)

Would it be possible to write a test for this?

@ctsk ctsk force-pushed the fix/df-overmounted-device branch from f18724f to 093eb76 Compare February 1, 2023 12:45
@ctsk
Copy link
Contributor Author

ctsk commented Feb 1, 2023

Yup, shouldn't have pushed those blobs.

RE: tests - Isn't it sufficient that a GNU test covers this?

@sylvestre
Copy link
Contributor

RE: tests - Isn't it sufficient that a GNU test covers this?

nope, the GNU testsuite takes an hour to run in the CI.
The Rust one just a minute or two :)

@github-actions
Copy link

github-actions bot commented Feb 1, 2023

GNU testsuite comparison:

GNU test failed: tests/misc/timeout. tests/misc/timeout is passing on 'main'. Maybe you have to rebase?

@github-actions
Copy link

github-actions bot commented Feb 2, 2023

GNU testsuite comparison:

GNU test failed: tests/tail-2/inotify-dir-recreate. tests/tail-2/inotify-dir-recreate is passing on 'main'. Maybe you have to rebase?

@ctsk ctsk marked this pull request as ready for review February 3, 2023 17:01
jfinkels
jfinkels reviewed Feb 5, 2023
View reviewed changes
Copy link
Collaborator

@jfinkels jfinkels left a comment

Choose a reason for hiding this comment

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

Looks good to me, but under the Run GNU root tests job, we still have a FAIL: tests/df/over-mount-device.sh. Any idea why that might be? Were you able to run that test on a local machine or virtual machine?

@sylvestre sylvestre force-pushed the fix/df-overmounted-device branch from 2b38fce to 65fe643 Compare February 7, 2023 08:22
@ctsk
Copy link
Contributor Author

ctsk commented Feb 7, 2023

Looks good to me, but under the Run GNU root tests job, we still have a FAIL: tests/df/over-mount-device.sh. Any idea why that might be? Were you able to run that test on a local machine or virtual machine?

There are quotes missing around the path :)

jfinkels
jfinkels approved these changes Feb 9, 2023
View reviewed changes
Copy link
Collaborator

@jfinkels jfinkels left a comment

Choose a reason for hiding this comment

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

Cool:

PASS: tests/df/over-mount-device.sh

Looks like there are some CI failures though

@ctsk
Copy link
Contributor Author

ctsk commented Mar 1, 2023

This should fix the spell checks, I believe that's all that's left to do?

@sylvestre
Copy link
Contributor

not yet, needs some love on Windows:


---- test_df::test_exclude_all_types stdout ----
run: D:\a\coreutils\coreutils\target\debug\coreutils.exe df --output=fstype
run: D:\a\coreutils\coreutils\target\debug\coreutils.exe df -x NTFS
thread 'test_df::test_exclude_all_types' panicked at 'Command was expected to fail.
stdout = Filesystem              1K-blocks  Used Available Use% Mounted on
\Device\HarddiskVolume1    512000 35788         0 100% 

 stderr = ', tests\by-util\test_df.rs:315:10
stack backtrace:
   0: std::panicking::begin_panic_handler
             at /rustc/31f858d9a511f24fedb8ed997b28304fec809630/library\std\src\panicking.rs:579
   1: core::panicking::panic_fmt
             at /rustc/31f858d9a511f24fedb8ed997b28304fec809630/library\core\src\panicking.rs:64
   2: tests::common::util::CmdResult::failure
             at .\tests\common\util.rs:397
   3: tests::common::util::UCommand::fails
             at .\tests\common\util.rs:1549
   4: tests::test_df::test_exclude_all_types
             at .\tests\by-util\test_df.rs:313
   5: tests::test_df::test_exclude_all_types::closure$0
             at .\tests\by-util\test_df.rs:299
   6: core::ops::function::FnOnce::call_once<tests::test_df::test_exclude_all_types::closure_env$0,tuple$<> >
             at /rustc/31f858d9a511f24fedb8ed997b28304fec809630\library\core\src\ops\function.rs:250
   7: core::ops::function::FnOnce::call_once
             at /rustc/31f858d9a511f24fedb8ed997b28304fec809630/library\core\src\ops\function.rs:250
note: Some details are omitted, run with `RUST_BACKTRACE=full` for a verbose backtrace.

@sylvestre
Copy link
Contributor

@ctsk ping?

@ctsk
Copy link
Contributor Author

ctsk commented Mar 14, 2023

@ctsk ping?

Hey, got caught up with other matters. Might get to it later this week.

It seems like over-mounting is not a thing on windows, hopefully all that's needed is a platform check,

@ctsk ctsk force-pushed the fix/df-overmounted-device branch from 482c251 to 83a032f Compare March 27, 2023 11:28
@ctsk ctsk force-pushed the fix/df-overmounted-device branch from 83a032f to c221f4c Compare March 27, 2023 11:31
@uutils uutils deleted a comment from github-actions bot Mar 27, 2023
@ctsk ctsk closed this Apr 21, 2023
@sylvestre sylvestre mentioned this pull request Apr 22, 2023
Closed
@jfinkels jfinkels mentioned this pull request Jan 11, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jfinkels jfinkels jfinkels approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ctsk @sylvestre @jfinkels