Mostly mounted directory are empty when they are umounted
CCBUG: 394348
Details
Details
Diff Detail
Diff Detail
- Repository
- R245 Solid
- Lint
Lint Skipped - Unit
Unit Tests Skipped
| src/solid/devices/backends/udisks2/udisksmanager.cpp | ||
|---|---|---|
| 220 | This code needs some restructuring, and with the additional conditions, some comments ...
| |
Comment Actions
- Return early when path is empty
- Don't check for empty interfaces
| src/solid/devices/backends/udisks2/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. | |
Comment Actions
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
Comment Actions
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.
Comment Actions
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.
Comment Actions
So about docs when interface is added it will notify again for mounting, i think that's the idea behind interfaces.
Comment Actions
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.
Comment Actions
I'm aware that does not have an idea, what is the purpose of Predicate. What should i search in Solid::Predicate::matches?
Comment Actions
Predicates are Solid's query language for searching for only the devices you are interested in.
Comment Actions
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
Comment Actions
Mounting by cdemu also works as expected, not without patch as is described in bug report.
| src/solid/devices/backends/udisks2/udisksmanager.cpp | ||
|---|---|---|
| 220 | This does not answer my question. What I need from you is:
so the exact case where the patched code triggers, but the old does not. | |
| src/solid/devices/backends/udisks2/udisksmanager.cpp | ||
|---|---|---|
| 220 | Old code is complete broken, about me, it does not check any interface except that it's empty. | |
| src/solid/devices/backends/udisks2/udisksmanager.cpp | ||
|---|---|---|
| 220 | In which cases what you to know the interfaces? | |
| src/solid/devices/backends/udisks2/udisksmanager.cpp | ||
|---|---|---|
| 220 | Can you please just answer the question? | |
Comment Actions
mount mini.iso /mnt/mini/
umount /mnt/mini
Device(udi).interfaces() ("org.freedesktop.UDisks2.Loop", "org.freedesktop.UDisks2.Block")
interfaces ("org.freedesktop.UDisks2.Filesystem")
Comment Actions
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 MDTI 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.
Comment Actions
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.
| src/solid/devices/backends/udisks2/udisksmanager.cpp | ||
|---|---|---|
| 222 ↗ | (On Diff #37108) | 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. |
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
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, ...
Comment Actions
So write an example with commands, mounting, umounting flash drive, which is a filesystem, works.