GNOME Bugzilla – Bug 606194
PATCH: Call the cleanup (finalize) function of backends on unmount
Last modified: 2010-01-12 12:38:37 UTC
Created attachment 150887 [details] PATCH: Call the cleanup (finalize) function of backends on unmount PATCH: fix gvfsdeamon backend cleanup function not being called on unmount gvfs does not properly call the finalize function of backends, due to a missing unref call. This causes the cleanup functions of the libraries underlying the backends to not get called. In case of the gphoto2 backend, this causes the kernel driver for dual mode webcams (which have a kernel space webcam driver and a userspace stillcam driver), to not get re-attached to the device when then the gvfs mount gets unmounted. This patch fixes this by adding a g_object_unref (job) to g_vfs_daemon_initiate_mount, which is needed as g_vfs_daemon_queue_job takes a reference itself.
(In reply to comment #0) > In case of the gphoto2 backend, this causes the kernel driver for dual > mode webcams (which have a kernel space webcam driver and a userspace > stillcam driver), to not get re-attached to the device when then the gvfs > mount gets unmounted. Not opposed to the patch per se.... But.... you should know that relying on a libgphoto2-using process to not abnormally terminate is really dicey - libgphoto2 should really find a better way of doing this. For example, mean, what happens if gvfsd-gphoto2 segfaults? Then you've kinda lost. One approach is to fork a baby-sitter process or use atexit() or similar - AFAIR, there's really no good API in POSIX or Linux for this. Maybe the best option would probably to make your v4l driver automatically attach/detach when no-one is using the /dev/bus/usb node/<bus>/<device>. But I doubt such a patch would be accepted. The whole situation is really icky. (FWIW, with D-Bus, the traditional pattern is to listen to NameOwnerChanged() and then cleanup when the process that did something is terminated. For example, if process A calls InhibitScreensaver() on the screen saver process, then when process A exits the screen saver will stop being inhibited.)
(In reply to comment #1) > (In reply to comment #0) > > In case of the gphoto2 backend, this causes the kernel driver for dual > > mode webcams (which have a kernel space webcam driver and a userspace > > stillcam driver), to not get re-attached to the device when then the gvfs > > mount gets unmounted. > > Not opposed to the patch per se.... Erm, that is a really poor choice of words, the attached patch is 100% correct, even if this would not fix any user visible bug at all it should still get applied as the current code has got the ref-counting wrong, it really is as simple as that. This not fixing the user visible issue which triggered me to check the ref counting in 100% of the cases, does not change the fact that this is a correct patch, and as such should be applied. > But.... you should know that relying on a > libgphoto2-using process to not abnormally terminate is really dicey - > libgphoto2 should really find a better way of doing this. For example, mean, > what happens if gvfsd-gphoto2 segfaults? Then you've kinda lost. > > One approach is to fork a baby-sitter process or use atexit() or similar - > AFAIR, there's really no good API in POSIX or Linux for this. > > Maybe the best option would probably to make your v4l driver automatically > attach/detach when no-one is using the /dev/bus/usb node/<bus>/<device>. But I > doubt such a patch would be accepted. > > The whole situation is really icky. > So far I have not seen any real world issues with the device not getting re-attached due to programs crashing, and IMHO it is the applications responsibility to ensure to properly close the gphoto2 port (by deref-ing the camera), not libgphoto2, doing things like installing signal handlers from a library is generally not a good idea. And forking baby sitting processes does not make me go happy happy joy joy either (and also has signal handling issues).
(In reply to comment #2) > (In reply to comment #1) > > (In reply to comment #0) > > > In case of the gphoto2 backend, this causes the kernel driver for dual > > > mode webcams (which have a kernel space webcam driver and a userspace > > > stillcam driver), to not get re-attached to the device when then the gvfs > > > mount gets unmounted. > > > > Not opposed to the patch per se.... > > Erm, that is a really poor choice of words, the attached patch is 100% correct, > even if this would not fix any user visible bug at all it should still get > applied as the current code has got the ref-counting wrong, it really is as > simple as that. > > This not fixing the user visible issue which triggered me to check > the ref counting in 100% of the cases, does not change the fact that this > is a correct patch, and as such should be applied. Uhm, sorry for the choice of words - I was trying to convey that a) sure, there's a real bug here; and b) I hadn't had time to fully review the patch. Didn't mean to say the patch wasn't correct - I don't know that until I actually review it. I'll get to this (and your other patches) when I find some free cycles. > > But.... you should know that relying on a > > libgphoto2-using process to not abnormally terminate is really dicey - > > libgphoto2 should really find a better way of doing this. For example, mean, > > what happens if gvfsd-gphoto2 segfaults? Then you've kinda lost. > > > > One approach is to fork a baby-sitter process or use atexit() or similar - > > AFAIR, there's really no good API in POSIX or Linux for this. > > > > Maybe the best option would probably to make your v4l driver automatically > > attach/detach when no-one is using the /dev/bus/usb node/<bus>/<device>. But I > > doubt such a patch would be accepted. > > > > The whole situation is really icky. > > > > So far I have not seen any real world issues with the device not getting > re-attached due to programs crashing, and IMHO it is the applications > responsibility to ensure to properly close the gphoto2 port (by deref-ing the > camera), not libgphoto2, doing things like installing signal handlers from > a library is generally not a good idea. And forking baby sitting processes does > not make me go happy happy joy joy either (and also has signal handling > issues). Sure, as I tried to say there's really no good way of dealing with this because of the very way that POSIX and Linux works. I'm sure that once we resolve bug 606058, things will work great 99% of the time which probably is "good enough"; given it's all interactively driven by the user - e.g. the user can try to restart cheese etc.. But there will still be 1% of the time where things like 1. the user starts e.g. Cheese *just after* doing IO so there is no webcam visible 2. the user tries to do IO while using Cheese and this will fail which are like heisenbugs. And it will lead to hard-to-decipher bug reports and.. in the worst case.. lead to the maintainers inserting sleep() statements here and there. Not to mention the added complexity in resolving bug 606058 - it might very well introduce bugs (including probably exposing libgphoto2 bugs) that will require additional time to deal with. We just have to realize this is the cost if we want good support for this kind of hardware. It's *probably* a cost that worth dealing with. I don't know. As I said in bug 606058 and RH 551300 the problem is that a) these devices are totally non-standard insofar that they cram two functions into a single USB interface; and b) the fact that still-picture capture and moving-picture capture are handled so differently (one in user-mode, one in kernel-mode) makes things very difficult From a 50,000 point of view maybe it would have been better if the still-picture and moving-picture drivers were both in the kernel and shared some infrastructure so both drivers could be attached at the same time (e.g. a single driver would present both a filesystem interface (for gphoto2-ish stuff) and a V4L interface. It's just way too messy dealing with user-space drivers with the current way Linux works. Anyway, dreaming here in bugzilla won't get us there ;-) Anyway, enough rambling.
More details about the patch here https://bugzilla.redhat.com/show_bug.cgi?id=552842#c2
Alex, does this look OK to you? From a cursory look (and reading the explanation linked to in comment 4) this looks fine to me. But it touches code shared between all backends so I thought it would be useful if you could take a look. Thanks!
commited to master and gnome-2-28