Differential D14661

Force reevaluation of Predicates if interfaces are removed
ClosedPublic
Actions

Authored by bruns on Aug 6 2018, 8:01 PM.

Details

Reviewers
broulik
ngraham
apol
Group Reviewers
Frameworks
Commits
R245:8e9fc89918c4: Force reevaluation of Predicates if interfaces are removed
Summary

If an application wants to show only specicific devices based on predicate
matching, and one of the matching devices loses an interface which is
critical for the Predicate to match, the application has to be notified.

As there is no dedicated signal to notify the application about the
fact a device no longer has e.g. a Solid::StorageAccess iface, signal the
device has been removed, and immediately readd it, as the device may
still be relevant.

Remove the call to updateBackend(udi), as the device backend listens to
the InterfacesRemoved signal itself and updates its property cache.

CCBUG: 394348

Test Plan
  1. Image file mounting via loop-back
    • udisksctl loop-setup -f ~/fat.img -> Icon appears under "Devices"
    • unmount via context-menu in dolphin -> device icon stays in "Devices"
    • set "Auto-clear" flag on loop (e.g. losetup --detach-all)
    • remount
    • unmount -> device icon vanishes from "Devices"
  2. CD-ROM/DVD
    • icon appear when a non-blank medium is inserted
    • vanishes on medium removal
  3. USB stick
    • for each mountable filesystem, an icon appears under "Removable devices"
    • formatting a partition with a non-mountable filesystem/blank removes the entry
    • formatting with a mountable FS (re)adds the entry

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.
bruns created this revision.Aug 6 2018, 8:01 PM
Restricted Application added a project: Frameworks. · View Herald TranscriptAug 6 2018, 8:01 PM
Restricted Application added a subscriber: kde-frameworks-devel. · View Herald Transcript
bruns requested review of this revision.Aug 6 2018, 8:01 PM
bruns edited the test plan for this revision. (Show Details)Aug 6 2018, 11:24 PM
bruns added reviewers: Frameworks, broulik, ngraham.
bruns added a comment.Aug 6 2018, 11:27 PM

This is an alternative to D13869

anthonyfieroni added a subscriber: anthonyfieroni.Aug 12 2018, 6:00 AM
anthonyfieroni added inline comments.
src/solid/devices/backends/udisks2/udisksmanager.cpp
239

Your code seems to work, but i don't get it - why device is added again? How predicate will work on removed device?

bruns added inline comments.Aug 12 2018, 12:50 PM
src/solid/devices/backends/udisks2/udisksmanager.cpp
239

Adding it again allows the Predicate to determine if this device is still relevant.
If e.g. the Predicate is "give me all block devices, independent if it is formatted or not", then the device will be added, if the Predicate only allows formatted devices, the device will be filtered out.
The policy is left to the Predicate and not to the backend.

ngraham added a comment.Aug 13 2018, 8:06 AM

This works for me, FWIW. I've bee running with it for several days. Haven't found any regressions yet with other use cases.

anthonyfieroni added a reviewer: apol.Aug 14 2018, 7:59 AM
anthonyfieroni added a subscriber: apol.

Since solid does not have a maintainer, you can wait for @apol or @broulik or ship it before 5.50 tagging.

broulik accepted this revision.Aug 14 2018, 8:02 AM

Not pretty but if it works.. Thanks!

This revision is now accepted and ready to land.Aug 14 2018, 8:02 AM
Closed by commit R245:8e9fc89918c4: Force reevaluation of Predicates if interfaces are removed (authored by bruns). · Explain WhyAug 14 2018, 1:03 PM
This revision was automatically updated to reflect the committed changes.
bruns marked an inline comment as done.
volkov added a subscriber: volkov.EditedJan 15 2019, 2:14 PM

This change causes a bug: when you eject a mounted CD from the applet, then CD-ROM tray closes immediately after ejecting.

PS. Returning "updateBackend(udi);" fixes the bug.

broulik added a comment.Jan 15 2019, 2:38 PM

So, looks like adding the device causes us to probe it and that prompts UDisks to close the tray for examination? I lack a CD drive, so I cannot try this. Can you perhaps investigate this issue? :)

Easiest could be to check for device type and then in doubt just don't do the remove-add-dance in case it's an optical drive.

volkov added a comment.Jan 15 2019, 2:49 PM

Looks like the bug is caused by removing the line "updateBackend(udi);"

volkov added a comment.Feb 12 2019, 5:01 PM

I guess that there is a short period of time when UDisks daemon is in inconsistent state, i.e. org.freedesktop.UDisks2.Filesystem interface is removed for CD-ROM device, but the device's properties still have Optical=true.
Adding "updateBackend(udi);" before "emit deviceAdded(udi);" gives Udisks time to become consistent.

bruns added a comment.Feb 15 2019, 3:05 AM
In D14661#410848, @volkov wrote:

I guess that there is a short period of time when UDisks daemon is in inconsistent state, i.e. org.freedesktop.UDisks2.Filesystem interface is removed for CD-ROM device, but the device's properties still have Optical=true.
Adding "updateBackend(udi);" before "emit deviceAdded(udi);" gives Udisks time to become consistent.

Its not UDisks which is inconsistent, but the representation in Solid. It has a lot of issues:

  • properties are not per interface in Solid, but per device. This is plain wrong (e.g. the partition and the filesystem '/dev/sda5' may have different size, but Solid merges their "Size" property).
  • Solid does not listen properly to the propertiesChanged event, but insists on polling in a number of places.
  • DBus messages are handled in different places (e.g. the interfaces signals, handled in both DeviceManager and the Device), leading to inconsistent state.

Revision Contents
Changeset List

Diff 39691

View Options

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

Loading...