D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni abandoned this revision.
anthonyfieroni added a comment.


  https://phabricator.kde.org/D14661

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  
  
  In D13869#303905 , @bruns wrote:
  
  > And thinking about it, this very likely makes your code wrong. Have you 
tried mounting a filesystem again after unmounting it, **using via any mean 
implemented via solid**? That is, "solid-hardware mount ...", the device 
notifier, ...
  
  
  So write an example with commands, mounting, umounting flash drive, which is 
a filesystem, works.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added a comment.


  In D13869#303861 , @anthonyfieroni 
wrote:
  
  > Since you make question to new code I make for old one, did you know why 
Device interfaces is ever used, it's look wrong and why interfaces should be 
empty, after all my tests, it's easy to see that they are never empty.
  
  
  Thats trivial - remove the device if *all* interfaces have been removed. And 
thats correct, otherwise you can no longer call any solid method on the device.
  
  And thinking about it, this very likely makes your code wrong. Have you tried 
mounting a filesystem again after unmounting it, **using via any mean 
implemented via solid**? That is, "solid-hardware mount ...", the device 
notifier, ...

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added a comment.


  In D13869#303877 , @anthonyfieroni 
wrote:
  
  > About loop device it should present as well
  >  
http://storaged.org/doc/udisks2-api/2.6.3/gdbus-org.freedesktop.UDisks2.Loop.html
  >  After all you have ony hypothetical arguments, if you know case that this 
code not work let's discuss, till then this is right approach.
  
  
  Of course it is present for a loop device, but this is about **removed** aka 
lost interfaces.
  
  These are not hypothetical arguments, but practical arguments.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  About loop device it should present as well
  
http://storaged.org/doc/udisks2-api/2.6.3/gdbus-org.freedesktop.UDisks2.Loop.html

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  Since you make question to new code I make for old one, did you why Device 
interfaces is ever used, it's look wrong and they should be empty, after all my 
tests, it's easy to see that they are never empty.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added a comment.


  In D13869#303563 , @ngraham wrote:
  
  > FWIW, we got a thumbs up from someone in 
https://bugs.kde.org/show_bug.cgi?id=394348:
  
  
  A thumbs up is nice, but this does not catch the thumbs down of all the 
persons who are exposed to the changed code later, having a different setup. We 
have seen this in solid several times lately, because every change was only 
checked to "fix" a non-working case, but omitted all the other cases.
  
  Every change should both be checked in a real setup, **and** have a sound 
reasoning behind it why it is correct.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns requested changes to this revision.
bruns added a comment.
This revision now requires changes to proceed.


  The change needs a proper commit message. A reference to the bug tracker is 
not sufficient.
  
  The commit message should (at least) include:
  
  - Description of the setup
- The UDI of the relevant Solid device
- The UDIs of its parents
- Relevant Solid attributes of the devices, e.g. from solid-hardware details
  - Description of the actions done
  - Old and new behaviour
  
  Think of what happens if anyone changes the code later. The person must be 
able to replicate the setup, to check if any further changes break the code.

INLINE COMMENTS

> udisksmanager.cpp:222
> +if (interfaces.contains(UD2_DBUS_INTERFACE_FILESYSTEM)
> +||  interfaces.contains(UD2_DBUS_INTERFACE_LOOP)) {
>  emit deviceRemoved(udi);

According to your description (which is hard to follow, because you refrain 
from writing complete sentences), what triggers here is the removal of the 
o.f.U2.FileSystem interface. There is no reasoning why the Loop interface 
matters here.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, bruns
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  I'm using too.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Nathaniel Graham]

ngraham added a comment.


  FWIW, we got a thumbs up from someone in 
https://bugs.kde.org/show_bug.cgi?id=394348:
  
  >   Nick Stefanov 2018-08-02 06:59:28 MDT
  > 
  > I get the Antony's patch from here:
  >  https://phabricator.kde.org/D13869
  >  I use it two weeks already and so far I haven't any problems at all. The 
patch works flawlessly for me.
  >  Kudos to anthonyfieroni for providing fix for this annoying problem.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: ngraham, bcooksley, bruns, kde-frameworks-devel, michaelh

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  For 5.49?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  Ping, anything unclear ?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  Device(udi).interfaces() ("org.freedesktop.UDisks2.Loop", 
"org.freedesktop.UDisks2.Block") 
  interfaces ("org.freedesktop.UDisks2.Filesystem")

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> bruns wrote in udisksmanager.cpp:220
> This does not answer my question. What I need from you is:
> 
> - which interfaces are in `interfaces`
> - which interfaces are in `device.interfaces()`
> 
> so the **exact** case where the patched code triggers, but the old does not.

In which cases what you to know the interfaces?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in udisksmanager.cpp:220
> Old code is complete broken, about me, it does not check any interface except 
> that it's empty.

Can you please just answer the question?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> bruns wrote in udisksmanager.cpp:220
> This does not answer my question. What I need from you is:
> 
> - which interfaces are in `interfaces`
> - which interfaces are in `device.interfaces()`
> 
> so the **exact** case where the patched code triggers, but the old does not.

Old code is complete broken, about me, it does not check any interface except 
that it's empty.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> anthonyfieroni wrote in udisksmanager.cpp:220
> According to docs, 
> https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager,
>  interfaces should not be empty but a dict of looses ones.

This does not answer my question. What I need from you is:

- which interfaces are in `interfaces`
- which interfaces are in `device.interfaces()`

so the **exact** case where the patched code triggers, but the old does not.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  Moundting by cdemu also works as expected, not without patch as is described 
in bug report.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


Performing working tests

- mount -o loop xxx.iso /mnt/xxx

- mount xxx/iso /mnt/xxx

- Add Android phone

- Add second phone, first become inactive, second works

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 38153.
anthonyfieroni added a comment.


  First tests looks good, i'll make more later

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13869?vs=37129=38153

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksmanager.cpp

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Ben Cooksley]

bcooksley added a comment.


  Predicates are Solid's query language for searching for only the devices you 
are interested in.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bcooksley, bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  I'm aware that does not an idea, what is the purpose of Predicate. What 
should i search in Solid::Predicate::matches?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added a comment.


  In D13869#286746 , @broulik wrote:
  
  > Sure, but the device isn't "removed", so emitting a remove signal for 
something that is still there, is wrong, isn't it? Perhaps we could signal a 
removal if the device lost *all* its interfaces? But it probably doesn't in the 
unmount case.
  
  
  a valid device/dbus object has at least the `org.freedesktop.DBus.Properties` 
interface, so this signal is already emitted when the device is removed (== 
last interface removed).
  
  I think the code needs some overhauling - AFAIK it was written when UDisks2 
had a much more simplistic interface. The whole properties cache is quite 
broken ("something has changed - throw away the cache - fetch everything again" 
, "we don't know the property - lets fetch it again"), we are doing to many 
roundtrips.
  
  The list of properties is announced in the DBus introspection data for each 
interface. There is no need for the "negative" cache. We are mixing properties 
from different interfaces (e.g. size, a filesystem size may differ from the 
partition size).
  
  While the intention of the patch is right, I don't think its the right way to 
do it. I would favor a solution which signals changes in the `Predicate` 
matches. This is definitely more work, but otherwise we are papering over one 
issue after the other.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  So about docs when interface is added it will notify again for mounting, i 
think that's the idea behind interfaces.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Kai Uwe Broulik]

broulik added a comment.


  Sure, but the device isn't "removed", so emitting a remove signal for 
something that is still there, is wrong, isn't it? Perhaps we could signal a 
removal if the device lost *all* its interfaces? But it probably doesn't in the 
unmount case.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added a comment.


  So if KFilePlacesModel wants to hide, i don't see how this will break things, 
it only notify when udisk2 loose interface to mounted fs.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Kai Uwe Broulik]

broulik added a comment.


  I don't think this is the right approach, technically the device isn't gone 
but filtered by the client (`KFilePlacesModel` and Device Notifier use a 
predicate to show only matching devices).
  This probably needs some "device changed" signal, or even cooler, in 
`Solid::Predicate` a tracker that signals "the devices that match this 
predicate changed" but this is probably quite difficult to achieve

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni added inline comments.

INLINE COMMENTS

> bruns wrote in udisksmanager.cpp:220
> This code needs some restructuring, and with the additional conditions, some 
> comments ...
> 
> 1. `if (udi.isEmpty()) return;`
> 2. you are whitelisting a lot of conditions (`... || .. || ...`) - are the 
> any cases left where the signal is **not** emitted?

According to docs, 
https://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfaces-objectmanager,
 interfaces should not be empty but a dict of looses ones.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 37129.
anthonyfieroni added a comment.


  1. Return early when path is empty
  2. Don't check for empty interfaces

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13869?vs=37110=37129

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksmanager.cpp

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Stefan Brüns]

bruns added inline comments.

INLINE COMMENTS

> udisksmanager.cpp:220
>  
> -if (!udi.isEmpty() && (interfaces.isEmpty() || 
> device.interfaces().isEmpty())) {
> +if (!udi.isEmpty() && (!dir.exists() || dir.isEmpty() || 
> interfaces.isEmpty()
> +|| interfaces.contains(UD2_DBUS_INTERFACE_FILESYSTEM)

This code needs some restructuring, and with the additional conditions, some 
comments ...

1. `if (udi.isEmpty()) return;`
2. you are whitelisting a lot of conditions (`... || .. || ...`) - are the any 
cases left where the signal is **not** emitted?

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: bruns, kde-frameworks-devel, michaelh, ngraham

D13869: [solid] Notify when interface to mounted fs is lost

[Albert Astals Cid]

aacid resigned from this revision.
aacid added a comment.


  Removing myself i don't think i know much/anything about solid, sorry

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni updated this revision to Diff 37110.
anthonyfieroni added a comment.


  Add loop interface

REPOSITORY
  R245 Solid

CHANGES SINCE LAST UPDATE
  https://phabricator.kde.org/D13869?vs=37108=37110

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksmanager.cpp

To: anthonyfieroni, broulik, cfeck, dfaure, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni edited the summary of this revision.

REPOSITORY
  R245 Solid

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

To: anthonyfieroni, broulik, cfeck, dfaure, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns

D13869: [solid] Notify when interface to mounted fs is lost

[Anthony Fieroni]

anthonyfieroni created this revision.
anthonyfieroni added reviewers: broulik, cfeck, dfaure, aacid.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: kde-frameworks-devel.
anthonyfieroni requested review of this revision.

REVISION SUMMARY
  Mostly mounted directory are empty when they are umounted

TEST PLAN
  Not tested, it's kid of idea. hope someone can share better experience.

REPOSITORY
  R245 Solid

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

AFFECTED FILES
  src/solid/devices/backends/udisks2/udisksmanager.cpp

To: anthonyfieroni, broulik, cfeck, dfaure, aacid
Cc: kde-frameworks-devel, michaelh, ngraham, bruns