GNOME Bugzilla – Bug 549207
Use openssh extensions when it makes sense
Last modified: 2013-11-30 13:58:27 UTC
I don't know it it's interesting, but I'd guess it could be: openssh is extending the ssh protocol a bit, see http://www.openbsd.org/cgi-bin/cvsweb/~checkout~/src/usr.bin/ssh/PROTOCOL posix-rename@openssh.com and statvfs@openssh.com look potentially relevant (note that "Clients MUST check the version number before attempting to use the extension"). Also, totally irrelevant to this bug, I wonder about "6. sftp: Reversal of arguments to SSH_FXP_SYMLINK". Are all ssh servers implementing the same reverse order? Or are some following the protocol draft?
Just a note: We failed to handle the SSH_FXP_SYMLINK arg reversal before, but we now do that. I don't know if other servers do that or not, would be interesting with some feedback on that.
Created attachment 218965 [details] [review] sftp: support statvfs openssh extension Add support for the openssh statvfs extension to sftp backend.
Created attachment 257899 [details] [review] sftp: Implement try_query_fs_info using the OpenSSH statvfs extension Use the statvfs command from OpenSSH to fill in the filesystem size, free and used attributes as well as whether the filesystem is readonly or not. The extension is defined at http://api.libssh.org/rfc/PROTOCOL. The extension is signified by the name "statvfs@openssh.com" with a version of "2". Based on a patch Alban Browaeys.
The attached patch is based on Alban's patch with a few changes: Don't actually use the statvfs structure since some systems may not have it. Actually make use of the extension's existence check by returning FALSE if the extension is not available. Check for version "2", not version "1" of the spec as required by the protocol. Simplify things by using a single enumeration for extensions, SFTPServerExtensions, rather than a per server type enumeration. Simplify things by not using a commands array. Simplify things by placing the parse_attributes function inline since it is used only once. Only set attributes if they are requested. Also calculate the FILESYSTEM_USED attribute. Use frsize instead of bsize in calculations. If bfree and bavail are both 0, don't set any sizes as done in glib with local files. Tested against a local ssh server and confirmed that it gave the same results as running gvfs-info on the local file.
Review of attachment 257899 [details] [review]: Thanks for your patch, just several notes: ::: daemon/gvfsbackendsftp.c @@ +1658,3 @@ + if ((strcmp(extension_name, "statvfs@openssh.com") == 0) + && (strcmp(extension_data, "2") == 0)) + op_backend->server_extensions |= SFTP_EXT_OPENSSH_STATVFS; Wouldn't be better to do that similar as ftp backend does? op_backend->server_extensions |= 1 << SFTP_EXT_OPENSSH_STATVFS; @@ +4132,3 @@ + g_data_input_stream_read_uint64(reply, NULL, NULL); /* fsid */ + flags = g_data_input_stream_read_uint64(reply, NULL, NULL); + g_data_input_stream_read_uint64(reply, NULL, NULL); /* namemax */ Please add spaces before opening parenthesis to be consistent. @@ +4142,3 @@ + if (!no_size && + g_file_attribute_matcher_matches (op_job->attribute_matcher, + G_FILE_ATTRIBUTE_FILESYSTEM_FREE)) Wouldn't be better to check all attributes in try_query_fs_info and set it there only?
Created attachment 259947 [details] [review] sftp: Implement try_query_fs_info using the OpenSSH statvfs extension Use the statvfs command from OpenSSH to fill in the filesystem size, free and used attributes as well as whether the filesystem is readonly or not. The extension is defined at http://api.libssh.org/rfc/PROTOCOL. The extension is signified by the name "statvfs@openssh.com" with a version of "2". Based on a patch Alban Browaeys.
Thanks for the review. (In reply to comment #5) > Review of attachment 257899 [details] [review]: > > Thanks for your patch, just several notes: > > ::: daemon/gvfsbackendsftp.c > @@ +1658,3 @@ > + if ((strcmp(extension_name, "statvfs@openssh.com") == 0) > + && (strcmp(extension_data, "2") == 0)) > + op_backend->server_extensions |= SFTP_EXT_OPENSSH_STATVFS; > > Wouldn't be better to do that similar as ftp backend does? > op_backend->server_extensions |= 1 << SFTP_EXT_OPENSSH_STATVFS; Done. > > @@ +4132,3 @@ > + g_data_input_stream_read_uint64(reply, NULL, NULL); /* fsid */ > + flags = g_data_input_stream_read_uint64(reply, NULL, NULL); > + g_data_input_stream_read_uint64(reply, NULL, NULL); /* namemax */ > > Please add spaces before opening parenthesis to be consistent. Done. > > @@ +4142,3 @@ > + if (!no_size && > + g_file_attribute_matcher_matches (op_job->attribute_matcher, > + G_FILE_ATTRIBUTE_FILESYSTEM_FREE)) > > Wouldn't be better to check all attributes in try_query_fs_info and set it > there only? Yes, done.
Review of attachment 259947 [details] [review]: Looks good!
Pushed to master as da8daebe5cf91c2a02a6adf0b8ccd4c56af5029d. Thanks!