GNOME Bugzilla – Bug 766294
Different GFiles for same FUSE path with g_file_new_for_commandline_arg
Last modified: 2016-05-20 18:13:55 UTC
result in two identical GFiles for WebDAV shares if the share is mounted: Simple test code: int main(int argc, char *argv[]) { char path[] = "/run/user/1000/gvfs/dav:host=shotwell-project.org,ssl=false,prefix=%2Fdav/Capture.PNG"; GFile *f = g_file_new_for_commandline_arg (path); GFile *g = g_file_new_for_commandline_arg (path); g_print ("f: %s (local: %s) %s\ng: %s (local: %s) %s \n\n", g_file_get_path (f), G_OBJECT_TYPE_NAME (f), g_file_get_uri (f), g_file_get_path (g), G_OBJECT_TYPE_NAME (g), g_file_get_uri (g)); g_assert(g_file_equal (f, g)); return 0; } Output: /run/user/1000/gvfs/dav:host=shotwell-project.org,ssl=false,prefix=%2Fdav/Capture.PNG (local: GDaemonFile) dav://shotwell-project.org/dav/Capture.PNG (null) (local: GDaemonFile) dav://shotwell-project.org/Capture.PNG ** ERROR:foo.c:13:main: assertion failed: (g_file_equal (f, g)) Aborted (core dumped)
Created attachment 327667 [details] [review] wip Signed-off-by: Jens Georg <mail@jensge.org>
Comment on attachment 327667 [details] [review] wip Sorry, sent from wrong git repository
Created attachment 327668 [details] [review] Add mount prefix in cached fuse paths If a fuse path is encountered the first time, the fuse path is re-created taking the mount prefix into account. When the file is created for the path the second time from the cached mount info, the mount prefix is silently ignored, causing the new file not to be equal to the old one (it is even invalid): This change uses the mount prefix if available to construct the path just as the code does on first use.
Review of attachment 327668 [details] [review]: Thanks for the patch! It looks basically good, however similar code to convert GMountInfo to mount_path is already in _g_daemon_vfs_get_mount_info_by_fuse_sync. So it would be good to add new function with this functionality and use it on the both places, or just return info from lookup_mount_info_by_fuse_path_in_cache (not mount_path) and continue processing in _g_daemon_vfs_get_mount_info_by_fuse_sync.
Ah, right, good point.
Created attachment 327731 [details] [review] Always add mount prefix in cached fuse paths If a fuse path is encountered the first time, the fuse path is re-created taking the mount prefix into account. When the file is created for the path the second time from the cached mount info, the mount prefix is silently ignored, causing the new file not to be equal to the old one (it is even invalid): This change just looks up the mount info in the cache and then continues constructing the path as it would without having the info cached.
I did the "remove code" option. It seemed cleaner to me.
Review of attachment 327731 [details] [review]: Thanks for the updated patch! ::: client/gdaemonvfs.c @@ +717,3 @@ if (mount_info->fuse_mountpoint != NULL && g_str_has_prefix (fuse_path, mount_info->fuse_mountpoint)) + break; This doesn't work, it always returns NULL, because info variable is not set. Also you have to check fuse_path[len] to be sure it is same mount... There should be something like: { int len = strlen (mount_info->fuse_mountpoint); if (fuse_path[len] == 0 || fuse_path[len] == '/') { info = g_mount_info_ref (mount_info); break; } } @@ +967,3 @@ GVariant *iter_mount; + info = lookup_mount_info_by_fuse_path_in_cache (fuse_path); Be careful g_object_unref (proxy) at the end of _g_daemon_vfs_get_mount_info_by_fuse_sync may be called with uninitialized proxy if the info is returned from cache. You should unref the proxy in this following statement, or unref it later conditionally...
Review of attachment 327731 [details] [review]: ::: client/gdaemonvfs.c @@ +717,3 @@ if (mount_info->fuse_mountpoint != NULL && g_str_has_prefix (fuse_path, mount_info->fuse_mountpoint)) + break; Dread, didn't see this. This would be similar code to path_has_prefix in common/gmountspec.c - would it make sense to make this somewhat reusable? @@ +967,3 @@ GVariant *iter_mount; + info = lookup_mount_info_by_fuse_path_in_cache (fuse_path); Ah, good point
(In reply to Jens Georg from comment #9) > Review of attachment 327731 [details] [review] [review]: > > ::: client/gdaemonvfs.c > @@ +717,3 @@ > if (mount_info->fuse_mountpoint != NULL && > g_str_has_prefix (fuse_path, mount_info->fuse_mountpoint)) > + break; > > Dread, didn't see this. This would be similar code to path_has_prefix in > common/gmountspec.c - would it make sense to make this somewhat reusable? Indeed, it would be nice to use one common function, it could help to avoid such bugs, however not sure how do you want to reuse it there... It is ok for me if you just fix the patch as per my previous review :-)
> > Dread, didn't see this. This would be similar code to path_has_prefix in > > common/gmountspec.c - would it make sense to make this somewhat reusable? > > Indeed, it would be nice to use one common function, it could help to avoid > such bugs, however not sure how do you want to reuse it there... It is ok > for me if you just fix the patch as per my previous review :-) Just noticed that wile trying to find out what the difference between the two cache lookup functions was. Sorry, I was busy during the the week, I'll try to update the patch tonight
Created attachment 328218 [details] [review] Always add mount prefix in cached fuse paths If a fuse path is encountered the first time, the fuse path is re-created taking the mount prefix into account. When the file is created for the path the second time from the cached mount info, the mount prefix is silently ignored, causing the new file not to be equal to the old one (it is even invalid): This change just looks up the mount info in the cache and then continues constructing the path as it would without having the info cached.
Review of attachment 328218 [details] [review]: Thanks, looks good to me! Please push to master and stable. Usually, I complain about unnecessary whitespace changes, however it is also true that I want to get rid of the tabs...
Attachment 328218 [details] pushed as 7196c5c - Always add mount prefix in cached fuse paths