GNOME Bugzilla – Bug 794388
Implement support for move and copy operations
Last modified: 2018-03-27 02:36:25 UTC
Created attachment 369756 [details] [review] WIP: Patch implementing operations I discovered that Android P implements MoveObject and CopyObject (possibly the first time that any device has implemented these ever). So, I've plumbed support through libmtp (still pending review and merge) and verified that they do indeed work. I'm attaching my current work, but it's not quite finished - I think I want to share the file vs directory handling logic explicitly with do_push, or we'll have three copies of it in there.
Created attachment 369814 [details] [review] Implement support for move and copy operations Final form of patch to implement support, with appropriate version and capability checks.
Created attachment 369815 [details] [review] Refactor source/dest validation This patch refactors the logic for deciding what to do with particular source and destination combination so that we only have it in one place.
Review of attachment 369814 [details] [review]: Thanks, looks ok apart from missing support for meson and some nits, but I have not tested it yet... ::: configure.ac @@ +544,3 @@ + AC_DEFINE(HAVE_LIBMTP_1_1_15, 1, [Define to 1 if libmtp 1.1.15 is available]), + [] + ) We are in the middle of transfer from autotools to meson, so meson.build also needs to be updated... Btw it would be nice to remove some old macros and clean the code a bit (in a separate bug of course, not blocker for this). ::: daemon/gvfsbackendmtp.c @@ +2912,3 @@ + LIBMTP_file_t *file = LIBMTP_Get_Filemetadata (device, src_entry->id); + if (file != NULL) { + source_is_dir = file->filetype == LIBMTP_FILETYPE_FOLDER; nitpick: The whole backend uses pretty different coding style than the rest of the project, though GVfs uses different styles, however, it would be nice to use parentheses here for better readability at least: source_is_dir = (file->filetype == LIBMTP_FILETYPE_FOLDER); There are more occurrences in the patch... @@ +2920,3 @@ + + gchar **elements = g_strsplit_set (destination, "/", -1); + unsigned int ne = g_strv_length (elements); Btw it would be nice to introduce some helpers for the path manipulation since this is used on many places and it is not really obvious on the first look what going on (in a separate bug of course, not blocker for this)... @@ +2968,3 @@ + goto exit; + } + /* Source and Dest are files */ nitpick: Would be nice to add empty space before this line... @@ +2987,3 @@ + } + } + /* nitpick: Would be nice to add empty space before this line... @@ +2994,3 @@ + /* Unlike most calls, we must pass 0 for the root directory.*/ + uint32_t parent_id = parent->id == -1 ? 0 : parent->id; + int ret = LIBMTP_Move_Object(device, nitpick: There is missing space before opening parenthesis. @@ +3023,3 @@ + +static void +do_copy (GVfsBackend *backend, It seems that previous comments apply also for do_copy since it is more-or-less just a copy of it, I am just pondering about if the code can't be shared somehow...
(In reply to Ondrej Holy from comment #3) > ...I am just pondering about if the code can't > be shared somehow... Please ignore this comment, the second patch deals with it obviously...
Review of attachment 369815 [details] [review]: Looks ok, just I would like to test before pushing, thus the status "reviewed" now... ::: daemon/gvfsbackendmtp.c @@ +1598,3 @@ + */ +static gboolean +validate_source_and_dest(gboolean dest_exists, I am just pondering about whether this can't be generalized somehow, put in gvfsutils.c and used by other backends also... nitpick: There is missing space before opening parenthesis, there are more occurrences... @@ +1612,3 @@ + G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, + _("Target is a directory")); + goto exit; nitpick: I would rather see "return FALSE", or "ret = FALSE" since there isn't any reason for goto here...
Created attachment 369888 [details] [review] Implement support for move and copy operations v3 Update to reflect feedback. I will do general style cleanup in a separate pass when this is all settled. I only addressed style in the patch itself.
Created attachment 369889 [details] [review] Refactor source/dest validation v2 Update to reflect feedback The method could move to utils if you want. There's no mtp specific dependencies in there that I'm aware of.
Review of attachment 369888 [details] [review]: I have just little worries about the missing progress info. The backend is already very limited by the fact that only one operation may run at one time, however, the long-standing operations (i.e. pull/push) provides the progress info at least. Now we are going to introduce methods which may probably take a far more time given the fact that they allow transfers of the whole directories, but without progress info... What about limiting those operations only to files, all the potential would not be used, but progress would be reported after each file at least (and also other operations may be handled in between). Would it be such a slowdown? Or maybe at least if the locations are on different storages... But anyway we have to be sure that the callback is called at least once, sorry that I haven't realized it earlier, from GIO docs: "It is guaranteed that this callback will be called after all data has been transferred with the total number of bytes copied during the operation." I haven't realized that it is Android P only... so I can't test it.
Review of attachment 369889 [details] [review]: Looks ok, thanks. Let this code in mtp backend for now, but I will add it on my todo...
(In reply to Ondrej Holy from comment #8) > Review of attachment 369888 [details] [review] [review]: > > I have just little worries about the missing progress info. The backend is > already very limited by the fact that only one operation may run at one > time, however, the long-standing > operations (i.e. pull/push) provides the progress info at least. Now we are > going to introduce methods which may probably take a far more time given the > fact that they allow transfers of the whole directories, but without > progress info... What about limiting those operations only to files, all the > potential would not be used, but progress would be reported after each file > at least (and also other operations may be handled in between). Would it be > such a slowdown? Or maybe at least if the locations are on different > storages... It's a fair concern. I can artificially disable directory support for CopyObject - I've tried this (with the progress fix) and you do at least get correct looking progress updates as each file completes. Without it, you get an incomplete progress report that eventually times out. > But anyway we have to be sure that the callback is called at least once, > sorry that I haven't realized it earlier, from GIO docs: > "It is guaranteed that this callback will be called after all data has been > transferred with the total number of bytes copied during the operation." Good catch. I will update the patch shortly so it sends a 100% update at the end.
Created attachment 369981 [details] [review] Implement support for move and copy operations v4 Added 100% progress callback to this patch
Created attachment 369982 [details] [review] Refactor source/dest validation v3 Added restriction of folder copies and comments about motivation
Review of attachment 369981 [details] [review]: Thanks, but still this is not ready. meson.build update is missing. ::: daemon/gvfsbackendmtp.c @@ +2941,3 @@ + if (file != NULL) { + source_is_dir = (file->filetype == LIBMTP_FILETYPE_FOLDER); + filesize = file->filesize; I suppose that this is zero for folders, but the progress callback should report the size of all files inside if I am not mistaken... :-/ @@ +2959,3 @@ + + CacheEntry *entry = get_cache_entry (G_VFS_BACKEND_MTP (backend), destination); + gboolean dest_exists = entry != NULL && entry->id != -1; gboolean dest_exists = (entry != NULL && entry->id != -1);
Review of attachment 369982 [details] [review]: Thanks, I am happy now with ignoring of folders for copy... ::: daemon/gvfsbackendmtp.c @@ +1799,3 @@ g_file_info_get_file_type (local_info) == G_FILE_TYPE_DIRECTORY; + gboolean valid_move = validate_source_and_dest (dest_exists, nitpick: valid_pull? @@ +2091,3 @@ g_file_info_get_file_type (info) == G_FILE_TYPE_DIRECTORY; + gboolean valid_move = validate_source_and_dest (dest_exists, nitpick: valid_push? @@ +2992,3 @@ + dest_is_dir, + source_is_dir, + TRUE, // source_can_be_dir I see that there is a small chance, but why not test if the storage is equal: (parent->storage == src_entry->storage)? @@ +3128,3 @@ + * notify as each file completes. + */ + gboolean valid_move = validate_source_and_dest (dest_exists, nitpick: valid_copy?
Created attachment 370035 [details] [review] Implement support for move and copy operations v5 Whoops. Took the diff from the wrong branch. Addressed comments and fixed warnings I didn't see in the autotools build.
Created attachment 370036 [details] [review] Refactor source/dest validation v4 Updated to address comments
Review of attachment 370035 [details] [review]: I haven't tested it but looks ok, thanks! ::: daemon/gvfsbackendmtp.c @@ +2971,3 @@ + if (file != NULL) { + source_is_dir = (file->filetype == LIBMTP_FILETYPE_FOLDER); + filesize = file->filesize; You did not address my comment about filesize... maybe add at least some comment, that it doesn't work for folders... if it really returns zero for folders.
Review of attachment 370036 [details] [review]: I haven't tested it but looks ok, thanks!
Sorry, I forgot to add a comment. I have put in the following: + /* + * filesize is 0 for directories. However, given that we will only move + * a directory if it's staying on the same storage, then these moves + * will always be fast, and will finish too quickly for the progress + * value to matter. Moves between storages will be decomposed, with + * each file moved separately. + */ Is that fair to you?
Sure, thanks!
Comment on attachment 370036 [details] [review] Refactor source/dest validation v4 >From dd973e4dd2d38bd6ae14d8d3fde462afb403764f Mon Sep 17 00:00:00 2001 >From: Philip Langdale <philipl@overt.org> >Date: Sat, 17 Mar 2018 12:35:54 -0700 >Subject: [PATCH 2/2] mtp: Refactor source/dest file/dir validation logic > >There is a fairly elaborate set of conditions that need to be >respected when copying or moving files. Right now, we've got this >logic duplicated in four different places (push/pull/move/copy), >so it's worth the effort to consolidate that logic into a single >place. > >Along the way, I'm deliberately ignoring the ability of CopyObject >to copy folders. By forcing the client to do file-by-file copies, >we can at least notify when each file completes, to provide slightly >better progress reports. There is a performance impact - I suspect >that a large directory of small files will end up being orders of >magnitude slower, but for a small number of files, it's negligible. > >I have not done this for MoveObject, because that is fast unless >the source and target are on different Storages - and devices >with multiple storages are getting less and less common. > >https://bugzilla.gnome.org/show_bug.cgi?id=794388 >--- > daemon/gvfsbackendmtp.c | 293 ++++++++++++++++++++++-------------------------- > 1 file changed, 135 insertions(+), 158 deletions(-) > >diff --git a/daemon/gvfsbackendmtp.c b/daemon/gvfsbackendmtp.c >index a77c6e13..be094b78 100644 >--- a/daemon/gvfsbackendmtp.c >+++ b/daemon/gvfsbackendmtp.c >@@ -1635,6 +1635,59 @@ mtp_progress (uint64_t const sent, uint64_t const total, > } > > >+/** >+ * Validate whether a given combination of source and destination >+ * are valid for copying/moving. If not valid, set the appropriate >+ * error on the job. >+ */ >+static gboolean >+validate_source_and_dest (gboolean dest_exists, >+ gboolean dest_is_dir, >+ gboolean source_is_dir, >+ gboolean source_can_be_dir, >+ GFileCopyFlags flags, >+ GVfsJob *job) >+{ >+ /* Test all the GIO defined failure conditions */ >+ if (dest_exists) { >+ if (flags & G_FILE_COPY_OVERWRITE) { >+ if (!source_is_dir && dest_is_dir) { >+ g_vfs_job_failed_literal (G_VFS_JOB (job), >+ G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, >+ _("Target is a directory")); >+ return FALSE; >+ } else if (source_is_dir && dest_is_dir) { >+ g_vfs_job_failed_literal (G_VFS_JOB (job), >+ G_IO_ERROR, G_IO_ERROR_WOULD_MERGE, >+ _("Canât merge directories")); >+ return FALSE; >+ } else if (source_is_dir && !dest_is_dir) { >+ g_vfs_job_failed_literal (G_VFS_JOB (job), >+ G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >+ _("Canât recursively copy directory")); >+ return FALSE; >+ } >+ >+ /* Source can overwrite dest as both are files */ >+ return TRUE; >+ } else { >+ g_vfs_job_failed_literal (G_VFS_JOB (job), >+ G_IO_ERROR, G_IO_ERROR_EXISTS, >+ _("Target file already exists")); >+ return FALSE; >+ } >+ } else if (source_is_dir && !source_can_be_dir) { >+ g_vfs_job_failed_literal (G_VFS_JOB (job), >+ G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >+ _("Canât recursively copy directory")); >+ return FALSE; >+ } >+ >+ /* Source is valid and dest doesn't exist */ >+ return TRUE; >+} >+ >+ > static void > do_make_directory (GVfsBackend *backend, > GVfsJobMakeDirectory *job, >@@ -1759,50 +1812,29 @@ do_pull (GVfsBackend *backend, > g_error_free (error); > } > >- /* Test all the GIO defined failure conditions */ >- if (dest_exists) { >- gboolean dest_is_dir = >+ gboolean dest_is_dir = dest_exists && > g_file_info_get_file_type (local_info) == G_FILE_TYPE_DIRECTORY; > >- if (flags & G_FILE_COPY_OVERWRITE) { >- if (!source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, >- _("Target is a directory")); >- goto exit; >- } else if (source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_MERGE, >- _("Canât merge directories")); >- goto exit; >- } else if (source_is_dir && !dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >- _("Canât recursively copy directory")); >- goto exit; >- } >- /* Source and Dest are files */ >- g_debug ("(I) Removing destination.\n"); >- GError *error = NULL; >- gboolean ret = g_file_delete (local_file, >- G_VFS_JOB (job)->cancellable, >- &error); >- if (!ret) { >- g_vfs_job_failed_from_error (G_VFS_JOB (job), error); >- g_error_free (error); >- goto exit; >- } >- } else { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_EXISTS, >- _("Target file already exists")); >+ gboolean valid_pull = validate_source_and_dest (dest_exists, >+ dest_is_dir, >+ source_is_dir, >+ FALSE, // source_can_be_dir >+ flags, >+ G_VFS_JOB (job)); >+ if (!valid_pull) { >+ goto exit; >+ } else if (dest_exists) { >+ /* Source and Dest are files */ >+ g_debug ("(I) Removing destination.\n"); >+ GError *error = NULL; >+ gboolean ret = g_file_delete (local_file, >+ G_VFS_JOB (job)->cancellable, >+ &error); >+ if (!ret) { >+ g_vfs_job_failed_from_error (G_VFS_JOB (job), error); >+ g_error_free (error); > goto exit; > } >- } else if (source_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >- _("Canât recursively copy directory")); >- goto exit; > } > > MtpProgressData mtp_progress_data; >@@ -2075,48 +2107,27 @@ do_push (GVfsBackend *backend, > gboolean source_is_dir = > g_file_info_get_file_type (info) == G_FILE_TYPE_DIRECTORY; > >- /* Test all the GIO defined failure conditions */ >- if (dest_exists) { >- if (flags & G_FILE_COPY_OVERWRITE) { >- if (!source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, >- _("Target is a directory")); >- goto exit; >- } else if (source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_MERGE, >- _("Canât merge directories")); >- goto exit; >- } else if (source_is_dir && !dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >- _("Canât recursively copy directory")); >- goto exit; >- } >- /* Source and Dest are files */ >- g_debug ("(I) Removing destination.\n"); >- int ret = LIBMTP_Delete_Object (device, entry->id); >- if (ret != 0) { >- fail_job (G_VFS_JOB (job), device); >- goto exit; >- } >- g_hash_table_foreach (G_VFS_BACKEND_MTP (backend)->monitors, >- emit_delete_event, >- (char *)destination); >- remove_cache_entry (G_VFS_BACKEND_MTP (backend), >- destination); >- } else { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_EXISTS, >- _("Target file already exists")); >+ gboolean valid_push = validate_source_and_dest (dest_exists, >+ dest_is_dir, >+ source_is_dir, >+ FALSE, // source_can_be_dir >+ flags, >+ G_VFS_JOB (job)); >+ if (!valid_push) { >+ goto exit; >+ } else if (dest_exists) { >+ /* Source and Dest are files */ >+ g_debug ("(I) Removing destination.\n"); >+ int ret = LIBMTP_Delete_Object (device, entry->id); >+ if (ret != 0) { >+ fail_job (G_VFS_JOB (job), device); > goto exit; > } >- } else if (source_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >- _("Canât recursively copy directory")); >- goto exit; >+ g_hash_table_foreach (G_VFS_BACKEND_MTP (backend)->monitors, >+ emit_delete_event, >+ (char *)destination); >+ remove_cache_entry (G_VFS_BACKEND_MTP (backend), >+ destination); > } > > LIBMTP_file_t *mtpfile = LIBMTP_new_file_t (); >@@ -2994,48 +3005,29 @@ do_move (GVfsBackend *backend, > goto exit; > } > >- /* Test all the GIO defined failure conditions */ >- if (dest_exists) { >- if (flags & G_FILE_COPY_OVERWRITE) { >- if (!source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, >- _("Target is a directory")); >- goto exit; >- } else if (source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_MERGE, >- _("Canât merge directories")); >- goto exit; >- } else if (source_is_dir && !dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >- _("Canât recursively copy directory")); >- goto exit; >- } >- /* Source and Dest are files */ >- g_debug ("(I) Removing destination.\n"); >- int ret = LIBMTP_Delete_Object (device, entry->id); >- if (ret != 0) { >- fail_job (G_VFS_JOB (job), device); >- goto exit; >- } >- g_hash_table_foreach (G_VFS_BACKEND_MTP (backend)->monitors, >- emit_delete_event, >- (char *)destination); >- remove_cache_entry (G_VFS_BACKEND_MTP (backend), >- destination); >- } else { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_EXISTS, >- _("Target file already exists")); >+ gboolean source_can_be_dir = (parent->storage == src_entry->storage); >+ gboolean valid_move = validate_source_and_dest (dest_exists, >+ dest_is_dir, >+ source_is_dir, >+ source_can_be_dir, >+ flags, >+ G_VFS_JOB (job)); >+ if (!valid_move) { >+ goto exit; >+ } else if (dest_exists) { >+ /* Source and Dest are files */ >+ g_debug ("(I) Removing destination.\n"); >+ int ret = LIBMTP_Delete_Object (device, entry->id); >+ if (ret != 0) { >+ fail_job (G_VFS_JOB (job), device); > goto exit; > } >+ g_hash_table_foreach (G_VFS_BACKEND_MTP (backend)->monitors, >+ emit_delete_event, >+ (char *)destination); >+ remove_cache_entry (G_VFS_BACKEND_MTP (backend), >+ destination); > } >- /* >- * There is no special case here for the source being a directory. The device >- * is quite happy to move a directory around along with its contents. >- */ > > /* Unlike most calls, we must pass 0 for the root directory.*/ > uint32_t parent_id = (parent->id == -1 ? 0 : parent->id); >@@ -3148,48 +3140,33 @@ do_copy (GVfsBackend *backend, > goto exit; > } > >- /* Test all the GIO defined failure conditions */ >- if (dest_exists) { >- if (flags & G_FILE_COPY_OVERWRITE) { >- if (!source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_IS_DIRECTORY, >- _("Target is a directory")); >- goto exit; >- } else if (source_is_dir && dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_MERGE, >- _("Canât merge directories")); >- goto exit; >- } else if (source_is_dir && !dest_is_dir) { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_WOULD_RECURSE, >- _("Canât recursively copy directory")); >- goto exit; >- } >- /* Source and Dest are files */ >- g_debug ("(I) Removing destination.\n"); >- int ret = LIBMTP_Delete_Object (device, entry->id); >- if (ret != 0) { >- fail_job (G_VFS_JOB (job), device); >- goto exit; >- } >- g_hash_table_foreach (G_VFS_BACKEND_MTP (backend)->monitors, >- emit_delete_event, >- (char *)destination); >- remove_cache_entry (G_VFS_BACKEND_MTP (backend), >- destination); >- } else { >- g_vfs_job_failed_literal (G_VFS_JOB (job), >- G_IO_ERROR, G_IO_ERROR_EXISTS, >- _("Target file already exists")); >+ /* >+ * We ignore the ability to copy whole folders because we get poor progress >+ * updates in this situation. At least with file-by-file copies, we can >+ * notify as each file completes. >+ */ >+ gboolean valid_copy = validate_source_and_dest (dest_exists, >+ dest_is_dir, >+ source_is_dir, >+ FALSE, // source_can_be_dir >+ flags, >+ G_VFS_JOB (job)); >+ if (!valid_copy) { >+ goto exit; >+ } else if (dest_exists) { >+ /* Source and Dest are files */ >+ g_debug ("(I) Removing destination.\n"); >+ int ret = LIBMTP_Delete_Object (device, entry->id); >+ if (ret != 0) { >+ fail_job (G_VFS_JOB (job), device); > goto exit; > } >+ g_hash_table_foreach (G_VFS_BACKEND_MTP (backend)->monitors, >+ emit_delete_event, >+ (char *)destination); >+ remove_cache_entry (G_VFS_BACKEND_MTP (backend), >+ destination); > } >- /* >- * There is no special case here for the source being a directory. The device >- * is quite happy to copy a directory around along with its contents. >- */ > > /* Unlike most calls, we must pass 0 for the root directory.*/ > uint32_t parent_id = (parent->id == -1) ? 0 : parent->id; >-- >2.14.1 >