I'd like to give this a shot, but can't make a promise as to when I'll find time for this. I'll start with bug 586409 first.

Created attachment 137109 [details] [review]
initial porting of backend to gudev

This is an initial port of the gphoto2 backend to gudev. It builds on top of the cdda gudev port in bug 586409 and behaves similarly: If both hal and gudev are available, gudev is preferred, but it works with either.

The naming isn't perfect yet, since in current libgphoto2's udev rules the gphoto2 name is not exported. This can be fixed easily, I'll care about that.

Of course this still leaves the gphoto2 volume monitor, I didn't look into that yet.

Got it working now, will do some more testing.

Created attachment 137754 [details] [review]
port gvfs gphoto backend to gudev

Updated initial backend patch to git head, and in git format-patch form.

Created attachment 137755 [details] [review]
port gvfs gphoto volume monitor to gudev

This patch ports the gphoto monitor to gudev. Again, if both udev and hal are available at build time, it prefers udev. Unfortunately it became a real #ifdef hell, but I guess we need to keep around the hal bits for a while, until it's clearer what happens to non-Linux platforms.

automount and autounmount, gvfs-ls, gvfs-mounts -il, etc. all work fine. nautilus opens as well, and I quickly see the "digital camera" ribbon, but then it immediately crashes. However, it also crashes in exactly the same way with gvfs 1.3.1 without all my patches, so I don't think that's a regression due to this patch. 

However, I'd really appreciate testing by someone else as well before this gets pushed. While this is a relatively straightforward port, it is pretty intrusive due to sheer size.

Thanks for considering! Martin

Ah, I found the crash, was a bad nautilus extension. Working perfectly now.

Created attachment 137767 [details] [review]
port gvfs gphoto volume monitor to gudev

Improved patch:

 * Now doesn't reimplement the entire update_cameras() function, but just the chaned bits. More #ifdefs, but much more shared code, so in the end I think this is better to maintain.

 * Make #ifdef style consistent

 * Add g_return_val_if_fail() check to g_gphoto2_volume_new()

If you wonder about this single removal, this was dead code and thus not necessary to #ifdef away:

@@ -595,10 +722,13 @@ update_cameras (GGPhoto2VolumeMonitor *monitor,
           activation_mount_root = g_file_new_for_uri (uri);
           g_free (uri);

-          udi = hal_device_get_udi (d);

Hi,

Thanks for doing this work!

I think it would probably make the code more readable if you upped the granularity of #ifdeffing. Suggest to leave all the hal code alone and just add new monitor code for the gudev case.

It's worth pointing out somewhere that this won't work unless you have the ID_GPHOTO2 patch for libgphoto2. Any chance you can persuade the gphoto2 developers to do an official release with this? Then we can require that version in configure.in... 

Alternatively (but please try and ask the gphoto2 devs anyway), we need to leave a note in configure.in that this patch is needed - including a pointer where to get the patch....

Btw, are we guaranteed that $ENV{ID_GPHOTO2}=="1" for PTP cameras? If not, we need to do something like (strstr ($ENV{USB_ID_INTERFACES}, ":060101") != NULL) as well....

We're also missing the bits for daemon/gvfsbackendgphoto2.c - do you plan to add this as well?

> +#ifdef HAVE_GUDEV
> +  const char *subsystems[] = {"usb", NULL};
> +#endif

> +#ifdef HAVE_GUDEV
> +  new_camera_devices = g_udev_client_query_by_subsystem (monitor->gudev_client, "usb");

I think we can use "usb/usb_interface" here since we are only interested in devices where subsystem=="usb" and $ENV{DEVTYPE}=="usb_interface".

It's not clear to me we would be doing the right thing for PTP-based music players here - e.g. pick the right icon and name. This is probably pending hashing out the way media-player-id is going to work, right?

I also think the patch should include a patch to configure.in such that

 - if we have both libgudev and libgdu, don't automatically enable hal unless
   --enable-hal=yes is passed

 - otherwise, use hal if found unless --enable-hal=no is passed

 - print whether we have GUdev support (in the end of configure.in)

E.g. what we want to achieve is that we don't automatically hal stuff if we have both libgdu and libgudev.... that is, unless --enable-hal is passed.

> I think it would probably make the code more readable if you upped the
> granularity of #ifdeffing. Suggest to leave all the hal code alone and just add
> new monitor code for the gudev case.

OK, can do. Then I'll drop all the internal bookkeeping as well and just directly react to add/remove events.

> Any chance you can persuade the gphoto2 developers to do an official release with this?

Sure, I'll ping Marcus and point him to this bug.

> We're also missing the bits for daemon/gvfsbackendgphoto2.c - do you plan to
> add this as well?

Already done, patch is attached here (second in the list).

> It's not clear to me we would be doing the right thing for PTP-based music
> players here - e.g. pick the right icon and name. This is probably pending
> hashing out the way media-player-id is going to work, right?

Indeed, right now the icon name is fixed, since we don't have a better way to tell. I dropped the distinction between PTP and MTP, since this isn't really reflected in current libgphoto rules; they just say "PTP" for both, I think. I'm afraid I don't currently have a good answer for this.

> - if we have both libgudev and libgdu, don't automatically enable hal unless
>   --enable-hal=yes is passed

Not sure what libgdu should do here, but the patch is designed to prefer gudev ofer hal if both are available. That's why it always looks like "#ifdef HAVE_GUDEV ... #else ... (hal code) #endif". configure.ac ensures that at least one must be true, and as a fallback I'm also testing this in the first hunk ofggphoto2volume.h (#error Needs gudev or hal).

Perhaps I misunderstood you, do you want the code to behave different to what it does right now?

> E.g. what we want to achieve is that we don't automatically hal stuff if we
> have both libgdu and libgudev.... that is, unless --enable-hal is passed.

Hm, I don't quite see that it can work sensibly with _both_ at the same time, since you'd get all events twice? IMHO it's better to use and prefer gudev (and only that) if we have it available.

Thanks for the review!

> - print whether we have GUdev support (in the end of configure.in)

Whoops, good point. This should have happened in the cdda patch, will do in the fixed one for this.

Created attachment 138100 [details] [review]
port gvfs gphoto backend to gudev

The recent mega-patch in git head caused some conflicts. This is the gphoto2 backend patch rebased to current git head.

(In reply to comment #10)
> It's not clear to me we would be doing the right thing for PTP-based music
> > players here - e.g. pick the right icon and name. This is probably pending
> > hashing out the way media-player-id is going to work, right?
> 
> Indeed, right now the icon name is fixed, since we don't have a better way to
> tell. I dropped the distinction between PTP and MTP, since this isn't really
> reflected in current libgphoto rules; they just say "PTP" for both, I think.
> I'm afraid I don't currently have a good answer for this.

The current hal-based monitor does this and it would be a shame to lose this feature. I can also imagine lots of people complaining / filing bugs about this.

So we should get that into the media-player-id project and get that project into the distros (and perhaps make media-player-id ship a pkgconfig file so we can fail if it's not there). Also want a way to set the icon/name from the media-player-id metadata as requested here

http://lists.freedesktop.org/archives/devkit-devel/2009-July/000311.html

so it's possible to override on a per-model basis. It would probably make the code a lot simpler if we also exported the name/icons in udev attributes, saves looking up a file. I don't care much either way (actually I do care a little since the gdu volume monitor needs to do the sname) so up to you and teuf I suppose.

Anyway, a little polish like good icons/names goes a long way.

> Perhaps I misunderstood you, do you want the code to behave different to what
> it does right now?

Basically I want GVfs to do the "right thing" without having to pass any configure options - this is important since many people build gvfs via jhbuild so it needs to do the right thing out of the box.

It seems it already does this - expect for one nitpick that HAVE_HAL is set to 1 and e.g. the hal monitor is also built. That would be good to get rid of but it's not super important either. I mean, in Fedora we have libhal in the buildroot so we'll catch the errors (e.g. where new code depends on libhal) either way.

So it's probably fine the way things work now - carry on the good work!

    David

(In reply to comment #13)
> I mean, in Fedora we have libhal in the
> buildroot so we'll catch the errors (e.g. where new code depends on libhal)
> either way.

This should read "don't have libhal" instead "have libhal". Sorry, I do this all the time :-/

Note to self: 
 - Use ID_MODEL_ENC instead of ID_MODEL.
 - David prefers one patch instead of two for review.

I noticed two unanswered bits here:

> Btw, are we guaranteed that $ENV{ID_GPHOTO2}=="1" for PTP cameras? If not, we
> need to do something like (strstr ($ENV{USB_ID_INTERFACES}, ":060101") != NULL)
> as well....

Yes, we are:

$ grep PTP /lib/udev/rules.d/40-libgphoto2-2.rules
ENV{ID_USB_INTERFACES}=="*:060101:*", ENV{ID_GPHOTO2}="1", ENV{GPHOTO2_DRIVER}="PTP", MODE="0664", GROUP="plugdev"

since libgphoto2 has generic support for all PTP devices. (Nevermind the MODE="0664", GROUP="plugdev", we have that for hysterical raisins, and you can tell the generator script not do do that).

> > +#ifdef HAVE_GUDEV
> > +  new_camera_devices = g_udev_client_query_by_subsystem (monitor->gudev_client, "usb");

> I think we can use "usb/usb_interface" here since we are only interested in
> devices where subsystem=="usb" and $ENV{DEVTYPE}=="usb_interface".

Oh, that's an undocumented trick? Anyway, as I said I'd rather throw away update_cameras() completely for the gudev implementation and just directly react to added and removed cameras in on_uevent(). Should make the code much simpler and avoids iterating over all usb devices, when we already know exactly which device changed. As discussed, this was done in the old hal days when  a device removal event wouldn't give us its properties.

(In reply to comment #16)
> > I think we can use "usb/usb_interface" here since we are only interested in
> > devices where subsystem=="usb" and $ENV{DEVTYPE}=="usb_interface".
> 
> Oh, that's an undocumented trick? 

Actually it's documented

 http://www.kernel.org/pub/linux/utils/kernel/hotplug/gudev/GUdevClient.html#g-udev-client-new

which references the GUdevClient::subsystems property

 http://www.kernel.org/pub/linux/utils/kernel/hotplug/gudev/GUdevClient.html#GUdevClient--subsystems

but right now it only works when constructing a GUdevClient. I should probably make it work for

http://www.kernel.org/pub/linux/utils/kernel/hotplug/gudev/GUdevClient.html#g-udev-client-query-by-subsystem

as well.

(FWIW, to compare with libudev, in general libgudev lacks something like

 g_udev_client query_by_subsystem_and_attributes (
   GUdevClient *client,
   const gchar *subsystem,
   const gchar *first_attr_name,
   const gchar *first_attr_value,
   ..)

with a %NULL-terminated argument list (API similar to e.g. g_object_get()). The problem is that this kind of API, varargs functions, while perfect for C, isn't very bindable to things like Javascript. The solution is probably to add helper object, I don't know. Either way, it's not a big deal not having this, clients can just filter themselves - it's just a bit slower but not much.)

> Anyway, as I said I'd rather throw away
> update_cameras() completely for the gudev implementation and just directly
> react to added and removed cameras in on_uevent(). Should make the code much
> simpler and avoids iterating over all usb devices, when we already know exactly
> which device changed. As discussed, this was done in the old hal days when  a
> device removal event wouldn't give us its properties.

Sure.

For the record (just discussed, but might be useful later on), "usb/usb_interface" doesn't work, since ID_MODEL, ID_GPHOTO2, device node etc. are all on the device, not the interface.

Created attachment 138233 [details] [review]
port gphoto2 backend and monitor to gudev

I updated the patch according to your feedback:

- Merged backend and monitor patches into one patch

- Lower #ifdef granularity in monitor; before I was optimizing for code reuse, now it only #ifdef's away the hal bits (which you can't build without libhal) and adds completely new udev code. The code structure got much simpler now, since I replaced the internal added/removed_devices bookkeeping (it's not necessary with gudev)

- Support for ID_MEDIA_PLAYER* attributes for icons and naming

- Use ID_*_ENC and add a decoding function. I'll discuss that with Kay, and commit it to libudev/libgudev directly, since it's useful for other programs as well. But let's make this work with existing releases for now.

- configure now warns about libgphoto < 2.4.7 and points to the patch. I contacted Marcus (you were CC'ed), 2.4.7 should be released "soon", but not immediately.

- spotted some small bugs in previous patches, fixed

Created attachment 138235 [details] [review]
configure.ac: Show configured hotplug backend

This patch addresses the review comment about showing gudev/hal in configure summary. With both libgudev and libhal this shows:

------------ 8< --------------
gvfs configuration summary:

        gio module directory : /usr/lib/gio/modules

        hotplug backend:              gudev

        FTP/HTTP/WebDAV support       yes
        [...]
------------ 8< --------------

With --disable-gudev (or libgudev not installed):

        hotplug backend:              hal

With neither, it shows "none".

Created attachment 138237 [details] [review]
don't build hal monitor when building gdu monitor

Since all the recent patches prefer gudev over hal, the hal monitor should not be built if gdu is used, even if libhal-dev is available.

When configuring with --disable-gdu (or not having libgudev/libgdu), it builds hal stuff, as before.

This should have been done in the first gudev patch (cdda), but was missed there. But since the cdda bug is closed, and it came up here as a side review comment, I attach it here.

These three patches should now addresses all review comments. Please let me know if you spot any further warts.

Thank you!

Hm, this was working perfectly a few days ago, but with current gvfs this fails: removing the USB host does not cause termination of the gvfsd-gphoto2 process any more, and devices get ugly names. I tracked this down to what looks like a recent regression in g_mount_spec_get().

I attach the camera, it ends up as bus 001, device 015:

$ l /dev/bus/usb/001/015
crw-rw-r--+ 1 root plugdev 189, 14 2009-07-16 16:40 015

$ lsusb|grep Canon
Bus 001 Device 015: ID 04a9:3073 Canon, Inc. PowerShot A70 (ptp)

But the gphoto monitor gets told that it's device 013:

initing 0x15b3010
try_mount 0x15b3010
  host=[usb:001,013]
do_mount 0x15b3010
  host='[usb:001,013]'
  decoded host='usb:001,013'
Parsed 'usb:001,013' into device name /dev/bus/usb/001/013
-> did not find matching udev device

That's why 

  gphoto2_backend->udev_device = g_udev_client_query_by_device_file (gphoto2_backend->gudev_client, devname);

returns NULL, and above problems happen. I'm not quite sure what happens here, the hal code used exactly the same approach.

Oh, forgot to mention, 

  $ gphoto2 --auto-detect
  Canon PowerShot A70 (PTP)      usb:001,015

is correct as well.

When I revert the recent "Interaction when unmounting mounts and misc fixes" commit (8af5a558), things work again.

Since that regression also happens with the original gvfs code and the hal backend, and it seems orthogonal to this gudev port, I moved this to a new bug 588786.

I believe that the attached patches here are still fine.

Created attachment 138541 [details] [review]
port gphoto2 backend and monitor to gudev

Ah, please ignore me, this was apparently a red herring:

    g_debug ("gudev_add_camera: %s %i", 
            g_udev_device_get_property(device, "DEVNUM"), 
            g_udev_device_get_property_as_int (device, "DEVNUM"));

(process:19482): GVFS-GPhoto2-DEBUG: gudev_add_camera: 022 18

g_udev_device_get_property_as_int() is clever about octal numbers,
and the previous tries just happened to have invalid octal numbers (such as
'8').

This updated patch avoids the usage of _as_int() and just uses atoi() itself. Works fine now.

I've pushed this patch to master. We might want to revisit naming/icons but I think it looks good. Many thanks for your work on this Martin!

http://git.gnome.org/cgit/gvfs/commit/?id=12c9c9ae561f9c572ec137d78c6da3bf9ba520bf

Created attachment 141671 [details] [review]
don't build hal monitor when building gdu monitor

It seems that the patch from comment 21 got accidentally deleted, attaching again. It was independent from the actual porting, thus I kept it as a separate patch.

Comment on attachment 138235 [details] [review]
configure.ac: Show configured hotplug backend

The "Show configured hotplug backend" isn't obsolete either. David asked me to do this, I think it also should be applied.

Sorry for the previous confusion, the attachments weren't actually removed, just hidden in the updated bz.

Just talked to David about the two straggling patches here: 

00:01:39     pitti | davidz: is it okay for you if I just commit those?                                     
00:02:06    davidz | pitti: hey                                 
00:02:10    davidz | pitti: sure, go for it                     

So I committed those now.