Fix inverted logic in IOKitStorage::isRemovable
ClosedPublic

Authored by mwolff on Jan 31 2020, 1:29 PM.

Details

Summary

Reading through the code, I realized that the isRemovable check
returned true when the kDADiskDescriptionDeviceInternalKey property
is set to true. But that sounds like the check needs to be inverted:

According to [1] e.g. a disk is non-removable when it is internal.
And kDADiskDescriptionDeviceInternalKey returns whether the disk
is internal, not external.

[1]: https://stackoverflow.com/questions/38499860/thunderbolt-drives-not-marked-as-ejectable-in-disk-arbitration-iokit-although-th#comment64407405_38499860

I do not own a device with IOKit platfor, so I cannot actually test
whether this patch is correct or not.

Diff Detail

Repository
R245 Solid
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mwolff created this revision.Jan 31 2020, 1:29 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptJan 31 2020, 1:29 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
mwolff requested review of this revision.Jan 31 2020, 1:29 PM

before:

udi = 'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver/APPLE SSD SM0256F Media'
  parent = 'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver'  (string)
  vendor = ''  (string)
  product = 'APPLE SSD SM0256F                       '  (string)
  description = 'APPLE SSD SM0256F Media'  (string)
  icon = 'drive-removable-media'  (string)
  Block.major = 1  (0x1)  (int)
  Block.minor = 0  (0x0)  (int)
  Block.device = '/dev/disk0'  (string)
  StorageAccess.accessible = false  (bool)
  StorageAccess.filePath = ''  (string)
  StorageAccess.ignored = false  (bool)
  StorageDrive.bus = 'Platform'  (0x5)  (enum)
  StorageDrive.driveType = 'HardDisk'  (0x0)  (enum)
  StorageDrive.removable = true  (bool)
  StorageDrive.hotpluggable = false  (bool)
  StorageDrive.inUse = true  (bool)
  StorageDrive.size = 251000193024  (0x3a70c70000)  (qulonglong)
  StorageVolume.ignored = false  (bool)
  StorageVolume.usage = 'PartitionTable'  (0x3)  (enum)
  StorageVolume.fsType = ''  (string)
  StorageVolume.label = ''  (string)
  StorageVolume.uuid = ''  (string)
  StorageVolume.size = 251000193024  (0x3a70c70000)  (qulonglong)

udi = 'IOService:/AppleACPIPlatformExpert/PCI0@0/AppleACPIPCI/RP06@1C,5/IOPP/SSD0@0/AppleAHCI/PRT0@0/IOAHCIDevice@0/AppleAHCIDiskDriver/IOAHCIBlockStorageDevice/IOBlockStorageDriver/APPLE SSD SM0256F Media'
  BSD Major = 1  (0x1)  (int)
  BSD Minor = 0  (0x0)  (int)
  BSD Name = 'disk0'  (string)
  BSD Unit = 0  (0x0)  (int)
  Content = 'GUID_partition_scheme'  (string)
  Content Hint = ''  (string)
  Ejectable = false  (bool)
  IOBusyInterest = 'IOCommand is not serializable'  (string)
  IOGeneralInterest = 'IOCommand is not serializable'  (string)
  IOMediaIcon = ''  (string)
  IOPolledInterfaceStack = 'IOPolledFilePollers is not serializable'  (string)
  Leaf = false  (bool)
  Open = true  (bool)
  Preferred Block Size = 512  (0x200)  (qlonglong)
  Removable = false  (bool)
  Size = 251000193024  (0x3a70c70000)  (qlonglong)
  Whole = true  (bool)
  Writable = true  (bool)
  className = 'IOMedia'  (string)

Note the Ejectable = false (bool) vs. StorageDrive.removable = true (bool). The patch here fixes it to yield StorageDrive.removable = false (bool)

before:

[snip]

Note the Ejectable = false (bool) vs. StorageDrive.removable = true (bool). The patch here fixes it to yield StorageDrive.removable = false (bool)

Where does this output come from, and what kind of drive does it concern (apart from some kind of Apple-branded SSD)? In other words, should removable be false, or should ejectable be true? I concur that the status quo appears inappropriate, at least in "Finder speak" which uses "Eject" for any form of unmounting regardless of whether it implies a physical eject.

In principle my IOKit code worked appropriately with the various drives I have at my disposal (USB, Firewire, even an internal optical drive) and as far as Solid and IOKit API/protocols can be mapped to one another. Digikam identifies removable drives correctly, for instance.

The logic in summary here seems correct but it's not impossible that I implemented one or two counterintuitive things in order to make things behave the way Mac users would expect. I'll try to take a look this week but maybe Gilles can actually test with a Thunderbolt enclosure?

before:

[snip]

Note the Ejectable = false (bool) vs. StorageDrive.removable = true (bool). The patch here fixes it to yield StorageDrive.removable = false (bool)

Where does this output come from, and what kind of drive does it concern (apart from some kind of Apple-branded SSD)? In other words, should removable be false, or should ejectable be true? I concur that the status quo appears inappropriate, at least in "Finder speak" which uses "Eject" for any form of unmounting regardless of whether it implies a physical eject.

The output comes from running solid-hardware5 details and solid-hardware nonportableinfo on the UID.

In principle my IOKit code worked appropriately with the various drives I have at my disposal (USB, Firewire, even an internal optical drive) and as far as Solid and IOKit API/protocols can be mapped to one another. Digikam identifies removable drives correctly, for instance.

The logic in summary here seems correct but it's not impossible that I implemented one or two counterintuitive things in order to make things behave the way Mac users would expect. I'll try to take a look this week but maybe Gilles can actually test with a Thunderbolt enclosure?

The output comes from running `solid-hardware5 details` and `solid-hardware nonportableinfo` on the UID.

That much I got, but on what hardware and with what kind of drive (the summary basically says you don't have a Mac)? Can you add a screenshot of how the drive in question is shown in the Finder?

In short, I need to be able to assess how appropriate the output is.

I now got remote access to a macOS machine. That's where I now got the output from. How do I see all drives in finder? I.e. how could I make the association between the solid-hardware5 output and finder?

How do you connect? The Mac OS has a built-in VNC server but it has to be activated. Once it is you should be able to connect using any VNC client (possibly using ssh tunnelling?).

You can configure the Finder to show every connected volume on the desktop, IIRC it used to be the default that it shows everything. Should be in the Preferences that you can find under the application menu (the one immediately to the right of the Apple icon/menu).

Can you please ask the owner how that drive is connected? The SM0256F is a PCIe drive; according to https://www.anandtech.com/show/9136/the-2015-macbook-review/8 In the 2013 MacBook Air 13” for example the drive model was “SM0256F”.

How do you connect? The Mac OS has a built-in VNC server but it has to be activated. Once it is you should be able to connect using any VNC client (possibly using ssh tunnelling?).

Yes, I'm connected via VNC and ssh.

You can configure the Finder to show every connected volume on the desktop, IIRC it used to be the default that it shows everything. Should be in the Preferences that you can find under the application menu (the one immediately to the right of the Apple icon/menu).

Can you please ask the owner how that drive is connected? The SM0256F is a PCIe drive; according to https://www.anandtech.com/show/9136/the-2015-macbook-review/8 In the 2013 MacBook Air 13” for example the drive model was “SM0256F”.

This is a MacBook Pro Retina, 13-inch, Mid 2014 with macOS 10.15.2 with only this one disk. It's clearly not supposed to get removed, imo :)

rjvbb added a comment.Feb 10 2020, 2:32 PM
This is a MacBook Pro Retina, 13-inch, Mid 2014 with macOS 10.15.2 with only this one disk. It's clearly not supposed to get removed, imo :)

Indeed. The icon shown is for internal disks.

Thanks for drawing attention to this, I'm going to have to look into things.

Is it possible that PCIe drives are mounted over something other than SATA (I don't see any mention of that bus in the UDI, and IIRC I do on my own 2011 Mac that still uses 2.5" drives.

ping? any update?

rjvbb added a comment.EditedFeb 13 2020, 12:35 PM

Sorry, no. Swamped with last-minute reconstruction efforts in the, erm, structure that's supposed to become my new house next week :-/

(The good news is that it has 200Mbps glass fibre internet, the lesser news that the walls are so thick we lose up to 90% of that speed in the various rooms when using wifi so I'll be having to lay cables...)

rjvbb added a subscriber: kde-mac.Feb 15 2020, 9:36 AM

Well, I got as far as confirming you're probably right, when I finally got to sit at my Mac yesterday night ... and then when I woke up it was 3am.

I'm tempted to green-light this but I think someone is going to have to verify the results after flipping the logic combined with the check of the Removable device property, with more than just a single internal drive. Let's see if anyone else is reading the kde-mac ML ...

rjvbb accepted this revision.Feb 15 2020, 10:45 PM

Works fine as far as I can tell, amazing no one else noticed this before!

This revision is now accepted and ready to land.Feb 15 2020, 10:45 PM
This revision was automatically updated to reflect the committed changes.