GNOME Bugzilla – Bug 740057
g_file_move fails if backend can't create backup
Last modified: 2014-11-19 07:57:07 UTC
Created attachment 290619 [details] [review] gfile: g_file_move try fallback if can't create backup Move operation with some backends (i.e. afc, dav, ftp, smb) fails immediately with G_IO_ERROR_CANT_CREATE_BACKUP if G_FILE_COPY_BACKUP is specified. Consequently copy and delete fallback isn't proceeded despite the fact G_FILE_COPY_NO_FALLBACK_FOR_MOVE isn't specified. I don't think it is correct to return G_IO_ERROR_NOT_SUPPORTED from GVfs instead of G_IO_ERROR_CANT_CREATE_BACKUP to force doing fallback. Therefor copy and delete fallback should be tried if G_IO_ERROR_CANT_CREATE_BACKUP. Attaching proposed patch for g_file_move...
Why shouldn't G_IO_ERROR_NOT_SUPPORTED be returned? Otherwise there's no way to distinguish between a genuine backup failure (e.g. a permission denied error) which shouldn't be retried and the case where the backend doesn't support backups in the native move but it can and should still be attempted with the generic code.
(In reply to comment #1) > Why shouldn't G_IO_ERROR_NOT_SUPPORTED be returned? Because e.g. client doesn't know that operation was failed because of creating backup file if G_FILE_COPY_NO_FALLBACK_FOR_MOVE is specified, and therefor the client won't repeat this operation again without G_FILE_COPY_BACKUP flag if he really wants to do this... > Otherwise there's no way to distinguish between a genuine backup failure (e.g. > a permission denied error) which shouldn't be retried and the case where the > backend doesn't support backups in the native move but it can and should still > be attempted with the generic code. But you are right, that generic g_file_move code doesn't know what happens inside the backends and G_IO_ERROR_CANT_CREATE_BACKUP could mean that something was changed on server for example and we shouldn't try the fallback. So afc, dav, ftp, smb should be modified accordingly in this case...
Moving the bug to gvfs...
Created attachment 290629 [details] [review] ftp: try copy and delele fallback if backup couldn't be created
Created attachment 290630 [details] [review] dav: try copy and delele fallback if backup couldn't be created
Created attachment 290631 [details] [review] afc: try copy and delele fallback if backup couldn't be created
Patch for smb is missing, because there is it different case, because the error is returned as a result of unsuccessful smbc_rename operation...
Created attachment 290694 [details] [review] ftp: try copy and delele fallback if backup couldn't be created
Created attachment 290695 [details] [review] dav: try copy and delele fallback if backup couldn't be created
Created attachment 290696 [details] [review] afc: try copy and delele fallback if backup couldn't be created
Review of attachment 290694 [details] [review]: (minor) In the commit message: try copy and delele fallback -> try copy and delete fallback to proceed the fallback. -> to proceed with the fallback. ::: daemon/gvfsbackendftp.c @@ +1336,3 @@ if (flags & G_FILE_COPY_BACKUP) { /* FIXME: implement? */ How about returning G_IO_ERROR_CANT_CREATE_BACKUP if (flags & G_FILE_COPY_NO_FALLBACK_FOR_MOVE) else G_IO_ERROR_NOT_SUPPORTED? That will handle your first concern in comment #2. With this change, the commit message should be updated. @@ +1338,3 @@ /* FIXME: implement? */ + /* Return G_IO_ERROR_NOT_SUPPORTED instead of G_IO_ERROR_CANT_CREATE_BACKUP + * to be proceeded copy and delete fallback */ (minor) This should be "to proceed with copy and delete fallback". @@ +1342,3 @@ G_IO_ERROR, + G_IO_ERROR_NOT_SUPPORTED, + _("Operation not supported (backups not supported)")); With the above suggestion this is an internal error that will not be seen by application code (or users), so it would be better to simply use "Operation not supported" with no translation.
My comments about the first patch apply to the second and third as well.
Created attachment 290916 [details] [review] ftp: try copy and delete fallback if backup couldn't be created
Created attachment 290917 [details] [review] dav: try copy and delete fallback if backup couldn't be created
Created attachment 290918 [details] [review] afc: try copy and deletle fallback if backup couldn't be created
(In reply to comment #11) > Review of attachment 290694 [details] [review]: > How about returning G_IO_ERROR_CANT_CREATE_BACKUP if (flags & > G_FILE_COPY_NO_FALLBACK_FOR_MOVE) else G_IO_ERROR_NOT_SUPPORTED? Thanks for the review! I've realized this solution later also, it would be best probably...
Review of attachment 290916 [details] [review]: Looks good
Review of attachment 290918 [details] [review]: Looks good
Review of attachment 290917 [details] [review]: Looks good
Comment on attachment 290916 [details] [review] ftp: try copy and delete fallback if backup couldn't be created commit d4aa8d9e48d08738e14d4855130a0dcad19bd2eb
Comment on attachment 290917 [details] [review] dav: try copy and delete fallback if backup couldn't be created commit 7c856adf3dc29a51bd07464c4b9a00252ad8bb69
Comment on attachment 290918 [details] [review] afc: try copy and deletle fallback if backup couldn't be created commit 3e73fe863c5a5201f77fe8f6e0bec81cad22288d
Thanks for the reviews!