After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 789491 - mtp volume is not removed when unplugging
mtp volume is not removed when unplugging
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: mtp backend
git master
Other Linux
: Normal normal
: ---
Assigned To: Philip Langdale
gvfs-maint
: 790555 (view as bug list)
Depends on:
Blocks:
 
 
Reported: 2017-10-25 18:32 UTC by Ondrej Holy
Modified: 2018-04-03 15:03 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
udevadm monitor -u -p log (3.24 KB, text/plain)
2017-10-25 18:32 UTC, Ondrej Holy
  Details
mtp: Do not check ID_MTP_DEVICE when removing (1.66 KB, patch)
2017-10-25 18:33 UTC, Ondrej Holy
none Details | Review
mtp: Fix volume removal with current udev behavior (1.67 KB, patch)
2017-10-26 06:53 UTC, Ondrej Holy
committed Details | Review
monitor: Remove device file checks (3.03 KB, patch)
2017-10-31 16:47 UTC, Ondrej Holy
committed Details | Review

Description Ondrej Holy 2017-10-25 18:32:53 UTC
Created attachment 362286 [details]
udevadm monitor -u -p log

Volume for my Nexus 5x is not removed correctly and thus the number of volumes increases when plugging... 

gio mount -l
Volume(0): Nexus 5X
  Type: GProxyVolume (GProxyVolumeMonitorMTP)
Volume(1): Nexus 5X
  Type: GProxyVolume (GProxyVolumeMonitorMTP)
Volume(2): Nexus 5X
  Type: GProxyVolume (GProxyVolumeMonitorMTP)
...

The problem is that remove uevent is discarded because ID_MTP_DEVICE is not set:
https://git.gnome.org/browse/gvfs/tree/monitor/mtp/gmtpvolumemonitor.c#n212

$ G_MESSAGES_DEBUG=all /opt/gnome/libexec/gvfs-mtp-volume-monitor 
...
(process:8527): GVFS-MTP-DEBUG: on_uevent: action=remove, device=(null)
(process:8527): GVFS-MTP-DEBUG: on_uevent: discarding, not ID_MTP
(process:8527): GVFS-MTP-DEBUG: on_uevent: action=unbind, device=/dev/bus/usb/002/095
(process:8527): GVFS-MTP-DEBUG: on_uevent: discarding, not ID_MTP
(process:8527): GVFS-MTP-DEBUG: on_uevent: action=remove, device=/dev/bus/usb/002/095
(process:8527): GVFS-MTP-DEBUG: on_uevent: discarding, not ID_MTP

Not sure why this is happening and whether it applies also to other devices, or not...

Philip, do you have any idea what is wrong?
Comment 1 Ondrej Holy 2017-10-25 18:33:33 UTC
Created attachment 362287 [details] [review]
mtp: Do not check ID_MTP_DEVICE when removing

UDev events for devices without ID_MTP_DEVICE property are ignored.
Although ID_MTP_DEVICE seems don't have to be set for device when removing
and thus the volume is not removed. Let's ignore ID_MTP_DEVICE when removing.
Comment 2 Philip Langdale 2017-10-25 20:00:41 UTC
Review of attachment 362287 [details] [review]:

Looks fine; it makes sense to consider a remove event on a known device to be valid, regardless of the device properties.

I assume this is due to some recent udev or device database change.
Comment 3 Ondrej Holy 2017-10-26 06:53:49 UTC
Created attachment 362310 [details] [review]
mtp: Fix volume removal with current udev behavior

Thanks for review! Updated commit title...
Comment 4 Ondrej Holy 2017-10-26 06:54:32 UTC
Attachment 362310 [details] pushed as 0c7f803 - mtp: Fix volume removal with current udev behavior
Comment 5 Ondrej Holy 2017-10-26 06:56:21 UTC
And pushed to gnome-3-26 as well for sure...
Comment 6 Philip Langdale 2017-10-26 14:44:00 UTC
Thanks. Note that the logic here was shamelessly copied from the gphoto2 volume monitor, and it continues to do a property check on events (looks for ID_GPHOTO2 property). You might want to verify what's going on there as well.
Comment 7 Ondrej Holy 2017-10-27 09:25:37 UTC
Thanks for the notice, the same happens also for gphoto2 volume monitor indeed...
Comment 8 Ondrej Holy 2017-10-27 09:27:20 UTC
So I have pushed the same fix for gphoto2 also as commit 9f8561d.
Comment 9 Ondrej Holy 2017-10-31 12:35:39 UTC
Review of attachment 362310 [details] [review]:

::: monitor/mtp/gmtpvolumemonitor.c
@@ +211,3 @@
+  if (g_strcmp0 (action, "add") == 0 && g_udev_device_has_property (device, "ID_MTP_DEVICE"))
+    gudev_add_device (monitor, device, TRUE);
+  else if (g_strcmp0 (action, "remove") == 0 && g_udev_device_get_device_file (device) != NULL)

I've just realized that I used g_udev_device_get_device_file () check here, because it seems it can be NULL in some cases and I thought that it is used for comparison in gudev_remove_device, however, g_udev_device_get_sysfs_path is used there... should I remove it? Philip?
Comment 10 Philip Langdale 2017-10-31 14:30:35 UTC
Honestly, I'd be fine with no check there - after all, if you get a remove event on a device you already added, you should remove it right? Regardless of anything else. But if you think some check is required, switching would make sense.
Comment 11 Ondrej Holy 2017-10-31 16:47:12 UTC
Created attachment 362647 [details] [review]
monitor: Remove device file checks

The device file checks were added by mistake and it is not clear whether
it can't have some side-effect. Let's remove them and use g_strcmp0 for
sysfs path comparison for sure...
Comment 12 Ondrej Holy 2017-10-31 16:47:58 UTC
Comment on attachment 362647 [details] [review]
monitor: Remove device file checks

Attachment 362647 [details] pushed as 5db1651 - monitor: Remove device file checks
Comment 13 Philip Langdale 2017-11-19 15:07:58 UTC
*** Bug 790555 has been marked as a duplicate of this bug. ***
Comment 14 Mark 2018-04-01 22:37:37 UTC
Hello!

I'm also running into this issue in Debian Stretch (Gnome 3.22) and suspect there are many other users since Debian is so widely used. 

Would it be possible to get this fix pushed into 3.22 so that (hopefully soon), it would flow down through the Debian repository? Pretty please?

If not, could you suggest a hack to get this fix without upgrading all of Gnome to a newer rev?

Thanks much!
Mark
Comment 15 Ondrej Holy 2018-04-03 14:27:47 UTC
The fix is already part of gnome-3-28 and gnome-3-26 releases. I am going to push into older branches also, but it is up to downstream packagers to use this commits... I am not about making another release in those old branches. So you should report this to Debian.
Comment 16 Mark 2018-04-03 15:03:01 UTC
Thanks very much! I'll contact Debian to ask them to incorporate your fix.