D12648: Update mount point after mount operations

[David Edmundson]

davidedmundson added a comment.


  > There is still no guarantee the method return is equivalent/isochronous 
with the property change.
  
  Sure there is. We know udisks has completely finished mounting a filesystem, 
therefore we know udisks property about whether the file system is mounted will 
have changed.
  It's equivalent to calling QLabel::setText and then after it returns knowing 
you can get the right result from QLabel::text
  
  That's not relying on an implementation detail, it's that anything else would 
be a definite bug.

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12648: Update mount point after mount operations

[Stefan Brüns]

bruns added a comment.


  In D12648#257365 , @davidedmundson 
wrote:
  
  > Let me clarify
  >
  > At the time of the return from udisks the property on udisks is updated but 
the signal saying the property is updated is not yet sent, as solid caches 
properties we have a wrong value locally but udisks has the correct value 
remotely
  
  
  There is still no guarantee the method return is equivalent/isochronous with 
the property change. The property is changed when you receive the 
propertiesChanged signal, not when the method returns. The current behaviour is 
an implementation detail you should not rely on.

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12648: Update mount point after mount operations

[David Edmundson]

davidedmundson added a comment.


  Let me clarify
  
  At the time of the return from udisks the property on udisks is updated but 
the signal saying the property is updated is not yet sent, as solid caches 
properties we have a wrong value locally but udisks has the correct value 
remotely

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12648: Update mount point after mount operations

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> udisksstorageaccess.cpp:170
> +m_device->invalidateCache();
>  m_device->broadcastActionDone("setup");
>  

This replaces one race condition with another (less likely) one. There is no 
guarantee the property is already updated when checkAccessibility is called.

The correct approach would be to defer until the propertiesChanged signal 
arrives and broadcast the "setup done" afterwards.

A different, but more intrusive approach would be to defer any check for the 
"acessible" property on the caller side, waiting for the accessibilityChanged 
signal.

REPOSITORY
  R245 Solid

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

To: davidedmundson, #plasma, broulik, mart
Cc: bruns, #frameworks, michaelh

D12648: Update mount point after mount operations

[David Edmundson]

This revision was automatically updated to reflect the committed changes.
Closed by commit R245:d735708ff11c: Update mount point after mount operations 
(authored by davidedmundson).

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D12648?vs=33460=33488

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

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

To: davidedmundson, #plasma, broulik, mart
Cc: #frameworks, michaelh, bruns

D12648: Update mount point after mount operations

[Kai Uwe Broulik]

broulik accepted this revision.
This revision is now accepted and ready to land.

REPOSITORY
  R245 Solid

BRANCH
  master

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

To: davidedmundson, #plasma, broulik
Cc: #frameworks, michaelh, bruns

D12648: Update mount point after mount operations

[Marco Martin]

mart accepted this revision.

REPOSITORY
  R245 Solid

BRANCH
  master

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

To: davidedmundson, #plasma, broulik, mart
Cc: #frameworks, michaelh, bruns

D12648: Update mount point after mount operations

[David Edmundson]

davidedmundson created this revision.
davidedmundson added a reviewer: Plasma.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
davidedmundson requested review of this revision.

REVISION SUMMARY
  The order of udisks evaluation has changed from:
  
  call Mount
  propertiesChanged
  mount call returns
  
  call Mount
  mount call returns
  propertiesChanged
  
  The mount has finished, but the property is not yet updated.
  
  Solid caches properties, updating them when they change. This worked
  before, but due to the re-ordering client code gets "setupDone" requests
  the mount point, gets an outdated version from the cache and we get
  errors. Invalidating the cache causes us to round-trip to the udisks
  daemon meaning we'll have the correct values.
  
  BUG: 370975

TEST PLAN
  Diagnosed but with dbus-monitor trace 
  Asked someone on the bug report to test this

REPOSITORY
  R245 Solid

BRANCH
  master

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

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

To: davidedmundson, #plasma
Cc: #frameworks, michaelh, bruns