D20433: Use mount point returned from DBus instead of using property value

[Nicolas Fella]

nicolasfella abandoned this revision.
nicolasfella added a comment.


  The issue has been fixed in another commit

REPOSITORY
  R245 Solid

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

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

D20433: Use mount point returned from DBus instead of using property value

[Nicolas Fella]

nicolasfella added a comment.


  In D20433#447667 , @bruns wrote:
  
  > This approach is completely wrong.
  >
  > The right approach is to wait for the information in a PropertiesChanged 
signal, and only when the mountpoint has been set in the property propage the 
signal.
  >
  > This whole "our information is inconsistent, lets query for it explicitly" 
dance is a mess.
  
  
  They key issue is that the PropertiesChanged signal hasn't been emitted by 
the time filePath() is called. We could wait for the signal there, but we don't 
need to since we already get the same information from the mount() call, but we 
just didn't use it before. That won't work if the drive is already mounted by 
the time solid is started, so we query the mountPoints property in that case, 
but that is no different to the old code.

REPOSITORY
  R245 Solid

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

To: nicolasfella, broulik, bruns
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20433: Use mount point returned from DBus instead of using property value

[Stefan Brüns]

bruns added a comment.


  This approach is completely wrong.
  
  The right approach is to wait for the information in a PropertiesChanged 
signal, and only when the mountpoint has been set in the property propage the 
signal.
  
  This whole "our information is inconsistent, lets query for it explicitly" 
dance is a mess.

REPOSITORY
  R245 Solid

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

To: nicolasfella, broulik, bruns
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20433: Use mount point returned from DBus instead of using property value

[Kai Uwe Broulik]

broulik added inline comments.

INLINE COMMENTS

> udisksstorageaccess.cpp:35
>  StorageAccess::StorageAccess(Device *device)
> -: DeviceInterface(device), m_setupInProgress(false), 
> m_teardownInProgress(false), m_passphraseRequested(false)
> +: DeviceInterface(device), m_setupInProgress(false), 
> m_teardownInProgress(false), m_passphraseRequested(false), m_mountPoint()
>  {

Initialization not needed for complex types

> udisksstorageaccess.cpp:82
>  {
> -QByteArrayList mntPoints;
> -
> -if (isLuksDevice()) {  // encrypted (and unlocked) device
> -const QString path = clearTextPath();
> -if (path.isEmpty() || path == "/") {
> -return QString();
> -}
> -Device holderDevice(path);
> -mntPoints = 
> qdbus_cast(holderDevice.prop("MountPoints"));
> +if (m_mountPoint.isEmpty()) {
> +QByteArrayList mntPoints = 
> qdbus_cast(m_device->prop("MountPoints"));

Won't this bypass the luks stuff when it is not mounted or something?
On mount you do this `holderDevice` dance but otherwise you just go straight to 
`MountPoints`

> udisksstorageaccess.cpp:151
> +if (isLuksDevice()) {
> +if(!isAccessible()) { // unlocked device, now mount it
> +mount();

Coding style: `if (!...) {`

> udisksstorageaccess.cpp:200
>  m_teardownInProgress = false;
>  m_device->invalidateCache();
>  m_device->broadcastActionDone("teardown");

Don't you need to clear the `m_mountPoint` after unmount?

REPOSITORY
  R245 Solid

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

To: nicolasfella, broulik, bruns
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20433: Use mount point returned from DBus instead of using property value

[Kai Uwe Broulik]

broulik added a reviewer: bruns.

REPOSITORY
  R245 Solid

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

To: nicolasfella, broulik, bruns
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20433: Use mount point returned from DBus instead of using property value

[Nicolas Fella]

nicolasfella reopened this revision.

REPOSITORY
  R245 Solid

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

To: nicolasfella, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20433: Use mount point returned from DBus instead of using property value

[Nicolas Fella]

This revision was not accepted when it landed; it landed in state "Needs 
Review".
This revision was automatically updated to reflect the committed changes.
Closed by commit R245:3ed42e887066: Use mount point returned from DBus instead 
of using property value (authored by nicolasfella).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D20433?vs=55898=55900

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksstorageaccess.cpp
  src/solid/devices/backends/udisks2/udisksstorageaccess.h

To: nicolasfella, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D20433: Use mount point returned from DBus instead of using property value

[Nicolas Fella]

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

REVISION SUMMARY
  There is a slight time frame (~2ms) where mount has returned but the 
MountPoints property is not updated. When the mounpoint is queried during that 
time it will be empty and opening a plugged in USB device in Dolphin will fail.
  To avoid this use the mountpoint information returned by the mount() call

REPOSITORY
  R245 Solid

BRANCH
  limux_master

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksstorageaccess.cpp
  src/solid/devices/backends/udisks2/udisksstorageaccess.h

To: nicolasfella, broulik
Cc: kde-frameworks-devel, michaelh, ngraham, bruns