GNOME Bugzilla – Bug 609340
Port gvfsd-obexftp to internal Obex handling
Last modified: 2015-03-04 15:37:13 UTC
This saves us from much D-Bus-y pain with obex D-Bus servers.
Created attachment 153274 [details] [review] Port gvfsd-obexftp to internal Obex handling
Review of attachment 153274 [details] [review]: Looks fine overall, I inlined some comments, mostly about code style and some leaks. I don't like c/p large-ish chunks of code though, so it'd probably be better to start packaging gw-obex as a separate library and link to that? ::: daemon/gvfsbackendobexftp.c @@ +97,3 @@ + guint8 channel; + int sock; + uuid_t uuid; You should free this in _finalize () if it's possible @@ +634,3 @@ + return 0; +#endif Any reason to keep this #if0-ed code? @@ +647,3 @@ g_free (backend->icon_name); g_free (backend->bdaddr); + g_hash_table_destroy (backend->cache); backend->current_dir is leaked AFAICS. @@ +696,3 @@ /* Are we already at the root? */ + if (strcmp (op_backend->current_dir, "/") != 0) It's probably better to use g_strcmp0() here @@ +699,2 @@ { + if (gw_obex_chdir(op_backend->obex, "", &err) == FALSE) Missing space before ( @@ +713,3 @@ continue; if (*req_components[i] == '\0') + if (gw_obex_chdir(op_backend->obex, req_components[i], &err) == FALSE) Ditto @@ +716,2 @@ { + g_message ("ChangeCurrentFolder '%s' failed (%d)", req_components[i], err); You're already printing the error message during _obex_error_to_error(), so you can probably leave the (%d) part out of this message. @@ +910,3 @@ + int channel = -1; +get_rfcomm_channel (sdp_record_t *rec) +static int Some missing spaces here. @@ +915,3 @@ } +find_service_channel (bdaddr_t *adapter, bdaddr_t *device, uint16_t svclass_id) +static int There are many missing spaces before brackets in this function and in rfcomm_connect() @@ +1070,2 @@ + str2ba(op_backend->bdaddr, &dst); + bacpy(&src, BDADDR_ANY); Missing spaces here. @@ +1154,3 @@ { if (op_backend->bdaddr) + if (session_create(op_backend) < 0) Missing space. @@ +1403,2 @@ { + GError *error = NULL; You should probably free the handle in this case as well. @@ +1422,1 @@ + g_free (backend_handle->source); It should be safe to add an if (backend_handle->xfer != NULL) gw_obex_xfer_free () just in case. @@ +1465,3 @@ + if (_query_file_info_helper (backend, filename, info, &error) != FALSE || + info = g_file_info_new (); + /* Change into the directory check whether the file exists */ Why is "!= FALSE" here needed? I find that syntax a bit confusing. @@ +1552,3 @@ + g_vfs_job_failed_from_error (G_VFS_JOB (job), error); + g_error_free (error); + } Does close() get called in this case? Otherwise we're leaking the backend handle. This also applies to the _read() case
Oh by the way, I saw you #if0 some code related to USB handling. Does that mean this patch drops support for those devices?
Patch to export osso-gwobex directly upstream: http://thread.gmane.org/gmane.linux.bluez.kernel/4504 (In reply to comment #3) > Oh by the way, I saw you #if0 some code related to USB handling. Does that mean > this patch drops support for those devices? Yes, but it was never exposed, as we didn't have a volume monitor for it. I'll check what's needed to implement it soon.
(In reply to comment #2) > Review of attachment 153274 [details] [review]: > > Looks fine overall, I inlined some comments, mostly about code style and some > leaks. > I don't like c/p large-ish chunks of code though, so it'd probably be better to > start packaging gw-obex as a separate library and link to that? Posted the link to the patch to do that. > ::: daemon/gvfsbackendobexftp.c > @@ +97,3 @@ > + guint8 channel; > + int sock; > + uuid_t uuid; > > You should free this in _finalize () if it's possible Nothing to free here. > @@ +634,3 @@ > > + return 0; > +#endif > > Any reason to keep this #if0-ed code? Yeah, it's the USB handling code... > @@ +647,3 @@ > g_free (backend->icon_name); > g_free (backend->bdaddr); > + g_hash_table_destroy (backend->cache); > > backend->current_dir is leaked AFAICS. fixed. > @@ +696,3 @@ > /* Are we already at the root? */ > > + if (strcmp (op_backend->current_dir, "/") != 0) > > It's probably better to use g_strcmp0() here I'd rather not. It's never supposed to be NULL, so it's easier to make it crash, rather than add a separate assert :) > @@ +699,2 @@ > { > + if (gw_obex_chdir(op_backend->obex, "", &err) == FALSE) > > Missing space before ( Fixed everywhere. > @@ +713,3 @@ > continue; > if (*req_components[i] == '\0') > + if (gw_obex_chdir(op_backend->obex, req_components[i], &err) == FALSE) > > Ditto Fixed. > @@ +716,2 @@ > { > + g_message ("ChangeCurrentFolder '%s' failed (%d)", > req_components[i], err); > > You're already printing the error message during _obex_error_to_error(), so you > can probably leave the (%d) part out of this message. Fair enough. > @@ +910,3 @@ > + int channel = -1; > +get_rfcomm_channel (sdp_record_t *rec) > +static int > > Some missing spaces here. Fixed. > @@ +915,3 @@ > } > +find_service_channel (bdaddr_t *adapter, bdaddr_t *device, uint16_t > svclass_id) > +static int > > There are many missing spaces before brackets in this function and in > rfcomm_connect() Fixed. > @@ +1070,2 @@ > + str2ba(op_backend->bdaddr, &dst); > + bacpy(&src, BDADDR_ANY); > > Missing spaces here. Fixed. > @@ +1154,3 @@ > { > if (op_backend->bdaddr) > + if (session_create(op_backend) < 0) > > Missing space. Fixed. > @@ +1403,2 @@ > { > + GError *error = NULL; > > You should probably free the handle in this case as well. > > @@ +1422,1 @@ > + g_free (backend_handle->source); > > It should be safe to add an > > if (backend_handle->xfer != NULL) > gw_obex_xfer_free () > > just in case. > > @@ +1465,3 @@ > + if (_query_file_info_helper (backend, filename, info, &error) != FALSE || > + info = g_file_info_new (); > + /* Change into the directory check whether the file exists */ > > Why is "!= FALSE" here needed? I find that syntax a bit confusing. I always use != FALSE... > @@ +1552,3 @@ > + g_vfs_job_failed_from_error (G_VFS_JOB (job), error); > + g_error_free (error); > + } > > Does close() get called in this case? Otherwise we're leaking the backend > handle. This also applies to the _read() case close is supposed to be called afterwards. This code is the same in all the backends.
Created attachment 153351 [details] [review] Port gvfsd-obexftp to internal Obex handling This saves us from much D-Bus-y pain with obex D-Bus servers.
Review of attachment 153351 [details] [review]: Other than the following comment, looks great. ::: daemon/gvfsbackendobexftp.c @@ +98,1 @@ It was this |obex| variable that I am afraid could be leaked (it's missing a gw_obex_close() somewhere); for a strange Splinter bug the comment did not contain the reference to the right line but only to the lines before that...
Hrm, Splinter hates me. This is the snippet I am referring to: @@ -80,9 +91,10 @@ struct _GVfsBackendObexftp char *icon_name; gint usbintfnum; - DBusGConnection *connection; - DBusGProxy *manager_proxy; - DBusGProxy *session_proxy; + uuid_t uuid; + int sock; + guint8 channel; + GwObex *obex;
Created attachment 154118 [details] [review] Port gvfsd-obexftp to internal Obex handling This saves us from much D-Bus-y pain with obex D-Bus servers.
Latest patch: - fixes the directory cache - fixes the memleak from comment 8 - forces use of the cache when another transfer is on-going - simplifies the locking slightly (osso-gwobex already does its own locking) Works quite well, along with the osso-gwobex patches I sent to the linux-bluetooth mailing-list. I was able to play music files in Totem using this code.
Fedora review request and packages, for testers: https://bugzilla.redhat.com/show_bug.cgi?id=566397 http://koji.fedoraproject.org/koji/taskinfo?taskID=1996444
Created attachment 154207 [details] daemon upload log, copied via tuxcmd Compiles fine, with the following warnings (not relevant): gvfsbackendobexftp.c:444: warning: ‘_find_ods_usb_intfnum’ defined but not used gvfsbackendobexftp.c:482: warning: ‘_get_usb_intfnum_and_properties’ defined but not used Works like a charm when copying from the phone, pretty fast, no suspicious messages from the backend process. Mounting, unmounting works. But uploading files to the phone doesn't work - phone asks me whether to accept the file, I confirm, but file isn't copied, no error displayed. See the attached file for daemon log. Subsequent tries will get me "Device or resource busy" errors, no prompts on the phone. Need to remount, devices are not wedged. This is Sony Ericsson K750i with very old firmware.
Created attachment 154208 [details] daemon upload log (try #2), copied via nautilus Second try, cleanly remounted, uploading a file via Nautilus. Can see upload progress on both sides but when it's finished, the file is not there (not even in the phone).
Created attachment 154318 [details] [review] Port gvfsd-obexftp to internal Obex handling This saves us from much D-Bus-y pain with obex D-Bus servers. The USB support requires osso-gwobex and openobex patches.
TODO: - Add udev based information gathering for the device (not sure how to get the icon though, will probably assume it's a phone, as I don't know anything other than phones implementing Obex over USB) - Fix uploading - Fix possible disconnection (easy to see on USB when listing the same directory multiple times, could be the sync/async function mix, as mentioned by jhe, could be fixable in osso-gwobex though).
Created attachment 197253 [details] [review] Port gvfsd-obexftp to internal Obex handling This saves us from much D-Bus-y pain with obex D-Bus servers. The USB support requires osso-gwobex and openobex patches.
Comment on attachment 197253 [details] [review] Port gvfsd-obexftp to internal Obex handling osso-gwobex isn't maintained anymore, and had quite a few bugs. So all this nice code would need to be ported to use gobex, as used internally (for now) to obexd. See https://github.com/connectivity/obexd/blob/master/gobex/gobex.h (will move back to git.kernel.org when it's back up).
Obexftp backend has been obsoleted in GNOME 3.10, because it is incompatible with Bluez 5, however limited bluetooth support is currently built-in in GNOME desktop.