The naming convention here was copied from what the ptp backend was doing. I think it's reasonable to ask for invariant names, but usually the problem is that it's always possible to find some device somewhere where these fields are not filled in or not unique (the infamous 0000000 serial number), which always causes problems with USB devices.

Granted, some devices may violate the specs. On the other hand, I don't think it's going to create a lot of practical problems.

The average user is going to have no more than a few devices, and unless two (or more) of a user's devices report the same buggy identification data and the user has both plugged in at the same time, he/she won't even notice.

Would it be an option to use the USB vendor & device ID rather than the MTP manufacturer and model strings? The USB identification should be correct for most devices (as a bunch more things would break if it weren't) – the only thing I can't say for sure is whether or not the USB tuple corresponds to no more than one MTP Manufacturer/Model tuple.

For the majority of users, just an accurate vendor/model tuple would provide sufficient disambiguation as I expect few users to have two or more identical devices.

In case there's really a collision, i.e. a newly connected device reports the same identification data as a device that is already connected, it's still possible to introduce a counter or similar into the name. For example, the current "usb:002,005" string could be appended in such cases. While this would still leave some issues open for non-compliant devices, it would alleviate the situation for thise devices, and fully resolve it for compliant devices.

At least mtp, ptp and obexftp has this naming convention. I think majority of users don't need this feature. And those who wants to use scripting, they should be advanced enough to write something like to get the correct location:

vendor="<vendor>:<product>"
line=$(lsusb -d $vendor)
location="mtp://[usb:${line:4:3},${line:15:3}]/"

(In reply to Michael von Glasow from comment #2)
> (snap)
> In case there's really a collision, i.e. a newly connected device reports
> the same identification data as a device that is already connected, it's
> still possible to introduce a counter or similar into the name. For example,
> the current "usb:002,005" string could be appended in such cases. While this
> would still leave some issues open for non-compliant devices, it would
> alleviate the situation for thise devices, and fully resolve it for
> compliant devices.

Ok, but when we add "counter" into the name, it breaks the scripts again. But you are right that usb vendor and id pair should be unique...

Thanks for the trick, this would work for scripts that use GVFS URLs. When working with legacy tools that expect a filesystem path, though, one has to resort to gvfs-fuse – and that's quite a different story as MTP paths in their current form are quite an escape hell in gvfs-fuse.

Also, scripts are just one possible use case. Another is using Unison to sync files between mobile phone and PC – or really any tool that just expects a static path. In general, I think it's a good idea to have stable mount paths for removable media – whether these are plain USB drives or MTP devices.

As for adding the counter – the idea is the following: First, assemble a URL from vendor/device/serial and see if it is still available. If it is, use that – if not, append the counter. That way, paths will be reproducible at least as long as there is no collision.

(In reply to Michael von Glasow from comment #4)
> Thanks for the trick, this would work for scripts that use GVFS URLs. When
> working with legacy tools that expect a filesystem path, though, one has to
> resort to gvfs-fuse – and that's quite a different story as MTP paths in
> their current form are quite an escape hell in gvfs-fuse.

I really don't recommend to use fuse daemon e.g. for syncing files (it is just limited fallback), but I don't see problem to generate the correct location (untested):

fuse="/run/user/$UID/gvfs/mtp:host=%5Busb%3A${line:4:3}%2C${line:15:3}%5D/" 

However I would recommend to use gvfs utils like gvfs-copy for scripting...

> Also, scripts are just one possible use case. Another is using Unison to
> sync files between mobile phone and PC – or really any tool that just
> expects a static path. In general, I think it's a good idea to have stable
> mount paths for removable media – whether these are plain USB drives or MTP
> devices.

I agree that it makes sense to use static paths, but I think it isn't so important, because it doesn't affect many people... However we will be really happy if somebody provide patches.

> As for adding the counter – the idea is the following: First, assemble a URL
> from vendor/device/serial and see if it is still available. If it is, use
> that – if not, append the counter. That way, paths will be reproducible at
> least as long as there is no collision.

I see.

I'm moving this to general because it affects multiple backends - I don't see any sense in only changing how the mtp backend works. If the naming scheme needs to be changed, it needs to be changed.

I agree with Ondrej that it's empirically not a substantial problem - as we don't see a continuous stream of complaints.

Finally, do not trivialise the naming failure problem. As an example, note that Google Nexus devices are all 100% identical with respect to usb vid/pid, so uniqueness can only be established from the serial. In google's case, we have serials but as I say, I'm sure there are examples out there where serials are not reliable.

A counter breaks stability in general because, as today, insertion order affects the path.

I've modified GVFS to use the long serial (which includes vendor and model) to build the URI. If multiple MTP devices are found to use the same ID at the time  the URI is being created, the USB bus and device number (which are currently used to build the URI) are appended for disambiguation.

This leads to URIs which are stable (e.g. mtp://SAMSUNG_SAMSUNG_Android_26191497), as long as the device supplies a non-NULL, non-empty ID_SERIAL property which is unique on the system at that time.

Devices which do not supply ID_SERIAL (which, as I understand, is an issue with the Nexus 5) would get a URI like mtp://_004,012. (This is essentially equivalent to mtp://[usb:004,012] but has been altered for consistency.)

In case of an ID collision, determined when the URI is generated, the URI would be of the form mtp://Lemon_Blingphone_00000000_004,015 (assuming the ID_SERIAL is Lemon_Blingphone_00000000 and the device in question is device 015 on bus 004).

Both cases of misbehaving devices would suffer from the same instability we have now, but there is little we can do about that.

The code is at: https://gitlab.com/mvglasow/gvfs/tree/bug744267 (I can submit it as a patch if you prefer it that way).

I've developed on top of 1.24.2 and then rebased onto master, as I use Ubuntu MATE 15.10, which ships with 1.24.2 and the latest version requires changes to the dbus configuration. The code I tested with is at https://gitlab.com/mvglasow/gvfs/tree/1.24.2-bug744267.

Since I don't have access to any "misbehaving" devices, I could only test these cases by modifying conditions in code and making sure the fallback URLs could still be resolved back to the device. This has worked, but it would be helpful if someone could test with a noncompliant device.

I realized that metadata daemon using also those non-stable paths to store metadata for the connected devices, so it would be nice to have stable paths also from this reason...

(In reply to Michael von Glasow from comment #7)
> I've modified GVFS to use the long serial (which includes vendor and model)
> to build the URI. If multiple MTP devices are found to use the same ID at
> the time  the URI is being created, the USB bus and device number (which are
> currently used to build the URI) are appended for disambiguation.
> 
> This leads to URIs which are stable (e.g.
> mtp://SAMSUNG_SAMSUNG_Android_26191497), as long as the device supplies a
> non-NULL, non-empty ID_SERIAL property which is unique on the system at that
> time.
> 
> Devices which do not supply ID_SERIAL (which, as I understand, is an issue
> with the Nexus 5) would get a URI like mtp://_004,012. (This is essentially
> equivalent to mtp://[usb:004,012] but has been altered for consistency.)

I would use the traditional uri, i.e. mtp://[usb:004,012], if it is not possible to generate stable uri...

It would be good to take a look in uri specification, I am not sure whether it doesn't have to be in the brackets in order to be valid uri...

> In case of an ID collision, determined when the URI is generated, the URI
> would be of the form mtp://Lemon_Blingphone_00000000_004,015 (assuming the
> ID_SERIAL is Lemon_Blingphone_00000000 and the device in question is device
> 015 on bus 004).
> 
> Both cases of misbehaving devices would suffer from the same instability we
> have now, but there is little we can do about that.
> 
> The code is at: https://gitlab.com/mvglasow/gvfs/tree/bug744267 (I can
> submit it as a patch if you prefer it that way).

Please attach the patches to the bugzilla...

> I've developed on top of 1.24.2 and then rebased onto master, as I use
> Ubuntu MATE 15.10, which ships with 1.24.2 and the latest version requires
> changes to the dbus configuration. The code I tested with is at
> https://gitlab.com/mvglasow/gvfs/tree/1.24.2-bug744267.
> 
> Since I don't have access to any "misbehaving" devices, I could only test
> these cases by modifying conditions in code and making sure the fallback
> URLs could still be resolved back to the device. This has worked, but it
> would be helpful if someone could test with a noncompliant device.

Same change has to be done for gphoto2 backend/monitor also if we accept this...

Philip, what are you thinking about?

I agree that the fallback behaviour should be the same URIs we have today - don't see a reason to change that.

What I was thinking was whether we can extract the physical 'path' from udev.

The 'device number' is this monotonically increasing number that is annoying when it changes on a replug (or a reboot, or a suspend/resume). Fair enough.

Internally, there's also a concept of a port number at each level in the USB tree. These values are stable for any device plugged into a particular port. It's not unique to a device, to be sure, but it's stable if you use the same port. I wonder if this gets us sufficient stability without having to worry about lots of edge cases where one identifier is present and another isn't etc.

For example:

philipl@fido6:/dev$ udevadm info $PWD/libmtp-3-1
P: /devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:02.0/0000:09:00.0/usb3/3-1
N: bus/usb/003/003
S: libmtp-3-1
E: BUSNUM=003
E: COLORD_DEVICE=1
E: COLORD_KIND=camera
E: DEVLINKS=/dev/libmtp-3-1
E: DEVNAME=/dev/bus/usb/003/003
E: DEVNUM=003
E: DEVPATH=/devices/pci0000:00/0000:00:1c.4/0000:05:00.0/0000:06:02.0/0000:09:00.0/usb3/3-1
E: DEVTYPE=usb_device
E: DRIVER=usb
E: GPHOTO2_DRIVER=PTP
E: ID_BUS=usb
E: ID_FOR_SEAT=usb-pci-0000_09_00_0-usb-0_1
E: ID_GPHOTO2=1
E: ID_MEDIA_PLAYER=1
E: ID_MODEL=Nexus_6P
E: ID_MODEL_ENC=Nexus\x206P
E: ID_MODEL_FROM_DATABASE=Nexus Device (debug)
E: ID_MODEL_ID=4ee2
E: ID_MTP_DEVICE=1
E: ID_PATH=pci-0000:09:00.0-usb-0:1
E: ID_PATH_TAG=pci-0000_09_00_0-usb-0_1
E: ID_REVISION=0310
E: ID_SERIAL=Huawei_Nexus_6P_8XV7N15A17001776
E: ID_SERIAL_SHORT=8XV7N15A17001776
E: ID_USB_INTERFACES=:ffff00:ff4201:
E: ID_VENDOR=Huawei
E: ID_VENDOR_ENC=Huawei
E: ID_VENDOR_FROM_DATABASE=Google Inc.
E: ID_VENDOR_ID=18d1
E: MAJOR=189
E: MINOR=258
E: PRODUCT=18d1/4ee2/310
E: SUBSYSTEM=usb
E: TAGS=:uaccess:seat:
E: TYPE=0/0/0
E: USEC_INITIALIZED=2713680312

You can see in the DEVPATH (and in fact in the DEVLINKS symlink) that this device is plugged into port "3-1" which is the first port on the third root hub. Another device might be "1-4.2.2" which is connected to the second port of a hub connected to the second port of another hub, connected to the fourth port on the first root hub.

TL;DR:
* +1 for physical path
* Having device serials, even ambiguous ones, in the path is better for usability
* Brackets, as in [usb:002,004] are illegal in URIs and come at usability drawbacks, thus I'd advocate dropping them
* I'd favor URIs of the form
     mtp-uri = "mtp://" ( id-serial / ( [ id-serial ] "@" usb-port ) )
  with <id-serial> mandatory where available and the second form used only where <id-serial> is unavailable or ambiguous
* How do I get the "@" using g_mount_spec_get()?
* Is the list of backends exhaustive?
* Where should the code go? commons/gvfsgphoto2utils.c?


Long answer:


I'd definitely prefer the physical path to the traditional one for disambiguation. Their strict association with a physical port and survival across a replug is a huge advantage. I'll see how I can implement that.

As for ambiguous serials, the advantage of having them in the URI is that it makes the URI more meaningful. Consider the following use case:

1. You plug in a Lemon_Blingphone_00000000. At that time the ID is unique on the system, thus the URI assigned is mtp://Lemon_Blingphone_00000000.
2. You plug in another Lemon_Blingphone_00000000. Now the ID is no longer unique. Depending on implementation, the URI assigned is either:
(a) mtp://Lemon_Blingphone_00000000_1-4 or mtp://Lemon_Blingphone_00000000_001,002
(b) mtp://1-4 or mtp://[usb:001,002]

Assume that after step 2, you try to access the phone from the command line. You type

   /var/run/user/1000/gvfs/Lemon<TAB>

and in scenario (a), you get something like

   Lemon_Blingphone_00000000     Lemon_Blingphone_00000000_1-4

while in (b), the shell will autocomplete Lemon_Blingphone_00000000. While in (a) you are alerted to the presence of two similar devices, in (b) you may end up working on the wrong one. (Things are similar when browsing through files in a non-GVFS-aware application.)


As for the URI specification, I assume you are referring to RFC3986, Appendix A (https://tools.ietf.org/html/rfc3986#appendix-A), which has:

   URI           = scheme ":" hier-part [ "?" query ] [ "#" fragment ]

   hier-part     = "//" authority path-abempty   ; begins with "/" or is empty
                 / path-absolute                 ; begins with "/" but not "//"
                 / path-rootless                 ; begins with a segment
                 / path-empty                    ; zero characters

   authority     = [ userinfo "@" ] host [ ":" port ]
   host          = IP-literal / IPv4address / reg-name
   IP-literal    = "[" ( IPv6address / IPvFuture  ) "]"
   IPvFuture     = "v" 1*HEXDIG "." 1*( unreserved / sub-delims / ":" )
   reg-name      = *( unreserved / pct-encoded / sub-delims )

   segment       = *pchar
   segment-nz    = 1*pchar
   segment-nz-nc = 1*( unreserved / pct-encoded / sub-delims / "@" )
                 ; non-zero-length segment without any colon ":"

   path-abempty  = *( "/" segment )
   path-absolute = "/" [ segment-nz *( "/" segment ) ]
   path-noscheme = segment-nz-nc *( "/" segment )
   path-rootless = segment-nz *( "/" segment )
   path-empty    = 0<pchar>

   pchar         = unreserved / pct-encoded / sub-delims / ":" / "@"

   pct-encoded   = "%" HEXDIG HEXDIG
   unreserved    = ALPHA / DIGIT / "-" / "." / "_" / "~"
   reserved      = gen-delims / sub-delims
   gen-delims    = ":" / "/" / "?" / "#" / "[" / "]" / "@"
   sub-delims    = "!" / "$" / "&" / "'" / "(" / ")"
                 / "*" / "+" / "," / ";" / "="

Judging from the code in gvfsbackendmtp.c, the device ID (of whatever form we choose) is treated as if it were a <host>. Since it is obviously not an IP address of any kind, it would thus be a <reg-name>. The latter can be made up of literal characters from the <unreserved> and <sub-delims> category, or any <pct-encoded> character. However, "[", "]" and ":" are in neither of the first two categories and would thus be illegal in a <host> of the <reg-name> form.

Alternatively, it could be considered a <path-rootless>, which is essentially a concatenation of <segment>s separated with "/" and must start with a non-empty <segment>, i.e. <segment-nz>. The latter allows the same character set as <reg-name>, plus ":" and "@".

Either way, the brackets are illegal but gvfs's parser is letting that slip. I'd be happy to see them go, as they hamper legibility of path names in the file system:

   mtp:host=%5Busb%3A002%2C007%5D

From a design point of view, I'd treat the device ID as a <path-nz> and use a literal "@" to denote the USB port:

   mtp://SAMSUNG_SAMSUNG_Android_26191497
   mtp://Lemon_Blingphone_00000000@1-4 or mtp://Lemon_Blingphone_00000000@002,004
   mtp://@1-4 or mtp://@002,004

I'd just need some help on retrieving that string (or its subcomponents), as

   g_mount_spec_get (mount_spec, "host");

will drop everything up to the first "@" (including the "@" itself, thus I'll never know if I'm dealing with a device serial or a reference to a USB port).


If this code is going to be required by multiple backends, it should probably go somewhere in "common". Are there any other backends beyond MTP, PTP and Obex which would be affected? Where should code shared by these backends go – would the existing "commons/gvfsgphoto2utils.c" be the place to put it?

Sorry for the delay...

(In reply to Michael von Glasow from comment #10)
> TL;DR:
> * +1 for physical path
> * Having device serials, even ambiguous ones, in the path is better for
> usability
> * Brackets, as in [usb:002,004] are illegal in URIs and come at usability
> drawbacks, thus I'd advocate dropping them

Hmm, I thought this is somehow valid, good to know, but still I prefer the original one from the reasons mentioned earlier...

mtp://[usb:002,004] seems better to me than mtp://@002,004

> * I'd favor URIs of the form
>      mtp-uri = "mtp://" ( id-serial / ( [ id-serial ] "@" usb-port ) )
>   with <id-serial> mandatory where available and the second form used only
> where <id-serial> is unavailable or ambiguous

I would use original uri in case, when id-serial is unusable/ambiguous. Anyway I expect that people don't use more mtp devices connected in one time often and I don't expect that people have multiple devices with same id-serial, but who knows... 

> * How do I get the "@" using g_mount_spec_get()?

Conseqently this is not needed. But see client/gdaemonvfs.c:get_mountspec_from_uri(). I suppose part before @ is decoded as "user" and port is decoded as "host"... so some custom uri parser would be needed probably, see afpuri.c, smburi.c, httpuri.c...

> * Is the list of backends exhaustive?

Obexftp was removed, so only gphpoto2 backend... there is also afc, but it uses some kind of uuid in uri... maybe would be good to investigate afc volume monitors codes also, but it seems it doesn't care about possible ambiguity...

> * Where should the code go? commons/gvfsgphoto2utils.c?

That's good place...

If you're looking for more users requesting this then add me to the list. I wanted to play music stored on my phone. I initially tried several players that didn't support GVFS but the volatile path meant that the media library would break each time. It took me a while to find one that didn't resolve symlinks. I thought changing to a player that did support GVFS (like XMMS2) would fix this annoyance but I was surprised to find that the mtp:// URL is just as volatile as the real path.

(In reply to Ondrej Holy from comment #3)
> At least mtp, ptp and obexftp has this naming convention.

MTP and PTP both have URLs deduced from USB bus and device numbers which will change depending on when and how the devices were plugged in.

ObexFTP was using the device's unique Bluetooth address, the USB support was never finished. Seeing as ObexFTP has been removed anyway, remove it from the title.

Created attachment 370270 [details] [review]
Switch to a stable device URI

I uploaded this patch to another related bug a couple of months ago, but i guess it got lost in the noise.

This implementation uses the physical port as the ID as I discussed previously - so it doesn't depend on any device information and will always work. Use the same port, get the same Id every time.

(In reply to Philip Langdale from comment #14)
> Created attachment 370270 [details] [review] [review]
> Switch to a stable device URI
> 
> I uploaded this patch to another related bug a couple of months ago, but i
> guess it got lost in the noise.

Sorry for that, please ping me in such cases.

> This implementation uses the physical port as the ID as I discussed
> previously - so it doesn't depend on any device information and will always
> work. Use the same port, get the same Id every time.

This can definitely improve some use-cases but still, it doesn't solve for example the issue with metadata mentioned in Comment 8 :-( Just a note that I personally do not plug USB cable into the same port every time...

I see the reasons why you want to avoid using something device-specific, but I would still be happier if ID_SERIAL could be used. It should be unique as per spec, it isn't reliable as said before, but on the other hand, I would say that it is pretty small chance that somebody will connect more devices with the same serial... isn't it? So maybe I would simply ignore newly connected devices with the same serial in the volume monitor (manual mounting over mtp://[usb:...] would be still possible) and mount random one, or simply abort mounting if not yet mounted. Because I don't think we can reliably handle this case just by adding some suffix as suggested before. What do you think?

Created attachment 370635 [details] [review]
Switch to a stable device URI v2

Updated diff that uses ID_SERIAL and refuses to mount a second device if the ID_SERIAL value is identical to an existing one.

Review of attachment 370635 [details] [review]:

Thanks, that's more-or-less what I have in my mind! Although it is not applicable to master due to commit 7a6fd7b, but I made some tests anyway and it seems that I have some issues with (auto)mounting and force unmounting, I have to make more tests and will let you know...

Do I understand correctly that kernel ensures that ID_SERIAL is always set to something? I have expected that fallback for devices without ID_SERIAL will be needed... but this would be better.

It would be nice to make the same changes for gphoto2 backend and volume monitor once we agree on the solution for mtp. My camera also has ID_SERIAL set, so hope it is possible for gphoto2 also...

::: daemon/gvfsbackendmtp.c
@@ +576,3 @@
 on_uevent (GUdevClient *client, gchar *action, GUdevDevice *device, gpointer user_data)
 {
+  const char *dev_path = g_udev_device_get_property (device, "ID_SERIAL");

dev_path -> serial_id?

But not sure that ID_SERIAL is always set at this point, which is maybe the reason why force unmount doesn't work for me currently, maybe it is similar to Bug 789491. So maybe it would be better to use still device path internally here instead if ID_SERIAL.

@@ +845,3 @@
+  devices = g_udev_client_query_by_subsystem (gudev_client, "usb");
+  l = devices;
+  while (l != NULL) {

Why not use for (l = devices; l != NULL; l = l->next) {... ?

@@ +848,3 @@
+    GList *next = l->next;
+    const char *id = g_udev_device_get_property (l->data, "ID_SERIAL");
+    if (g_strcmp0(id, host) == 0) {

nitpick: missing space before opening parenthesis, it applies also on the following lines

@@ +849,3 @@
+    const char *id = g_udev_device_get_property (l->data, "ID_SERIAL");
+    if (g_strcmp0(id, host) == 0) {
+      *device = l->data;

*device = g_object_ref (l->data);

@@ +853,3 @@
+                                  NULL, 10);
+      *dev_num = g_ascii_strtoull(g_udev_device_get_property (l->data, "DEVNUM"),
+                                  NULL, 10);

break;

...and remove the following code up to g_list_free_full.

@@ +862,2 @@
   }
+  g_list_free (devices);

g_list_free_full (devices, g_object_unref);

@@ -860,2 @@
-  /* find corresponding GUdevDevice */
-  *device = g_udev_client_query_by_device_file (gudev_client, dev_path);

I am not really sure that we should not keep supporting the previous uri format as a fallback (for compatibility and debug purposes)...

@@ +870,3 @@
   }
 
+  return g_strdup (host);

It doesn't make sense to return input parameter... but I would return dev_path anyway, store in backend->dev_path and consequently use in on_uevent() and get_device() functions...

::: monitor/mtp/gmtpvolumemonitor.c
@@ +150,3 @@
+  if (usb_serial_id == NULL) {
+    g_warning ("device %s has no ID_SERIAL property, ignoring",
+               g_udev_device_get_device_file (device));

Please see Bug 793925, I suppose this needs to be modified a bit in order to not fill logs by this warning (or the following one) for some unsupported children devices...

Created attachment 370828 [details] [review]
Switch to a stable device URI v3

Updated to reflect comments. I've adjusted to account for commit 7a6fd7b and kept the internal dev_path the same.

I'd much rather avoid keeping compatibility code if at all possible. I recognise there's a scenario where you'd want it (monitor doesn't get restarted and then you plug in a new device) but that seems a sufficient edge case to me...

Review of attachment 370828 [details] [review]:

(In reply to Philip Langdale from comment #18)
> I'd much rather avoid keeping compatibility code if at all possible. I recognize there's a scenario where you'd want it (monitor doesn't get restarted and then you plug in a new device) but that seems a sufficient edge case to me...

That's exactly when it perfectly makes sense to have the fallback code, but this won't go into stable branch, so the most people will get this after system update which is not possible without restarting, so probably ok... but still, I am just a bit shy to change the uri format without fallback. This could be used in cases when somebody plug more devices with the same serial, so he could manually mount the device if really needed, or so...

nitpick: If you don't want compatibility code, then please rather bump required libmtp version to 1.1.12 at least and remove relevant HAVE_LIBMTP_* macros...

> 1: Modern Android devices actually trigger a replug event when you
>    switch them into File Transfer mode, so if you, or your file
>    manager opens a window before switching modes, that window
>    ends up being useless.

Still, the volume and mount is removed and added, which is not optimal.

> Note that this does not perfectly fix problem 1) because there's
> no event triggered which causes Nautilus to refresh the device
> window. If you manually refresh, you will see the contents.

Please file bug against nautilus after pushing this patch. Nautilus is aware of the mount/volume change, so it should refresh the folder if it is shown...

::: daemon/gvfsbackendmtp.c
@@ +836,3 @@
                                    const char *host,
+                                   uint32_t *bus_num,
+                                   uint32_t *dev_num,

nitpick: Those params are probably redundant, backend->dev_path could be used in get_device, but if you want them I am not against...

@@ +859,1 @@
   if (!*device) {

You have to initialize *device to NULL at first, otherwise, it may be undefined...

Created attachment 370918 [details] [review]
Switch to a stable device URI v4

(In reply to Ondrej Holy from comment #19)
> 
> That's exactly when it perfectly makes sense to have the fallback code, but
> this won't go into stable branch, so the most people will get this after
> system update which is not possible without restarting, so probably ok...
> but still, I am just a bit shy to change the uri format without fallback.
> This could be used in cases when somebody plug more devices with the same
> serial, so he could manually mount the device if really needed, or so...

I've put in compatibility logic. I buy the argument.
 
> nitpick: If you don't want compatibility code, then please rather bump
> required libmtp version to 1.1.12 at least and remove relevant HAVE_LIBMTP_*
> macros...

I'll do this in a separate change.

> > 1: Modern Android devices actually trigger a replug event when you
> >    switch them into File Transfer mode, so if you, or your file
> >    manager opens a window before switching modes, that window
> >    ends up being useless.
> 
> Still, the volume and mount is removed and added, which is not optimal.

It is not optimal. I've talked to the Google engineer who owns this, and he says that it's too big of a behaviour to change for Android P, but he'll try and do something for Q. Apparently there is Android Auto hardware (ie: cars) that will fail if the phone doesn't advertise MTP support when in charging-only mode. And it's too big a task at this point to implement the old-style behaviour where there is a single MTP mode with the storages being hidden until file transfer mode is turned on. So we're stuck for a while.

> > Note that this does not perfectly fix problem 1) because there's
> > no event triggered which causes Nautilus to refresh the device
> > window. If you manually refresh, you will see the contents.
> 
> Please file bug against nautilus after pushing this patch. Nautilus is aware
> of the mount/volume change, so it should refresh the folder if it is shown...

I will do so.

> ::: daemon/gvfsbackendmtp.c
> @@ +836,3 @@
>                                     const char *host,
> +                                   uint32_t *bus_num,
> +                                   uint32_t *dev_num,
> 
> nitpick: Those params are probably redundant, backend->dev_path could be
> used in get_device, but if you want them I am not against...

I prefer this as it avoids having to do additional string parsing on the dev_path. We've just gone to a lot of trouble to isolate the numbers, so I don't want to have to parse again.

> @@ +859,1 @@
>    if (!*device) {
> 
> You have to initialize *device to NULL at first, otherwise, it may be
> undefined...

Fixed.

Thanks!

Review of attachment 370918 [details] [review]:

Thanks, it works correctly to me, feel free to push with the corrected comment (see below).

Will you propose also a similar update for gphoto2, please?

::: daemon/gvfsbackendmtp.c
@@ +865,3 @@
+  /* For compatibility, handle old style host specifications. */
+  if (g_str_has_prefix (host, "[usb:")) {
+    /* Split [usb:001,002] into: '[usb:', '001', '002', ']' */

'[usb', '001', '002', ''

@@ +866,3 @@
+  if (g_str_has_prefix (host, "[usb:")) {
+    /* Split [usb:001,002] into: '[usb:', '001', '002', ']' */
+    char **elements = g_strsplit_set (host, ":,]", -1);

nitpick: This allows various malformed URIs, but the previous code also allowed some sort of malformed URIs and this is just a fallback, so ok...

@@ +867,3 @@
+    /* Split [usb:001,002] into: '[usb:', '001', '002', ']' */
+    char **elements = g_strsplit_set (host, ":,]", -1);
+    if (g_strv_length (elements) == 4) {

...but maybe at least: "&& element[3][0] == '\0'"

Pushed.

I added your suggested validation, plus I reordered the code to check that the strtoull calls succeeded before proceeding.

I will put together an equivalent change for gphoto2 (with a new tracking bug) and remove the old libmtp support for < 1.1.12

Thanks a lot for all your work here!

Nautilus issue: https://gitlab.gnome.org/GNOME/nautilus/issues/371