D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:d2172d3c0843: Fix inverted logic in 
IOKitStorage::isRemovable (authored by mwolff).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D27065?vs=74772=75857

REVISION DETAIL
  https://phabricator.kde.org/D27065

AFFECTED FILES
  src/solid/devices/backends/iokit/iokitstorage.cpp

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-mac, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb accepted this revision.
rjvbb added a comment.
This revision is now accepted and ready to land.


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

REPOSITORY
  R245 Solid

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-mac, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb added a subscriber: kde-mac.
rjvbb added a comment.


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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-mac, kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, 
bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb added a comment.


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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  ping? any update?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb added a comment.


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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  In D27065#608920 , @rjvbb wrote:
  
  > 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).
  
  F8095364: mac_disks.png 
  
  > 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 :)

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb added a comment.


  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”`.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  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?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb added a comment.


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

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  In D27065#608818 , @rjvbb wrote:
  
  > In D27065#608746 , @mwolff wrote:
  >
  > > 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?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[René J . V . Bertin]

rjvbb added a reviewer: cgilles.
rjvbb added a comment.


  In D27065#608746 , @mwolff wrote:
  
  > 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?

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb, cgilles
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a comment.


  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  (0x3a70c7)  (qulonglong)
  StorageVolume.ignored = false  (bool)
  StorageVolume.usage = 'PartitionTable'  (0x3)  (enum)
  StorageVolume.fsType = ''  (string)
  StorageVolume.label = ''  (string)
  StorageVolume.uuid = ''  (string)
  StorageVolume.size = 251000193024  (0x3a70c7)  (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  (0x3a70c7)  (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)`

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a reviewer: rjvbb.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks, rjvbb
Cc: kde-frameworks-devel, LeGast00n, cblack, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff added a reviewer: Frameworks.

REPOSITORY
  R245 Solid

REVISION DETAIL
  https://phabricator.kde.org/D27065

To: mwolff, #frameworks
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns

D27065: Fix inverted logic in IOKitStorage::isRemovable

[Milian Wolff]

mwolff created this revision.
Herald added a project: Frameworks.
Herald added a subscriber: kde-frameworks-devel.
mwolff requested review of this revision.

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

REPOSITORY
  R245 Solid

BRANCH
  master

REVISION DETAIL
  https://phabricator.kde.org/D27065

AFFECTED FILES
  src/solid/devices/backends/iokit/iokitstorage.cpp

To: mwolff
Cc: kde-frameworks-devel, LeGast00n, GB_2, michaelh, ngraham, bruns