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 729628 - Cannot move MTP mounted subdir up a level (on same MTP device)
Cannot move MTP mounted subdir up a level (on same MTP device)
Status: RESOLVED FIXED
Product: gvfs
Classification: Core
Component: mtp backend
1.20.x
Other Linux
: Normal normal
: ---
Assigned To: Philip Langdale
gvfs-maint
Depends on:
Blocks:
 
 
Reported: 2014-05-06 08:32 UTC by Rich
Modified: 2014-08-05 17:55 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
screenshot of ubuntu crash reporter (172.36 KB, image/png)
2014-05-06 08:38 UTC, Rich
  Details
Fix - make do_open_for_read() fail on a directory (1.59 KB, patch)
2014-08-04 19:43 UTC, Rok Mandeljc
accepted-commit_now Details | Review

Description Rich 2014-05-06 08:32:59 UTC
I'm using Ubuntu Gnome 14.04 LTS. I have gvfs-backends 1.20.1-1ubuntu1 as part of this.

I plug in my phone. Immediately I get the mount notification. Open the folder.

If I have folders root/foo/bar then if I try to move bar to the same level as foo, it fails, giving:

"libmtp error: Unknown error."

At this point it has created an empty *file* in root/bar which I must then manually delete.
Comment 1 Rich 2014-05-06 08:38:34 UTC
Created attachment 275951 [details]
screenshot of ubuntu crash reporter
Comment 2 Philip Langdale 2014-05-06 14:23:39 UTC
You can't do this. To successfully move a directory around, the device needs to implement the 'move' MTP command, and no device known to man has ever implemented it. Without it, the computer has to copy the file to the PC, delete it, and then copy it back in the new location. This is slow and fragile, and should not be done because it's far too easy to end up in a situation where the file is not on the device and only in some temp location on the PC. Imagine the PC crashing at this point, so the /tmp gets wiped and your file is simply gone.

So, it's actually a feature that the operation fails before it actually deletes anything - the error should be better, of course. And did it actually crash? That shouldn't have happened.

The only safe things you can do are:

1) Manually copy the directory to the PC and then copy it back
2) Move the directory using an on-device file manager

What device do you have?
Comment 3 Rich 2014-05-07 08:34:46 UTC
I have a Nexus 5.

Surely move can be implemented as copy a file > write a file > delete a file. 

Even if copy a file has to be done as copy-to-tmp, copy back to device.

That way no data can possibly be lost and the only requirement is that the fs has enough space for two copies of the one file.

The crash came up a few seconds after this happened so I can't tell if it's related (but the screenshot certainly seems to point to gvfsd-mtp).
Comment 4 Rok Mandeljc 2014-08-04 19:43:55 UTC
Created attachment 282477 [details] [review]
Fix - make do_open_for_read() fail on a directory

GIO actually already has an automatic open-read-write-close ("slow-path") fallback for the cases when files/directories cannot be directly copied/moved. But on MTP volumes, it works only with files, while it fails for directories with the symptoms from the original report.

The problem appears to be that we let do_open_for_read() succeed on directories; this causes the fallback to create the destination as file(!) using do_open_for_write() and attempt to copy the contents via a series of read/write calls.

By making do_open_for_read() fail with G_IO_ERROR_IS_DIRECTORY instead (which is also done, for example, by SMB backend), the directory and its contents get properly moved/copied.
Comment 5 Philip Langdale 2014-08-04 22:42:34 UTC
Review of attachment 282477 [details] [review]:

Good catch. Someone really needs to document these error messages - they define a pretty nuanced interface between the backend and GIO/Nautilus.
Comment 6 Ross Lagerwall 2014-08-05 06:08:36 UTC
(In reply to comment #5)
> Review of attachment 282477 [details] [review]:
> 
> Good catch. Someone really needs to document these error messages - they define
> a pretty nuanced interface between the backend and GIO/Nautilus.

For what it's worth, this is explained in the collective documentation of g_file_read, g_file_copy and g_file_move. g_file_read says that it must return G_IO_ERROR_IS_DIRECTORY if it is a directory, and g_file_copy and g_file_move explain in what circumstances other errors are returned (e.g. G_IO_ERROR_WOULD_MERGE and G_IO_ERROR_WOULD_RECURSE). But yes I agree that there's low visibility on this. Take a look a bug 599354 for a similar example where opening a directory caused problems.

Thanks for fixing this.

https://developer.gnome.org/gio/stable/GFile.html#g-file-read
https://developer.gnome.org/gio/stable/GFile.html#g-file-copy
https://developer.gnome.org/gio/stable/GFile.html#g-file-move
Comment 7 Philip Langdale 2014-08-05 17:14:14 UTC
For me, the confusion with those docs was they describe the client facing behaviour, and it is physically possible for GIO to implement that behaviour independent of the backend (ie: enough file metadata is available for the common layer to work out when those error codes are needed), rather than blinding forwarding calls to the backend, which then has to realise it needs to behave in this way. This is why directory merging didn't work with the mtp backend for a long time too :-)
Comment 8 Ross Lagerwall 2014-08-05 17:55:21 UTC
Well I suppose the problem with doing it at the higher level is that it is often not efficient (e.g. requires extra query_info calls to determine file types) and can be racy. Usuaully one needs to read the source code of the local file implementation and the smb and sftp implementations to get an idea of what to do...