GNOME Bugzilla – Bug 734305
MTP: implementation does not fully conform to expected GIO copy/move semantics
Last modified: 2014-09-13 21:07:23 UTC
In bug 729628, Ross pointed out that the client facing behaviour is documented, but it's the backend's responsibility to implement that behaviour. https://developer.gnome.org/gio/stable/GFile.html#g-file-copy https://developer.gnome.org/gio/stable/GFile.html#g-file-move Right now, the MTP backend doesn't fully conform with what's documented here - it's correct enough to merge directories, but doesn't handle file->directory and directory->file errors correctly. do_pull, do_push, and probably do_make_directory, require changes.
Hmm, indeed. At a quick glance: do_make_directory needs to return G_IO_ERROR_EXISTS if destination (file or directory) already exists. do_pull and do_push are missing G_IO_ERROR_WOULD_MERGE. Also, if overwrite flag is specified, the (existing) destination ends up being removed before we validate the source (whether it exists at all, whether it would recurse, etc.), which means that the operation can fail but the former destination is removed anyway, which may not be what we want... Also, the error for "ne < 3" checks (the ones that ensure that the path is at storage level or deeper) could maybe be changed to G_IO_ERROR_NOT_SUPPORTED (this is what is for example returned if I set destination to network:/// URI). I can try to come up with patches over weekend, after I take a closer look...
(In reply to comment #1) ... > Also, the error for "ne < 3" checks (the ones that ensure that the path is at > storage level or deeper) could maybe be changed to G_IO_ERROR_NOT_SUPPORTED > (this is what is for example returned if I set destination to network:/// URI). > That's because the network backend doesn't implement writing at all. You could maybe choose G_IO_ERROR_PERMISSION_DENIED but it's up to you. Something else to ensure is that directories which should not be writable must have CAN_WRITE set to FALSE. E.g maybe the top couple of levels? That way Nautilus won't even allow you to try and write something to a top level dir.
Created attachment 282861 [details] [review] Fix the error semantics I can't let Rok to all the work :-) Here's an implementation of correct error semantics as discussed above.
Review of attachment 282861 [details] [review]: Looks good apart from a few nitpicks. Thanks! ::: daemon/gvfsbackendmtp.c @@ +1077,3 @@ g_file_info_set_attribute_boolean (info, G_FILE_ATTRIBUTE_ACCESS_CAN_EXECUTE, TRUE); g_file_info_set_attribute_boolean (info, G_FILE_ATTRIBUTE_ACCESS_CAN_TRASH, FALSE); + g_file_info_set_attribute_boolean (info, G_FILE_ATTRIBUTE_ACCESS_CAN_RENAME, TRUE); I think this should be FALSE. @@ +1599,3 @@ + g_file_query_file_type (local_file, G_FILE_QUERY_INFO_NONE, + G_VFS_JOB (job)->cancellable) == + G_FILE_TYPE_DIRECTORY; You may want to use a single g_file_query_info here rather than doing two separate stat() calls. @@ +1732,3 @@ + + local_file = g_file_new_for_path (local_path); + g_assert(local_file); g_file_new_for_path never fails and neither do glib allocations so I think this is unnecessary. (should have a space after the function name) @@ +1756,3 @@ + g_file_query_file_type (local_file, G_FILE_QUERY_INFO_NONE, + G_VFS_JOB (job)->cancellable) == + G_FILE_TYPE_DIRECTORY; Instead of doing another stat of the local file, rather request G_FILE_ATTRIBUTE_STANDARD_TYPE in the g_file_query_info and then use g_file_info_get_file_type ().
Created attachment 283042 [details] [review] Updated patch Updated to address feedback
Created attachment 286129 [details] [review] mtp: Fix a runtime warning Fix a runtime warning introduced by 32983ccd7e3d ("MTP: Fix error semantics for do_pull/do_push/do_make_directory") which occurs when pulling and the destination does not exist.
Review of attachment 286129 [details] [review]: Thanks for fixing this!
Pushed to master as 50a5d4c1de2997cfebc14d37e49fe54658cf1678. Thanks for the review!