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

Overhaul Apple disk implementation improvement #855

Merged

Conversation

complexspaces
Copy link
Contributor

This PR rewrites a significant portion of the Apple disk handling code, partially in response to #841. Some of the notable improvements are:

  • Better detection of devices that are actually ejectable, but just don’t report as such.
  • Continues to skip most1 of the hidden APFS partitions on modern Apple devices.
  • More correctly determines the name of external drives. In my test case, it detected an external SSD had the name External” and not “Untitled 1”, which matches disk utility.
  • More accurate available capacity numbers
  • Almost2 entirely supported on iOS (!)

In more detail, this PR completely replaces DiskArbitration.framework with CoreFoundation's volume handling instead. This was done for three reasons:

  1. CoreFoundation allows accessing the "visibility" property of a volume/disk. This is very helpful for filtering out internal partitions and snapshots.
  2. It is fully supported on all supported versions of iOS, unlike DiskArbitration.framework which is solely available on macOS.
  3. Its supported in the default macOS app sandbox.

By utilizing kCFURLVolumeIsBrowsableKey, a volume property that tells you if its browsable on the system by default, which matches other Apple apps such as Finder volumes, disk utility, and system information's storage devices list. Using this framework also allows us to access better space estimation values that match what the rest of the system would report.

In addition, I took the chance to remove the "hacky" implementation of disk medium type detection and replace it with something well-documented via IOKit. This change is also compatible with the default macOS sandbox, further reducing the feature gap of the crate with and without the apple-sandbox feature enabled.

Finally, I removed the crate's build script to improve compilation times. Ever since Objective-C code was removed from the crate, it hasn't been strictly needed. The #[link] attribute can do the same thing the current version of it was doing to make sure IOKit functions and constants are available.

This PR was tested in and outside of the default macOS app sandbox with a variety of external drives attached, and inside the iOS simulator and on a real iOS device.

Closes #841

Footnotes

  1. In order to make some use cases possible, the disk list now returns /System/Volumes/Data too.

  2. Disk type detection is currently unsupported on iOS due to version requirements.

@complexspaces complexspaces force-pushed the macos-disk-changes branch 2 times, most recently from 80b58de to 20bff69 Compare October 5, 2022 03:41
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

It looks like a great start. Mostly nits at this point. Thanks for working on this!

@GuillaumeGomez
Copy link
Owner

Hi @complexspaces. Do you plan to continue working on this PR?

@complexspaces
Copy link
Contributor Author

Hey there @GuillaumeGomez. I do plan on working on this again, sorry. I have been busy enough with work that this one got de-prioritized. I'll try and have it cleaned up by the weekend at the latest.

@GuillaumeGomez
Copy link
Owner

Don't worry, no hurry. Just wanted to check if you still planned to work on it.

src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
src/apple/disk.rs Outdated Show resolved Hide resolved
@GuillaumeGomez
Copy link
Owner

One comment to resolve and one comment to add and it'll be ready to go. I'm so excited about these changes to be merged. :)

@GuillaumeGomez
Copy link
Owner

Thanks a lot for this work!

@GuillaumeGomez GuillaumeGomez merged commit 31134a7 into GuillaumeGomez:master Oct 30, 2022
@complexspaces
Copy link
Contributor Author

Thank you for working through this with me, I'm glad to see it merged 🎉

@GuillaumeGomez GuillaumeGomez changed the title Overhaul Apple disk implementation Overhaul Apple disk implementation improvement Oct 31, 2022
@complexspaces complexspaces deleted the macos-disk-changes branch October 31, 2022 18:06
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.

No reasonable way to establish the relationship between root directory and volume on macOS.
2 participants