GNOME Bugzilla – Bug 757901
Resurrect obexftp backend
Last modified: 2018-09-21 17:54:55 UTC
Created attachment 315196 [details] [review] Initial version of the patch. This is an attempt to add back the obexftp backend. This patch is based on old backend code, ported to use Bluez 5 obexd. USB support was removed, as it is not supported by the obexd. dbus_glib code was replaced with GDBus objects.
Bastien, you have best knowledges in this area, could you look at the patch, please? Though I am not really sure it is meaningful to add the backend nowadays...
(In reply to Ondrej Holy from comment #1) > Bastien, you have best knowledges in this area, could you look at the patch, > please? Though I am not really sure it is meaningful to add the backend > nowadays... It's still useful for a number of devices. Probably not the latest smartphones, but there's plenty of feature phones that would make the device's use easier.
Review of attachment 315196 [details] [review]: We don't use signed-off-by, but you can add your name in the copyright headers if you modified them. I would split up your patch into a "revert portions of commit XXXX" (I don't think the cap-parser* files changed, did they?) and actually adding it to the build system. Even a pure revert followed by your changes might make it easier to review. ::: daemon/gvfsbackendobexftp.c @@ +135,3 @@ + gchar *name = NULL; + GError *error = NULL; + GDBusObjectManager *manager = g_dbus_object_manager_client_new_for_bus_sync( That'd be nicer if you separated the declaration and the assignment. There should also be a spec before "(" and you can put that all on the same line, we're not restricted to 80 character lines. @@ +146,3 @@ + &error); + if (!manager) + { You'll need to make sure you're consistent with your braces. On the same line as the condition or not? @@ +147,3 @@ + if (!manager) + { + g_error_free (error); Why get the error if you're not going to use it? That goes for all the similar uses of error later in the code. @@ +240,3 @@ + + if (backend->session_proxy != NULL) + g_object_unref (backend->session_proxy); I think the indentation is wrong in a bunch of places. @@ +344,3 @@ +} + +static GPtrArray *mem_types = NULL; This should probably go in the backend struct.
(In reply to Bastien Nocera from comment #3) > Review of attachment 315196 [details] [review] [review]: Thank for your review. > > We don't use signed-off-by, but you can add your name in the copyright > headers if you modified them. OK. I just have a commit -s in my .gitconfig, so it was applied automatically. > > I would split up your patch into a "revert portions of commit XXXX" (I don't > think the cap-parser* files changed, did they?) and actually adding it to > the build system. > Even a pure revert followed by your changes might make it easier to review. OK, I will split it into 'revert cap parser' and 'add obex backend'. The main backend code did change a lot (DBus library, DBus API, etc) so comparing it to the original code doesn't make too much sense to me. The Cap parser is unchanged (it might be worth switching to GMarkup, but I didn't want to do that ATM). The FileList parser was dropped - it's done entirely in obexd now. I will review the indentation, sorry about that.
Review of attachment 315196 [details] [review]: ::: daemon/gvfsbackendobexftp.c @@ +288,3 @@ + if (!ret) + { + g_message ("ChangeFolder failed"); This should probably be g_warning. @@ +934,3 @@ +do_open_for_read (GVfsBackend *backend, + GVfsJobOpenForRead *job, + const char *filename) Pull method should be implemented also to be used preferentially... @@ +983,3 @@ + + fd = g_file_open_tmp ("gvfsobexftp-tmp-XXXXXX", + &target, &error); Why not use g_file_new_tmp and consequently use gio api to read the stream? It would help to implement seeking etc. @@ +995,3 @@ + success = _get_file_helper (op_backend, job, filename, target, &error); + + g_message ("filename: %s copying to %s (retval %d)", filename, target, success); This should really be g_debug. @@ +1019,3 @@ + g_debug ("- do_open_for_read, filename: %s size %d\n", filename, size); + + g_vfs_job_open_for_read_set_can_seek (G_VFS_JOB_OPEN_FOR_READ (job), FALSE); Seeking should be implemented if there is whole open/read/close emulation code, but it can be done later. It would be really easy using GSeekable... @@ +1039,3 @@ + bytes_read = read (backend_handle->fd, buffer, bytes_requested); + + if (g_vfs_job_is_cancelled (G_VFS_JOB (job))) It seems to me this isn't needed if all I/O is already done, however why not use g_input_stream_read, which takes GCancellable... @@ +1052,3 @@ + } + else if (bytes_read == 0) + { This block seems to be redundant. @@ +1416,3 @@ + gboolean remove_source, + GFileProgressCallback progress_callback, + gpointer progress_callback_data) Progress callback has to be called at least with total size at the end of this function: g_vfs_job_progress_callback (size, size, job);
Bastien, thanks for looking at this. I remember Tomas told me, before he left, that there used to be same race conditions with the old code, which can't be fixed. Therefor it needs to be ported to bluez and also that same backend restructuralization is needed... I didn't compare old gvfsbackendobexftp.c with this new one. (It might be still interesting to revert whole old backend and then add commits with port to bluez, not just a part...) Do you remember what was wrong? Is it fixed by this port?
(In reply to Ondrej Holy from comment #6) > Bastien, thanks for looking at this. > > I remember Tomas told me, before he left, that there used to be same race > conditions with the old code, which can't be fixed. There are some problems related to the fact that the API is very high level, and that we use it in a way that's low-level. The fact that the front-end (nautilus) was talking to gvfs, which was then talking to an obex daemon, which was then talking to the device was causing problems. The obex daemon API wasn't a great match for what is needed to implement a file manager, hence the resulting bugs. See: https://bugzilla.gnome.org/buglist.cgi?component=[obsolete]%20obexftp%20backend&list_id=123242&product=gvfs&query_format=advanced&order=resolution,bug_id&query_based_on= > Therefor it needs to be > ported to bluez and also that same backend restructuralization is needed... It's not going to be enough to fix the inherent problems that ObexFTP isn't very well suited to something like gvfs. > I didn't compare old gvfsbackendobexftp.c with this new one. (It might be > still interesting to revert whole old backend and then add commits with port > to bluez, not just a part...) > > Do you remember what was wrong? Is it fixed by this port? I would either have the gvfs backend talk to the device themselves, directly, without a daemon in the middle. Or implement this as an application, as OSX has done for a number of years.
-- GitLab Migration Automatic Message -- This bug has been migrated to GNOME's GitLab instance and has been closed from further activity. You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gvfs/issues/267.