GNOME Bugzilla – Bug 733465
Issues with multiple storages that have same description
Last modified: 2014-07-23 22:52:23 UTC
I have an Asus TF701T tablet with dock, which has a microSD card slot on the tablet itself and an SD card slot on the dock, and therefore allows use of two SD cards at the same time. This combination does not seem to work correctly with GVFS MTP backend, because the latter refers to the storages by storage description strings obtained from libmtp, and these strings are not unique (i.e., storage description strings for both cards are "SD card"). The issue I am observing is in fact two fold: 1) only one "SD card" storage is listed (for example by Nautilus); this is expected because g_vfs_job_enumerate_add_info() gets called twice for an entry with same name 2) when using Nautilus to open "SD card", its contents are properly listed. However, if I try to copy a file, the file ends up on the other SD card, and the subsequent refresh lists the contents of the other SD card as well. Below are the debug traces that outline the cause of problem #2: First, device is opened in Nautilus: -------------------------------------------------------------------------------- backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x154a530 (GVfsJobQueryInfo) (I) do_query_info (filename = /) send_reply(0x154a530), failed=0 () (I) do_query_info done. backend_dbus_handler org.gtk.vfs.Mount:CreateDirectoryMonitor Queued new job 0x155ee70 (GVfsJobCreateMonitor) (I) create_dir_monitor (/) send_reply(0x155ee70), failed=0 () (I) create_dir_monitor done. backend_dbus_handler org.gtk.vfs.Mount:Enumerate Queued new job 0x155c990 (GVfsJobEnumerate) (I) do_enumerate (filename = /, n_elements = 2) (II) add_cache_entry: /Internal storage: 10001, FFFFFFFF (II) add_cache_entry: /SD card: 30001, FFFFFFFF (II) add_cache_entry: /SD card: 20001, FFFFFFFF send_reply(0x155c990), failed=0 () (I) do_enumerate done. -------------------------------------------------------------------------------- As can be seen, three storages are reported, and two of them have the same description (but different ID). Because add_cache_entry replaces the entries based on the file names (which are constructed from storage description), last call to add_cache_entry() overwrites the entry from the previous one. So, the entries for 10001 and 20001 end up being stored, which show up as "Internal storage" and "SD card", respectively. If we enter "SD card": -------------------------------------------------------------------------------- backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x154a5d0 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff88008f60 (I) found storage 20001 send_reply(0x154a5d0), failed=0 () (I) do_query_info done. backend_dbus_handler org.gtk.vfs.Mount:QueryFilesystemInfo Queued new job 0x155ef00 (GVfsJobQueryFsInfo) (I) do_query_fs_info (filename = /SD card) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff88008f60 send_reply(0x155ef00), failed=0 () (I) do_query_fs_info done. backend_dbus_handler org.gtk.vfs.Mount:CreateDirectoryMonitor Queued new job 0x156f830 (GVfsJobCreateMonitor) (I) create_dir_monitor (/SD card) send_reply(0x156f830), failed=0 () (I) create_dir_monitor done. backend_dbus_handler org.gtk.vfs.Mount:Enumerate Queued new job 0x155ca40 (GVfsJobEnumerate) (I) do_enumerate (filename = /SD card, n_elements = 2) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff88008f60 (III) remove_cache_entry: /SD card/ (III) remove_cache_entry done (II) add_cache_entry: /SD card/.SafeImages: 20001, 4CD3 (II) add_cache_entry: /SD card/.SafeOther: 20001, 4CD4 (II) add_cache_entry: /SD card/.SafeMusic: 20001, 4CD5 (II) add_cache_entry: /SD card/.SafeVideo: 20001, 4CD6 (II) add_cache_entry: /SD card/DCIM: 20001, 4CD7 ... ... ... send_reply(0x155ca40), failed=0 () (I) do_enumerate done. -------------------------------------------------------------------------------- As expected, "/SD card" resolves to storage 20001, and its contents are listed. Now, let us copy a file: -------------------------------------------------------------------------------- backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x154a350 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff88008f60 (I) found storage 20001 send_reply(0x154a350), failed=0 () (I) do_query_info done. backend_dbus_handler org.gtk.vfs.Mount:QueryFilesystemInfo Queued new job 0x156fb00 (GVfsJobQueryFsInfo) (I) do_query_fs_info (filename = /SD card) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff88008f60 send_reply(0x156fb00), failed=0 () (I) do_query_fs_info done. backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x154a2b0 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff88008f60 (I) found storage 20001 send_reply(0x154a2b0), failed=0 () (I) do_query_info done. backend_dbus_handler org.gtk.vfs.Mount:Push Remove Source: false Queued new job 0x7effa000ee20 (GVfsJobPush) (I) do_push (filename = /SD card/test.txt, local_path = /home/rok/test.txt) (III) get_cache_entry: /SD card/test.txt (III) add_cache_entries_for_filename: /SD card/test.txt, 3 (II) add_cache_entry: /SD card: 30001, FFFFFFFF (III) Ignoring query for non-existent file (III) add_cache_entries_for_filename done (III) get_cache_entry done: (nil) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff880045f0 g_vfs_job_progress_callback 12/452 g_vfs_job_progress_callback 440/440 send_reply(0x7effa000ee20), failed=0 () (II) emit_create_event. (III) emit_event_internal (/SD card/test.txt, 3) (III) emit_event_internal done. (II) emit_create_event. (III) emit_event_internal (/SD card/test.txt, 3) (III) emit_event_internal: Event 3 on directory /SD card for /SD card/test.txt (III) emit_event_internal done. (I) do_push done. backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x154a0d0 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card) (III) get_cache_entry: /SD card (III) get_cache_entry done: 0x7eff880045f0 (I) found storage 30001 send_reply(0x154a0d0), failed=0 () (I) do_query_info done. backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x154a490 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card/test.txt) (III) get_cache_entry: /SD card/test.txt (III) add_cache_entries_for_filename: /SD card/test.txt, 3 backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x7eff880030d0 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card/test.txt) backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x7eff88003170 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card/test.txt) (II) add_cache_entry: /SD card: 30001, FFFFFFFF (II) add_cache_entry: /SD card/test.txt: 30001, 6051 (III) add_cache_entries_for_filename done (III) get_cache_entry done: 0x7eff88018c20 send_reply(0x154a490), failed=0 () (I) do_query_info done. (III) get_cache_entry: /SD card/test.txt (III) get_cache_entry done: 0x7eff88018c20 backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x7eff88003210 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card/test.txt) send_reply(0x7eff880030d0), failed=0 () (I) do_query_info done. (III) get_cache_entry: /SD card/test.txt (III) get_cache_entry done: 0x7eff88018c20 send_reply(0x7eff88003170), failed=0 () (I) do_query_info done. (III) get_cache_entry: /SD card/test.txt (III) get_cache_entry done: 0x7eff88018c20 send_reply(0x7eff88003210), failed=0 () (I) do_query_info done. backend_dbus_handler org.gtk.vfs.Mount:QueryInfo Queued new job 0x7eff880032b0 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card/test.txt) (III) get_cache_entry: /SD card/test.txt backend_dbus_handler org.gtk.vfs.Mount:QueryInfo (III) get_cache_entry done: 0x7eff88018c20 Queued new job 0x7eff88003350 (GVfsJobQueryInfo) (I) do_query_info (filename = /SD card/test.txt) send_reply(0x7eff880032b0), failed=0 () (I) do_query_info done. (III) get_cache_entry: /SD card/test.txt (III) get_cache_entry done: 0x7eff88018c20 send_reply(0x7eff88003350), failed=0 () (I) do_query_info done. -------------------------------------------------------------------------------- In add_cache_entries_for_filename(), there is a storage lookup, based on the storage name (which is the first component of the destination file name). The look-up ends up taking first storage with matching description, and updates the storage entry - hence, we now have "SD card" resolving to 30001. The file is therefore copied to storage 30001, and subsequent refresh will display its content instead that of 20001. The proposed patch is split into three parts; first and second part modify the look-up in add_cache_entries_for_filename() so that storage path is first mapped to storage id (using backend's file cache), and then storage id is used to actually look-up the storage in the list returned from libmtp. This takes care of the second part of reported issue. The third patch replaces the use of storage descriptions with storage IDs, and fixes the listing of both SD cards.
Created attachment 281254 [details] [review] Proposed patch - 1st part
Created attachment 281255 [details] [review] Proposed patch - 2nd part
Created attachment 281256 [details] [review] Proposed patch - 3rd part
Ok, I buy the problem and the overall proposed solution, but I'm not convinced by patch 1 - it's possible to have an unmounted device and a pre-existing URI - today, if you try and load the URI in nautilus, it will trigger the mount and then try to directly resolve the URI elements, which will bypass any do_enumerate calls, so I'm pretty sure this part will break that functionality. Please try it out and verify.
Created attachment 281341 [details] [review] Proposed patch Ahhh, I missed the part where do_enumerate calls can get bypassed. Indeed, patches 1 and 2 break the (auto)mounting and direct resolution of URI elements. If referring to storages by their hexadecimal IDs as proposed is acceptable, then the look-up in add_cache_entries_for_filename() can be further simplified: since the first element of path is the ID in string form, it can be converted back to integer and used to find the correct storage. Revised patch attached - part 1 was dropped, while part 2 was modified as described above and merged with part 3.
Ok, I agree this is functional, but I have a concern. One of the original motivations for implementing the filename cache was so that all clients could see consistent names - including those using the fuse bridge, which doesn't respect display-name or copy-name. There are also some file managers (PCManFM?) that fail to use these properties. This means that sometimes you will see the name and sometimes the number. This isn't necessarily fatal (certainly not the extent it was with directory and file names) but it's unfortunate. Would something like 'Internal Storage (2001D)' work as a name? Combining both values so that they remain unique, and then continue to use the normal caching? Or if you're really ambitious, only postfix the storage ID if the names are ambiguous?
Created attachment 281442 [details] [review] Proposed patch, implementing "storage-description (hex-storage-id)" naming scheme Hmmm, if consistency is concern here, maybe the display-name should be changed to the storage id as well? :) But I agree that combination of description and ID looks better - the attached patch implements this scheme. The postfixing-on-demand should be possible as well - at least the scheme where we end up with "Internal Storage", "SD card" and "SD card (20001)" seems to be pretty straight-forward. However, I think it could lead to URI inconsistencies; one example I can think of is: - both cards are present, so the first one becomes "SD card" and the second "SD card (20001)" - second card is bookmarked - some time later, device is reconnected with only second card (the one with storage ID 20001), which will now be named "SD card", so the bookmark is broken But then again, the usb bus location and device number appear to change as device is reconnected, so bookmarking might be a rather moot activity anyway. What do you think? PS: it seems that storage descriptions we get from device depend on device's language setting - would it perhaps be better to construct our own descriptions based on storage->StorageType, and have them localized on host side?
Thanks for continuing to work on this! I do think it's worth doing the conditional postfix - this means that for the majority of users, without these identically named entries, things look exactly the same as before. As for the postfix scheme, I think it should postfix both entries with the same name, so in your example: * Internal Storage * SD card (20001) * SD card (40001) URI inconsistencies is theoretically a problem, but as you say, device numbers increment when you unplug/replug so it's not really possible to bookmark anything and keep URIs around for a long time. I'd rather not create descriptions locally: 1) the device language is presumably meaningful to the user - possibly more so than the system language 2) the names the device provides may be more meaningful than anything we can come up with (eg: I'm sure there's some device out there which has useful names for two SD slots)
Created attachment 281534 [details] [review] On-demand post-fixing Ok, the attached patch implements on-demand post-fixing by expanding the create_storage_name() function introduced in the previous patch. It turns out that if the storage is added/removed while device is mounted (although Android seems to be doing its best to prevent you from moutning/unmounting storages when MTP is active), a refresh changes the listed storages and their names, but the cache entries are not (entirely?) purged. This has a nice side effect that if one card (e.g. 30001) is removed), the old "SD Card (20001)" still resolves into new "SD Card". And if a card is added (e.g., 30001 is added to already-present 20001), the old "SD Card" still resolves into new "SD Card (20001)". Until next remount, of course.
Ok, looks good. The reason why the cache isn't updated on remove is that it doesn't handle STORAGE_REMOVED events. If it did, then I imagine I'd have made it remove the cache entry, like it does for OBJECT_REMOVED. So it's certainly safe. If you feel sufficiently motivated, you could implement a STORAGE_REMOVED handler that emits the delete event to gvfs without clearing the cache.
Review of attachment 281442 [details] [review]: ACK
Review of attachment 281534 [details] [review]: ACK