GNOME Bugzilla – Bug 340228
gnome_vfs_open() accept to open a directory for non-file: URI
Last modified: 2018-08-17 13:49:58 UTC
That bug has been described on https://launchpad.net/distros/ubuntu/+source/totem/+bug/41437 "Totem doesn't handle sftp directory properly ... I use gnome-vfs to access some remote files on a ssh server. I can drag and drop files into totem playlist, but droping a directory doesn't work. I get the following message : " Totem could not play 'sftp://XXX@XXX:22/mnt/musique/'. could not read from resource "" totem-pl-parser does that: " /* Open the file. */ result = gnome_vfs_open (&handle, uri, GNOME_VFS_OPEN_READ); if (result != GNOME_VFS_OK) { g_print("result: %d\n", result); if (result == GNOME_VFS_ERROR_IS_DIRECTORY) return DIR_MIME_TYPE; return NULL; }" the gnome_vfs_open accepts to open a vfs directory but not on a file: URI
I took a look at this bug. In OpenSSH, the sftp command SSH2_FXP_OPEN has no special case for treating directories. It just makes an "open" call, and return a handle. So, the only way for GnomeVFSMethodOpenFunc method to handle that case is to make a call at GnomeVFSMethodGetFileInfoFromHandleFunc, and check the "type" field. It's not a good thing from a latency point of view, as it makes one more round-trip, but it's the only way I found for correct behaviour.
Created attachment 67586 [details] [review] patch made from head Implement my previous comment.
Thanks for your efforts! Unfortunately, you forgot to close the valid handle when returning with an error. The handler is not expected to close it when we return with an error. Otherwise, I think the patch is correct.
Are you willing to rework the patch according to my suggestion? :)
Of course I will rework my patch. Thanks for the revue. I'm not sure to understand your comment : >Unfortunately, you forgot to close the valid handle when returning with an > error. The handler is not expected to close it when we return with an error. Which handle are you talking about ? do you mean method_handle should be set to NULL ? And another question : do I need to unref the connection as it's done a few line below ?
> Which handle are you talking about ? do_open() is an equivalent to the UNIX fopen(). When iobuf_read_handle succeeds, you have a handle pointing to an open file (similar to fopen()). Locally, you fclose() it again. Because you flag failure to the caller, he doesn't expect to receive a file handle at all. Thus, you should do the same as in the iobuf failure case, but also close the handle: * call do_close (method, method_handle, NULL); note that cancellation is NULL because cancellation isn't really desirable here (it might leak the handle), and we don't unref the connection in do_open because do_close does that already. * set the method handle to NULL * return GNOME_VFS_ERROR_IS_DIRECTORY; I also think that you shouldn't return res, but rather put the whole checks before return GNOME_VFS_OK; so that failure when getting the file info aren't passed to the caller (where he already has a valid file handle!).
(In reply to comment #6) > * call do_close (method, method_handle, NULL); I think I understand your advice, but what should I do when do_close fail ? I'm a bit suprised that do_close doesn't always succeed. > I also think that you shouldn't return res, but rather put the whole checks > before > > return GNOME_VFS_OK; > > so that failure when getting the file info aren't passed to the caller (where > he already has a valid file handle!). Do you mean the lack of result from do_get_info_from_handle shouldn't raise an error from do_open ?
> I think I understand your advice, but what should I do when do_close fail ? Ignore it and return GNOME_VFS_ERROR_IS_DIRECTORY. The worst thing that could happen is that we leak a handle on the server until the end of the session, so no data loss is involved. > Do you mean the lack of result from do_get_info_from_handle shouldn't raise an error from do_open ? Exactly. Oh, and I forgot to mention that if (res == GNOME_VFS_OK && file_info.type == GNOME_VFS_FILE_TYPE_DIRECTORY) should rather be if (res == GNOME_VFS_OK && (file_info.valid_fields & GNOME_VFS_FILE_INFO_FIELDS_TYPE != 0) && file_info.type == GNOME_VFS_FILE_TYPE_DIRECTORY) Thanks in advance for your efforts :).
Actually this: ---- SNIP ----- GnomeVFSFileInfo file_info; GnomeVFSResult res; res = do_get_file_info_from_handle (method, *method_handle, &file_info, GNOME_VFS_FILE_INFO_DEFAULT, context); if (res == GNOME_VFS_OK && file_info.type == GNOME_VFS_FILE_TYPE_DIRECTORY) { ---- SNAP ----- is wrong also. GnomeVfsFileInfo has internal refcounting and stuff and needs to be initialized. That is the reason why you should rather do a: GnomeVFSFileInfo *file_info; file_info = gnome_vfs_file_info_new (); [...] gnome_vfs_file_info_unref (file_info); ;-D
(In reply to comment #9) > GnomeVfsFileInfo has internal refcounting and stuff and needs to be > initialized. That is the reason why you should rather do a: > > GnomeVFSFileInfo *file_info; > file_info = gnome_vfs_file_info_new (); > [...] > gnome_vfs_file_info_unref (file_info); If I'm not mistaken (ie I haven't checked with the source), GnomeVFSFileInfo file_info = {0, }; might do the trick as well.
Teuf, yep, you are right, but I wanna discourage people from doing so. GnomeVFSFileInfo is a pseudo object and should be treated as such. ;-D
Created attachment 69195 [details] [review] second try Thanks for all the comments and advices, I hope it makes my contribution better.
can someone take a look at my second try patch ?
If we really wanna do this expensive stat commmand on each open we should do it before we open the file. I don't see any reason to do it the other way around. Is there?
Newer SFTP protocol versions [1] force the server to return SSH_FX_FILE_IS_A_DIRECTORY 24 when SSH_FXP_OPEN 3 is applied to a directory. We may want to wrap this status code to GNOME_VFS_ERROR_IS_DIRECTORY and figure out what version the server uses, so that we don't have to apply this hack when it supports this stuff. [1] since version 6, i.e. http://tools.ietf.org/wg/secsh/draft-ietf-secsh-filexfer/draft-ietf-secsh-filexfer-07.txt
gnome-vfs got deprecated in 2008. gnome-vfs is not under active development anymore and had its last code changes in 2011. Its codebase has been archived: https://gitlab.gnome.org/Archive/gnome-vfs/commits/master gio (in glib) and gvfs are its successors. See https://developer.gnome.org/gio/stable/ch33.html and https://people.gnome.org/~gicmo/gio-migration-guide/ for porting info. Closing this report as WONTFIX as part of Bugzilla Housekeeping to reflect reality. Feel free to open a task in GNOME Gitlab if the issue described in this task still applies to a recent + supported version of glib/gio/gvfs. Thanks!