I'm on Ubuntu 11.10/GNOME 3.2, using a Samsung Galaxy Nexus with Android 4/ICS.

What I observe is that connecting the GN in ptp mode behaves almost exactly as if it was connected as a USB mass storage device, i.e., automount, icon (camera) appearing in the Launcher & Nautilus, possibility to create/move/delete any kind of files/folders, etc. Even with Banshee (and Rhythmbox I presume), if you put an ".is_audio_player" at the root, the GN will me recognized as a Media/Audio player!  The only thing is that, strangely, it appears to limit the space access to +/-1,5 GB, as there is app. +/- 13 GB available (free) on the phone...

Anyway, I guess that if the mtp connection was providing the same kind of access as ptp (by default, GN is set to connect as mtp device), it would be easier for novice users to have their GN phone works with GNOME.

in debian sid with gnome 3.2.2 when i plug my Galaxy Nexus, the camera icon with "Galaxy" word appearing in Nautilus. When i clic on this new device, i have only a blanc zone with a wait cursor ... after some time i have this error : 
DBus error org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.

If i configure my Nexus in PTP and not MTP i have the same error ... 

For information i have more than 7GB of data on my nexus.

A lot of people using Ubuntu 12.04 also have the same problem, and it's easily repeatable. The "downstream" bug is at https://bugs.launchpad.net/ubuntu/+source/gvfs/+bug/972311

It looks like the following happens:

The "classic" mode to access MTP devices using libmtp is by transferring all the object details first, and then manipulating them. On my Nexus, this can take up to 6 minutes!

Google (and apparently, Microsoft) uses a different method where objects seem to be transferred on a per-directory basis, I think. This means that the initial setup time is much shorter.

Canonical doesn't seem to be willing or able to help with this issue, am I in the right place to get help?

Ubuntu 12.04
Unity
Galaxy Nexus Android 4.04

I'm having this same issue. When I open Nautilus to access my Galaxy Nexus files I get the following message in a dialog:

The folder contents could not be displayed.

Sorry, could not display all the contents of "Galaxy": DBus error org.freedesktop.DBus.Error.NoReply: Did not receive a reply. Possible causes include: the remote application did not send a reply, the message bus security policy blocked the reply, the reply timeout expired, or the network connection was broken.

This was working fine for a while in Ubuntu 12.04 development versions, but maybe it was just because the amount of files in the Galaxy Nexus was not too large and this time out was not being seen.

Ubuntu 12.04
Unity
Galaxy Nexus Android 4.0.4

Same here. I think it will affect more and more users as Android 4 updates are rolled out.

(In reply to comment #1)
>                                         The only thing is that, strangely, it
> appears to limit the space access to +/-1,5 GB, as there is app. +/- 13 GB
> available (free) on the phone...

I have Arch Linux installed, and have an device with Android 4.0. Both 'mtp-detect' and 'gphoto2 --summary' correctly detect the device as having 13,3 GB of total space, but 'gvfs-info -f "gphoto2://[usb:007,014]/"' shows it having +/- 1,3 GB, which is the same size shown by Nautilus.

Setting the device to use PTP instead of MTP I have the same result, except that 'mtp-detect' can't detect the device.

http://intr.overt.org/blog/?p=153

https://github.com/philipl/gvfs

A quick status report:

1) All the basic functionality is in place at this point.

You can browse, upload and download files, delete, rename.

2) With changes to libmtp (all now upstream), you can see thumbnails and have stores appear automatically when unlocking a phone.

3) The main remaining issue is error reporting. libmtp doesn't do a great job here, so it's very hard, if not impossible, to precisely identify errors and their causes - meaning that I pretty much have to either return raw error messages from the library or fallback to generic messages.

4) The one true bug that remains is that if you rename a file, it will simply disappear in Nautilus until you force a refresh. I've looked at the requests coming from Nautilus and they all seem reasonable, and I'm handling them properly, but it just disappears. I'm pretty sure its a Nautilus bug, but haven't traced it yet.

5) Upload/Download of a directory does not work correctly with Nautilus, due to limitations in Nautilus. It will end up issuing a single pull/push request on the directory itself, when it really needs to enumerate, recurse and issue individual calls for the files themselves. (file-roller does this, so you can compress a directory directly off the device).

6) Various other apps don't work seamlessly. evince and file-roller handle the device semantics properly, by making local temporary copies of files, but things like gedit and eog do not. This requires application level changes.

Sounds good!

(In reply to comment #8)
> 4) The one true bug that remains is that if you rename a file, it will simply
> disappear in Nautilus until you force a refresh. I've looked at the requests
> coming from Nautilus and they all seem reasonable, and I'm handling them
> properly, but it just disappears. I'm pretty sure its a Nautilus bug, but
> haven't traced it yet.

There might be several possibe reasons, just a few hints for debugging:
 - Nautilus may rely on GFileMonitor emmitting a signal when something changes in the directory
 - is the renamed file immediatelly available on the device? Aren't you returning from the rename operation too early?

> 5) Upload/Download of a directory does not work correctly with Nautilus, due to
> limitations in Nautilus. It will end up issuing a single pull/push request on
> the directory itself, when it really needs to enumerate, recurse and issue
> individual calls for the files themselves. (file-roller does this, so you can
> compress a directory directly off the device).

I think you just need to return proper error and Nautilus would fall back to recurse. Not every kind of error is exposed to user and actually controls the application.

> 6) Various other apps don't work seamlessly. evince and file-roller handle the
> device semantics properly, by making local temporary copies of files, but
> things like gedit and eog do not. This requires application level changes.

AFAIR this was an issue with some other backend, perhaps obex? Caching was involved as a workaround.

(In reply to comment #9)
> Sounds good!
> 
> 
> There might be several possibe reasons, just a few hints for debugging:
>  - Nautilus may rely on GFileMonitor emmitting a signal when something changes
> in the directory
>  - is the renamed file immediatelly available on the device? Aren't you
> returning from the rename operation too early?

I have file monitors configured, and it emits a change event on the file. I've verified that if I rename the file using a separate gvfs client (the command line rename tool), then Nautilus sees the event and updates accordingly.

libmtp is blocking, so when the rename returns, the operation has really completed.
 
> > 5) Upload/Download of a directory does not work correctly with Nautilus, due to
> > limitations in Nautilus. It will end up issuing a single pull/push request on
> > the directory itself, when it really needs to enumerate, recurse and issue
> > individual calls for the files themselves. (file-roller does this, so you can
> > compress a directory directly off the device).
> 
> I think you just need to return proper error and Nautilus would fall back to
> recurse. Not every kind of error is exposed to user and actually controls the
> application.

Ok. I never tried returning unsupported on the directory pull. :-)

> > 6) Various other apps don't work seamlessly. evince and file-roller handle the
> > device semantics properly, by making local temporary copies of files, but
> > things like gedit and eog do not. This requires application level changes.
> 
> AFAIR this was an issue with some other backend, perhaps obex? Caching was
> involved as a workaround.

Yes, caching in the backend is possible, but as I discussed in my blog post, it's fragile and I've never seen it work well (and I wrote the gphoto2 fuse filesystem, so I've been there :-), so I've very deliberately written this to not do any caching of file data or metadata to maximise reliability. We cannot, and should not, hide this kind of semantic mismatch.

I think there might be a valid argument to implement caching in gvfs itself (why reinvent that wheel in each backend?) using the kind of logic that's in evince but it's not clear even that makes sense, as the application's semantic expectations are not always the same (evince is just a reader, but something like gedit expects to be able to write changes and persist them somewhere useful, etc).

And I'll repeat a specific concern for something like totem - you can't really have a multi gigabyte file on a device and then pretend that random access works on top of an implicit cache - you can't just assume the file will even be able to fit in /tmp (even worse when its a tmpfs), and as you have to download the whole file before you can access it, there's a huge latency that you can't hide from the application

Thanks!

(In reply to comment #10)
> libmtp is blocking, so when the rename returns, the operation has really
> completed.

Hmm, the only thing I can think of would be missing g_vfs_job_set_display_name_set_new_path() and g_vfs_job_succeeded() calls but no further ideas.

> Yes, caching in the backend is possible, but as I discussed in my blog post,
> it's fragile and I've never seen it work well (and I wrote the gphoto2 fuse
> filesystem, so I've been there :-), so I've very deliberately written this to
> not do any caching of file data or metadata to maximise reliability. We cannot,
> and should not, hide this kind of semantic mismatch.

I agree caching is not the right way, other backends like archive suffer from this even more. That's the price for flexibility of GIO.

> I think there might be a valid argument to implement caching in gvfs itself
> (why reinvent that wheel in each backend?) using the kind of logic that's in
> evince but it's not clear even that makes sense, as the application's semantic
> expectations are not always the same (evince is just a reader, but something
> like gedit expects to be able to write changes and persist them somewhere
> useful, etc).

Agreed, general caching layer would be good to have. I think gvfsreadchannel implements kind of readahead but that's only for certain blocks and is little different from what we want to have.

(In reply to comment #9)
> 
> > 5) Upload/Download of a directory does not work correctly with Nautilus, due to
> > limitations in Nautilus. It will end up issuing a single pull/push request on
> > the directory itself, when it really needs to enumerate, recurse and issue
> > individual calls for the files themselves. (file-roller does this, so you can
> > compress a directory directly off the device).
> 
> I think you just need to return proper error and Nautilus would fall back to
> recurse. Not every kind of error is exposed to user and actually controls the
> application.

I tried it and it didn't work, and I've looked inside Nautilus and I don't see any code that looks like mkdir+copy-files that would be needed to implement this.

If I return NOT_IMPLEMENTED for a directory, the error simply appears to the user.

(In reply to comment #10)
> (In reply to comment #9)
> > Sounds good!
> > 
> > 
> > There might be several possibe reasons, just a few hints for debugging:
> >  - Nautilus may rely on GFileMonitor emmitting a signal when something changes
> > in the directory
> >  - is the renamed file immediatelly available on the device? Aren't you
> > returning from the rename operation too early?
> 
> I have file monitors configured, and it emits a change event on the file. I've
> verified that if I rename the file using a separate gvfs client (the command
> line rename tool), then Nautilus sees the event and updates accordingly.
> 
> libmtp is blocking, so when the rename returns, the operation has really
> completed.

It might be completely unrelated, but whenever I rename a file to a dot file in Nautilus, Nautilus keeps showing the old file until I refresh manually (F5). I still haven't reported this bug *shame*

Created attachment 225004 [details] [review]
MTP backend in patch form against master

I'm not sure how you guys want to handle reviewing this. There's obviously my git tree, and I've attached the patch as well.

I think it's reached the point where it's worth starting the review process, although I know it's not 100% there.

I'm aware of the following:

1) Coding style is not perfectly consistent, but neither is the rest of gvfs, so I'm not sure what to hold myself to.

2) Nautilus rename bug remains undiagnosed

3) The incorrect handling of push/pull of directories. I believe this cannot be fixed in the backend, as discussed above.

ping?

Review of attachment 225004 [details] [review]:

Rough first pass.

::: configure.ac
@@ +506,3 @@
+
+  if test "x$msg_libmtp" = "xyes"; then
+    PKG_CHECK_MODULES(LIBMTP, libmtp >= 1.1.0)

Check for both gudev and libmtp here, so pkg-config can merge and order the libraries and cflags as required.

@@ +511,3 @@
+    save_libs="$LIBS"
+    LIBS="$LIBMTP_LIBS"
+    AC_CHECK_LIB(mtp, LIBMTP_Get_Thumbnail, have_libmtp_get_thumbnail=yes)

Why do we need to check for those by hand? Do we need a git version of libmtp?

::: daemon/Makefile.am
@@ +458,3 @@
+	$(GUDEV_CFLAGS) $(LIBMTP_CFLAGS)
+
+gvfsd_mtp_LDADD = $(libraries) $(GUDEV_LIBS) $(LIBMTP_LIBS)

Use only LIBMTP_LIBS here, as the gudev libs should already be in there.

::: daemon/gvfsbackendmtp.c
@@ +61,3 @@
+/* showing debug traces */
+#define DEBUG_SHOW_TRACES 1
+//#define DEBUG_SHOW_ENUMERATE_TRACES 1

No C++-style comments. You could also uncomment it and set it to 0 instead.

@@ +277,3 @@
+  GVfsBackendMtp *op_backend = G_VFS_BACKEND_MTP (user_data);
+
+  if (g_strcmp0(op_backend->dev_path, dev_path) == 0 &&

Space between function and parenthesis.

@@ +278,3 @@
+
+  if (g_strcmp0(op_backend->dev_path, dev_path) == 0 &&
+      strcmp (action, "remove") == 0) {

g_str_equal().

@@ +502,3 @@
+
+static void
+get_device_info(GVfsBackendMtp *backend, GFileInfo *info) {

Curly brace on the next line.

@@ +504,3 @@
+get_device_info(GVfsBackendMtp *backend, GFileInfo *info) {
+  LIBMTP_mtpdevice_t *device = backend->device;
+  const char *name = g_mount_spec_get(g_vfs_backend_get_mount_spec(G_VFS_BACKEND(backend)), "host");

Please split declaration and assignment.

@@ +569,3 @@
+  GIcon *icon;
+  switch (storage->StorageType) {
+  case 1:

What's those magic numbers? Can we have an enum instead?

@@ +987,3 @@
+static void
+do_push(GVfsBackend *backend,
+                                GVfsJobPush *job,

Indentation please.

@@ +1145,3 @@
+    g_vfs_job_failed (G_VFS_JOB (job),
+                      G_IO_ERROR, G_IO_ERROR_FAILED,
+                      "Can't delete entity.");

You'll want to review your error messages against the existing ones, and mark them to be translated.

::: monitor/mtp/gmtpvolume.c
@@ +102,3 @@
+/* Do not free result, it's a static buffer */
+static const char*
+udev_decode_string (const char* encoded)

I'm pretty sure there's decoded strings available through the gudev API as well, easier than this.

::: monitor/mtp/gmtpvolumemonitor.c
@@ +160,3 @@
+  */
+
+  gchar *uri = g_strdup_printf ("mtp://[usb:%s,%s]", usb_bus_num, usb_device_num);

No C99 declarations please.

@@ +211,3 @@
+  GMtpVolumeMonitor *monitor = G_MTP_VOLUME_MONITOR (user_data);
+
+  /* g_debug ("on_uevent: action=%s, device=%s", action, g_udev_device_get_device_file(device)); */

g_debug() is fine to leave uncommented.

Hi Bastien,

Thanks for taking a look. I'll attach an updated patch later this week.

(In reply to comment #16)
> 
> @@ +511,3 @@
> +    save_libs="$LIBS"
> +    LIBS="$LIBMTP_LIBS"
> +    AC_CHECK_LIB(mtp, LIBMTP_Get_Thumbnail, have_libmtp_get_thumbnail=yes)
> 
> Why do we need to check for those by hand? Do we need a git version of libmtp?

I did at the time I made the changes. They've since been released in 1.1.4 and
1.1.5.
 
> 
> @@ +569,3 @@
> +  GIcon *icon;
> +  switch (storage->StorageType) {
> +  case 1:
> 
> What's those magic numbers? Can we have an enum instead?

The enum is, unfortunately, hidden in ptp.h in libmtp - so I'd have to
duplicate it locally, but I can do that.
 
> ::: monitor/mtp/gmtpvolume.c
> @@ +102,3 @@
> +/* Do not free result, it's a static buffer */
> +static const char*
> +udev_decode_string (const char* encoded)
> 
> I'm pretty sure there's decoded strings available through the gudev API as
> well, easier than this.

I'll take a look. This came from the gphoto2 volume implementation.

Created attachment 226395 [details] [review]
Updated diff against master

This change addresses the initial review feedback with a couple of exceptions

1) I haven't made any configure.ac changes yet.

I do need to introduce version based checks for libmtp features, but the other feedback is confusing in that it doesn't reflect how gudev is handled by other gvfs components. Do you really want me to diverge from the rest of the file?

2) I couldn't find any other udev string decoding implementation, so I've retained the one I have (from gphoto2 monitor)

Review of attachment 226395 [details] [review]:

Posting first part of the review, the rest will follow (forcing browser to save the draft)

::: daemon/gvfsbackendmtp.c
@@ +266,3 @@
+                      G_IO_ERROR_FAILED,
+                    _("libmtp error: %s"),
+                    g_strrstr (error->error_text, ":") + 1);

We may want to tweak errors a little since some of them could be presented to end user, having "libmtp error" prefix.

@@ +296,3 @@
+    DEBUG ("(I) on_uevent: Quiting after remove event on device %s", dev_path);
+    /* TODO: need a cleaner way to force unmount ourselves */
+    exit (1);

If you would like to experiment, g_vfs_backend_unmount_with_operation() may do the job, the problem is getting valid GMountSource instance.

@@ +347,3 @@
+    g_set_error_literal (&error, G_IO_ERROR, G_IO_ERROR_FAILED, _("No device specified"));
+    g_vfs_job_failed_from_error (G_VFS_JOB (job), error);
+    g_error_free (error);

better to use g_vfs_job_failed_literal() directly

@@ +367,3 @@
+                      G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
+                      _("Unexpected host uri format."));
+    return;

leaking op_backend->gudev_client, multiple occurrences, may be beneficial to split udev device name parsing to a separate function

@@ +376,3 @@
+    g_vfs_job_failed (G_VFS_JOB (job),
+                      G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
+                      _("Malformed host uri."));

use _literal function variant if not using format string

@@ +398,3 @@
+  get_device (backend, host, G_VFS_JOB (job));
+  if (!G_VFS_JOB (job)->failed) {
+    GMountSpec *mtp_mount_spec = g_mount_spec_new ("mtp");

A matter of developer taste, moving variable declaration up may help code readability

@@ +406,3 @@
+
+#if HAVE_LIBMTP_READ_EVENT
+    g_thread_new ("events", check_event, backend);

Either take reference to backend or to the thread and cancel it in do_unmount(), to prevent having the thread running when the used data structures are freed.

@@ +536,3 @@
+  free (friendlyname);
+
+  g_file_info_set_file_type (info, G_FILE_TYPE_DIRECTORY);

Duplicate call :-)

@@ +664,3 @@
+
+    char *icon_id;
+    GIcon *icon;

icon variable redefinition

Review of attachment 226395 [details] [review]:

::: configure.ac
@@ +500,3 @@
+msg_libmtp=no
+LIBMTP_LIBS=
+LIBMTP_CFLAGS=

Wondering whether LIBMTP_LIBS and LIBMTP_CFLAGS don't need to be AC_SUBST()'ed and then used in monitor/mtp/Makefile.am

::: daemon/gvfsbackendmtp.c
@@ +625,3 @@
+}
+
+static void

Hint: would be beneficial to add a comment like "Called with mutex lock hold" for every function that the comment applies to, for better orientation and maintenance.

@@ +714,3 @@
+  gchar **elements = g_strsplit_set (filename, "/", -1);
+  unsigned int ne = 0;
+  for (ne = 0; elements[ne] != NULL; ne++);

Use g_strv_length() instead

@@ +1010,3 @@
+  get_file_info (backend, device, info, file);
+  LIBMTP_destroy_file_t (file);
+  file = NULL;

not necessary

@@ +1017,3 @@
+    if (file) {
+      error = NULL;
+      if (g_file_make_directory (file, G_VFS_JOB (job)->cancellable, &error)) {

My feeling is that pulling a directory should lead to an error, gvfs or client app should fall back to calling g_file_make_directory() itselves.

See do_pull_improve_error_message() in gvfsbackendftp.c for inspiration.

@@ +1094,3 @@
+
+  file = g_file_new_for_path (local_path);
+  if (!file) {

g_file_new_ family of functions don't fail if the file doesn't exist, it's not doing any I/O. Simple assert can catch possible NULL result, however that only happens when something goes really wrong. You need to catch file-not-found errors in the code below, e.g. as a result of g_file_query_info().

@@ +1108,3 @@
+     * here.
+     */
+    return do_make_directory (backend, G_VFS_JOB_MAKE_DIRECTORY (job),

- leaking file
- deadlock as the backend->mutex lock is held at this point

Moreover this should also return an error (G_IO_ERROR_WOULD_RECURSE), see do_push() in gvfsbackendobexftp.c for other error codes.

@@ +1152,3 @@
+ exit:
+  if (file) {
+    g_object_unref (file);

Hint: using g_clear_object() may be cleaner to the code

@@ +1229,3 @@
+
+  LIBMTP_file_t *file = LIBMTP_Get_Filemetadata (device, strtol (elements[ne-1], NULL, 10));
+  int ret = LIBMTP_Set_File_Name (device, file, display_name);

Note: beware of set_display_name() used for inter-mount file move. As I understand it, the mtp backend merges multiple file storages on the device. Changing file path to a different storage would involve an I/O transfer instead of simple rename. Please check.

::: monitor/mtp/gmtpvolume.c
@@ +104,3 @@
+udev_decode_string (const char* encoded)
+{
+  static char decoded[4096];

Hint: static variable, watch out for multithreaded use

::: monitor/mtp/gmtpvolumemonitor.c
@@ +293,3 @@
+  /* Today's Linux desktops pretty much need udev to have anything working, so
+   * assume it's there */
+  return TRUE;

...and what about BSD? :-)

(In reply to comment #18)
> Created an attachment (id=226395) [details] [review]
> Updated diff against master

So I have gone through this patch and posted some notes above. Most of my concern was about error reporting and minor leaks. I've added some hints and some personal opinions. Generally the code is in good shape and after fixing the major concerns, it's ready to be merged.

I haven't tested the functionality due to lack of available MTP devices. Compiles fine against git master.

found some compiler warnings:

gmtpvolumemonitor.c: In function ‘gudev_add_device’:
gmtpvolumemonitor.c:157:3: warning: format ‘%i’ expects argument of type ‘int’, but argument 5 has type ‘const char *’ [-Wformat]
gmtpvolumemonitor.c:157:3: warning: format ‘%i’ expects argument of type ‘int’, but argument 6 has type ‘const char *’ [-Wformat]
gmtpvolumemonitor.c:163:3: warning: passing argument 1 of ‘g_free’ discards ‘const’ qualifier from pointer target type [enabled by default]
In file included from /usr/include/glib-2.0/glib/glist.h:34:0,
                 from /usr/include/glib-2.0/glib/ghash.h:35,
                 from /usr/include/glib-2.0/glib.h:52,
                 from gmtpvolumemonitor.c:28:
/usr/include/glib-2.0/glib/gmem.h:70:7: note: expected ‘gpointer’ but argument is of type ‘const char *’

Hi Tomas,

Thanks for taking a look over it. I will update the diff with the relevant fixes as soon as I can. Given my current schedule it will probably be a couple of weeks before I can get it all done.

Review of attachment 226395 [details] [review]:

::: daemon/gvfsbackendmtp.c
@@ +126,3 @@
+  g_mount_spec_unref (mount_spec);
+
+  backend->monitors = g_hash_table_new (g_direct_hash, g_direct_equal);

you can use NULL, NULL instead of g_direct_*.

@@ +143,3 @@
+  g_mutex_clear (&backend->mutex);
+
+  if (G_OBJECT_CLASS (g_vfs_backend_mtp_parent_class)->finalize)

just a minimal style issue, here: finalize() is guaranteed to always be set. you should never check for NULL before chaining up.

@@ +170,3 @@
+
+  g_vfs_job_create_monitor_set_monitor (job, vfs_monitor);
+  g_hash_table_insert (mtp_backend->monitors, vfs_monitor, NULL);

if you want to use a hash table as a set, you can use g_hash_table_add() instead of g_hash_table_insert().

@@ +372,3 @@
+  char *comma;
+  char *dev_path = g_strconcat ("/dev/bus/usb/", host + 5, NULL);
+  if ((comma = strchr (dev_path, ',')) == NULL) {

I'd check for malformed host before concatenating it to the fixed /dev path, to avoid a needless allocation.

@@ +507,3 @@
+      LIBMTP_Dump_Errorstack (device);
+      LIBMTP_Clear_Errorstack (device);
+      break;

you're leaking name in case of success.

@@ +1130,3 @@
+  mtpfile->filesize = g_file_info_get_size (info);
+
+  MtpProgressData *mtp_progress_data = g_new0 (MtpProgressData, 1);

if mtp_progress_data has just to survive the LIBTMP_Send_From_From_File() call, then you can put it on the stack, and avoid hitting the allocator.

@@ +1155,3 @@
+  }
+  if (info) {
+    g_object_unref (info);

same as above: g_clear_object() allows you to avoid the NULL check entirely.

::: monitor/mtp/gmtpvolume.c
@@ +62,3 @@
+
+  if (volume->device != NULL)
+    g_object_unref (volume->device);

use g_clear_object().

@@ +65,3 @@
+
+  if (volume->activation_root != NULL)
+    g_object_unref (volume->activation_root);

same as above.

@@ +73,3 @@
+  g_free (volume->icon);
+
+  if (G_OBJECT_CLASS (g_mtp_volume_parent_class)->finalize)

do not check for NULL before chaining up finalize().

@@ +160,3 @@
+      /* we can't call udev_decode_string() twice in one g_strdup_printf(),
+       * it returns a static buffer */
+      gchar *temp = g_strdup_printf ("%s %s", vendor, model);

you can use g_strconcat() here.

::: monitor/mtp/gmtpvolumemonitor.c
@@ +61,3 @@
+list_free (GList *objects)
+{
+  g_list_foreach (objects, (GFunc)g_object_unref, NULL);

you can use g_list_free_full().

@@ +69,3 @@
+{
+  G_LOCK (vm_lock);
+  the_volume_monitor = NULL;

dispose can temporally auto-vivify an instance, so you probably want to move the singleton pointer unset into finalize.

@@ +72,3 @@
+  G_UNLOCK (vm_lock);
+
+  if (G_OBJECT_CLASS (g_mtp_volume_monitor_parent_class)->dispose)

same as with finalize, dispose() is guaranteed to always be !NULL.

@@ +228,3 @@
+  GList *usb_devices, *l;
+
+  usb_devices = g_udev_client_query_by_subsystem (monitor->gudev_client, "usb");

you're leaking usb_devices.

(In reply to comment #19)
> Review of attachment 226395 [details] [review]:
> 
> Posting first part of the review, the rest will follow (forcing browser to save
> the draft)

Hi Tomas,

I'm now finally getting around to addressing feedback.

Please see the individual fixes here:

https://github.com/philipl/gvfs/commits/mtp-1.14.0

I will attach a complete updated diff to this bug when I've addressed
all feedback.

> ::: daemon/gvfsbackendmtp.c
> @@ +266,3 @@
> +                      G_IO_ERROR_FAILED,
> +                    _("libmtp error: %s"),
> +                    g_strrstr (error->error_text, ":") + 1);
> 
> We may want to tweak errors a little since some of them could be presented to
> end user, having "libmtp error" prefix.

I'm not sure what you'd like to see. I'm forced to pass verbatim
errors from libmtp so it seems necessary to qualify that this is
what they are, rather than any error text I'm generating myself.

> If you would like to experiment, g_vfs_backend_unmount_with_operation() may do
> the job, the problem is getting valid GMountSource instance.

Took a brief look but I'll politely pass. :-) The logic here is identical
to the gphoto2 backend.
 
> better to use g_vfs_job_failed_literal() directly

Thanks. Used in all applicable places.

> 
> @@ +367,3 @@
> +                      G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
> +                      _("Unexpected host uri format."));
> +    return;
> 
> leaking op_backend->gudev_client, multiple occurrences, may be beneficial to
> split udev device name parsing to a separate function

Done.

> @@ +376,3 @@
> +    g_vfs_job_failed (G_VFS_JOB (job),
> +                      G_IO_ERROR, G_IO_ERROR_NOT_SUPPORTED,
> +                      _("Malformed host uri."));
> 
> use _literal function variant if not using format string

Done.

> @@ +398,3 @@
> +  get_device (backend, host, G_VFS_JOB (job));
> +  if (!G_VFS_JOB (job)->failed) {
> +    GMountSpec *mtp_mount_spec = g_mount_spec_new ("mtp");
> 
> A matter of developer taste, moving variable declaration up may help code
> readability

I've generally done this, but I want to keep declaration scope as
narrow as possible, so inside an if() block is appropriate.

> @@ +406,3 @@
> +
> +#if HAVE_LIBMTP_READ_EVENT
> +    g_thread_new ("events", check_event, backend);
> 
> Either take reference to backend or to the thread and cancel it in
> do_unmount(), to prevent having the thread running when the used data
> structures are freed.

See my commit description. I've done the best I can but libmtp prevents
a complete fix for the race.

> @@ +536,3 @@
> +  free (friendlyname);
> +
> +  g_file_info_set_file_type (info, G_FILE_TYPE_DIRECTORY);
> 
> Duplicate call :-)
> 
> @@ +664,3 @@
> +
> +    char *icon_id;
> +    GIcon *icon;
> 
> icon variable redefinition

Both fixed.

Thanks.

(In reply to comment #20)
> Wondering whether LIBMTP_LIBS and LIBMTP_CFLAGS don't need to be AC_SUBST()'ed
> and then used in monitor/mtp/Makefile.am

They do need to be SUBSTed but not needed in monitor/mtp as there are no libmtp
calls in there - only in daemon/
 
> 
> Hint: would be beneficial to add a comment like "Called with mutex lock hold"
> for every function that the comment applies to, for better orientation and
> maintenance.

Done.
 
> Use g_strv_length() instead

Done.
 
> @@ +1010,3 @@
> +  get_file_info (backend, device, info, file);
> +  LIBMTP_destroy_file_t (file);
> +  file = NULL;
> 
> not necessary

Removed.

> My feeling is that pulling a directory should lead to an error, gvfs or client
> app should fall back to calling g_file_make_directory() itselves.
> 
> See do_pull_improve_error_message() in gvfsbackendftp.c for inspiration.

Thank you! I could not work out what the magic error code was to make the
client do the right thing.

> g_file_new_ family of functions don't fail if the file doesn't exist, it's not
> doing any I/O. Simple assert can catch possible NULL result, however that only
> happens when something goes really wrong. You need to catch file-not-found
> errors in the code below, e.g. as a result of g_file_query_info().

Fixed.
 
> Moreover this should also return an error (G_IO_ERROR_WOULD_RECURSE), see
> do_push() in gvfsbackendobexftp.c for other error codes.

Yay again. I still need to do more work, as discussed in the commit description
before push completely works, but almost there.
 
> Hint: using g_clear_object() may be cleaner to the code

Done.
 
> Note: beware of set_display_name() used for inter-mount file move. As I
> understand it, the mtp backend merges multiple file storages on the device.
> Changing file path to a different storage would involve an I/O transfer instead
> of simple rename. Please check.

This is ok. I've never seen a move mapped to a rename, and in any case, it
would fail at the libmtp level as rename can only change the leaf filename.
 
> ::: monitor/mtp/gmtpvolume.c
> @@ +104,3 @@
> +udev_decode_string (const char* encoded)
> +{
> +  static char decoded[4096];
> 
> Hint: static variable, watch out for multithreaded use

Inherited from gphoto2 monitor but fixed.
 
> ::: monitor/mtp/gmtpvolumemonitor.c
> @@ +293,3 @@
> +  /* Today's Linux desktops pretty much need udev to have anything working, so
> +   * assume it's there */
> +  return TRUE;
> 
> ...and what about BSD? :-)

You tell me. :-) This is also taken verbatim from the gphoto2 monitor.

(In reply to comment #24)
> @@ +372,3 @@
> +  char *comma;
> +  char *dev_path = g_strconcat ("/dev/bus/usb/", host + 5, NULL);
> +  if ((comma = strchr (dev_path, ',')) == NULL) {
> 
> I'd check for malformed host before concatenating it to the fixed /dev path, to
> avoid a needless allocation.

Doing that would require doing the strchr twice, which seems overly convoluted.

All other recommendations applied.

Created attachment 232704 [details] [review]
Updated diff, incorporating all feedback

This diff includes all the discussed feedback.

I still need to make the additional changes to ensure push works as expected.

Thanks for fixing my concerns!

(In reply to comment #25)
> I'm not sure what you'd like to see. I'm forced to pass verbatim
> errors from libmtp so it seems necessary to qualify that this is
> what they are, rather than any error text I'm generating myself.

My point was about having more nice errors since some of them will appear in GUI error dialogs and end user may be confused if he sees technical error message. But anyway, leave it as it is for the moment, we can tune it later, based on feedback from users or designers if there's any.

> See my commit description. I've done the best I can but libmtp prevents
> a complete fix for the race.

Oh I see your point now. So let's hope the thread will block forever and we don't hit the corner case. Your change looks good to me.

Review of attachment 232704 [details] [review]:

Looks great, almost ready for a merge? What's your ETA for the push fixes? We still have time till mid-Feb, however people may start testing the pull way in the meantime.

Just a minor note from your recent addition:

::: daemon/gvfsbackendmtp.c
@@ +445,3 @@
+  if (dev_path == NULL) {
+    g_object_unref (op_backend->gudev_client);
+    // get_dev_path_from_host() sets job state.

Please, no C++ style comments (code style).

(In reply to comment #30)
> Review of attachment 232704 [details] [review]:
> 
> Looks great, almost ready for a merge? What's your ETA for the push fixes? We
> still have time till mid-Feb, however people may start testing the pull way in
> the meantime.

Thanks!

Yes, it's almost ready. I've got a working implementation of the push fix in
github, but I need to clean it up a little bit. I expect I'll be able to finalize it and be ready by the end of the week.

Mechanically, how do you want to handle the merge? Should I squash the change
history and push a single commit to master?

> Just a minor note from your recent addition:
> 
> ::: daemon/gvfsbackendmtp.c
> @@ +445,3 @@
> +  if (dev_path == NULL) {
> +    g_object_unref (op_backend->gudev_client);
> +    // get_dev_path_from_host() sets job state.
> 
> Please, no C++ style comments (code style).

Will fix.

(In reply to comment #31)
> Yes, it's almost ready. I've got a working implementation of the push fix in
> github, but I need to clean it up a little bit. I expect I'll be able to
> finalize it and be ready by the end of the week.

Cool, no hurry then.

> Mechanically, how do you want to handle the merge? Should I squash the change
> history and push a single commit to master?

I'm fine with having full history in the repo, just would be great to prepend all commit messages with something like "mtp: " to easily distinguish what was the commit related to. I think that can be easily done during rebase.

Created attachment 233036 [details] [review]
Final Diff

Here's the final diff including bug fixes and the extra logic needed to make do_push work properly.

Created attachment 233037 [details] [review]
Interdiff from last reviewed patch to final patch

This diff is just the differences from the last reviewed diff to the final diff.

(In reply to comment #34)
> Created an attachment (id=233037) [details] [review]
> Interdiff from last reviewed patch to final patch
> 
> This diff is just the differences from the last reviewed diff to the final
> diff.

Looks good, I've been looking at your git branch for particular commits. I think this is ready to merge, if you're confident it's working :-) Compiles fine, this is all that I can test.

So please go ahead and merge it to git master, keeping comment 32 in mind if possible. We'll handle coming issues separately.

Pushed! Thanks for the reviews. It's great to finally get this in!

Tomas, you should add an MTP backend component to gvfs and set me as the default assignee.

Thanks for the merge, everything is fine. Going to be released today as gvfs-1.15.2

(In reply to comment #37)
> Tomas, you should add an MTP backend component to gvfs and set me as the
> default assignee.

Done, thanks for reminding, expect lots of spam now :-)