GNOME Bugzilla – Bug 770549
Better handling of mount errors
Last modified: 2016-09-12 12:34:17 UTC
Currently we have a few open TODO items in the day backend when mounting fails: we leak some things, and have a duplicated error message condition. Also we don't handle cancellation of the auth dialog.
Christian has already proposed fixes for those issues on his github: https://github.com/gicmo/gvfs/commits/davmnterr
Created attachment 334363 [details] [review] Patch to change mount error handling
Review of attachment 334363 [details] [review]: Thanks for the patch! Please create the patches using the git format-patch next time... Add the link for this bug in the bug description please. ::: c/daemon/gvfsbackenddav.c @@ +1940,3 @@ if (is_collection == FALSE) break; + Please do not change whitespace independently. @@ +1997,3 @@ + + if (error_code == G_IO_ERROR_CANCELLED) + error_code = G_IO_ERROR_FAILED_HANDLED; It is ok, but please add a comment in the commit description describing why we need it and that it is same as in another backends... @@ +2005,3 @@ else + { + /* This is means, we have a valid DAV header, PROPFIND worked, "This is means" should be "This means" probably... @@ +2010,3 @@ + G_IO_ERROR, G_IO_ERROR_FAILED, + _("Could not find an enclosing directory")); + } I am convinced that this else-statement should be before the previous elseif-statement: $ gvfs-mount dav://notexisting/ Error mounting location: HTTP Error: Error resolving 'notexisting': No address associated with hostname --> OK $ gvfs-mount dav://example.com/ Error mounting location: Not a WebDAV enabled share --> OK $ gvfs-mount dav://www.ajaxfilebrowser.com/notexisting Error mounting location: HTTP Error: Not Found --> I think we should see "Could not find an enclosing directory" in this case $ gvfs-mount http://www.ajaxfilebrowser.com/User6e06bc4 --> OK @@ +2014,3 @@ + g_object_unref (msg_opts); + g_object_unref (msg_stat); + soup_uri_free (mount_base); Be careful, do_mount contains the following line and g_vfs_backend_http_finalize calls soup_uri_free on backend->mount_base, which causes segfaults in some cases (e.g. $ gvfs-mount dav://www.ajaxfilebrowser.com/notexisting)... G_VFS_BACKEND_HTTP (backend)->mount_base = mount_base;
Created attachment 334365 [details] [review] dav: Try also root when looking for the shortest prefix Dav backend always tries to mount WebDAV share with the shortest prefix. Unfortunately, it never tries the root when looking for it excepting the case, when the root is already specified. The patch just fixes the already implemented behavioral, though I am not sure this is the best idea, because a parent dir might be completely different webdav share, so it might cause problems... E.g. http://www.ajaxfilebrowser.com/User6e06bc4 http://www.ajaxfilebrowser.com/ mount doesn't contain folder User6e06bc4...
> $ gvfs-mount dav://www.ajaxfilebrowser.com/notexisting > Error mounting location: HTTP Error: Not Found --> I think we should see > "Could not find an enclosing directory" in this case This works as intended. All HTTP errors (except a 405 from OPTIONS) were intended to be reported in the second if case. Only the case where we have a DAV header, PROPFIND worked and we are just not in a dav::collection (very academical case indeed) were supposed to be handled in the last case. But maybe the whole "HTTP Error: XXXX" is confusing (and most likely irrelevant) and we can just have two conditions a) we detect it is not a WebDAV share (OPTIONS works but not no DAV header) and everything else ("Could not find an enclosing Directory")?
Sorry, I made a typo, http://www.ajaxfilebrowser.com/User6e06bc4 should be with dav:// prefix obviously...
Created attachment 334372 [details] [review] Better error handling during mount
Created attachment 334373 [details] [review] Cleanup in case of error during mount
Created attachment 334374 [details] [review] Return proper error code in case of cancellation during mount
Created attachment 334375 [details] [review] Better error message for a corner case during mount
Review of attachment 334372 [details] [review]: Looks good, thanks! Please announce the string changes to both gnome-i18n@ and gnome-doc-list@, see for details: https://wiki.gnome.org/ThreePointTwentyone#d2016-08-15
Review of attachment 334373 [details] [review]: Looks good, thanks!
Review of attachment 334374 [details] [review]: Looks good, thanks!
Review of attachment 334375 [details] [review]: Looks good, thanks!
(In reply to Ondrej Holy from comment #4) > The patch just fixes the already implemented behavioral, though I am not > sure this is the best idea, because a parent dir might be completely > different webdav share, so it might cause problems... Yeah, this is already the case if /foo/bar is a different webdav share then /foo. Not sure we can do much about it. We do detect changes in realm and then don't try to reuse the credentials, so if the realm changes we would stop at /foo.
Review of attachment 334365 [details] [review]: Good catch! Looks good to me!
Comment on attachment 334372 [details] [review] Better error handling during mount commit 99223f68c7d777dfca44d5d0fd8bbe0bb7640923
Comment on attachment 334373 [details] [review] Cleanup in case of error during mount commit 3c147616d5fbdf2aa22ef21aae9d38cf55675c6c
Comment on attachment 334374 [details] [review] Return proper error code in case of cancellation during mount commit 5a560b96d1e72efe104c3145893f0207436e3156
Comment on attachment 334375 [details] [review] Better error message for a corner case during mount commit b0e0f847a374d85547fe2ebbbe4b4c6e3c52244c
Comment on attachment 334365 [details] [review] dav: Try also root when looking for the shortest prefix I've not pushed this one, because it breaks the test suite and I don't have time to fix it right now...
Created attachment 334434 [details] [review] dav: Try also root when looking for the shortest prefix Ah, the test suite failures were caused by backend crashes due to NULL pointer passed in strcmp, because last_good_path may be NULL in case of https...
Review of attachment 334434 [details] [review]: Looks good!
Comment on attachment 334434 [details] [review] dav: Try also root when looking for the shortest prefix I marked the patch as "commit_after_freeze" initially, but the hard code freeze begins today, so I've pushed it as commit c4d92cf...