GNOME Bugzilla – Bug 678597
PATCH: add support for inhibiting automount
Last modified: 2012-07-06 06:50:06 UTC
Created attachment 216997 [details] [review] PATCH: add support for inhibiting automount When connecting to virtual machines with usb-device redirection, such as Spice enabled vms, automount may get in the way. Specifically if auto-usbredir is enabled in the vm-viewer, then the usbredir code and the automount code race for who gets to the device first. If the automount code wins the race this is a problem, since usbredir causes a device-disconnect (iow the usb mass storage driver sees an unplug), so in the end usbredir always wins, and we end up with a non clean potentially corrupt filesystem. Also see: https://bugzilla.redhat.com/show_bug.cgi?id=812972 There for the need exists to be able to inhibit gnome-shell's automounting, since all other inhibits run through gnome-session, I've chosen to do the same for the automount-inhibiting. I've also submitted a patch to gnome-session to reserve flag value 16 for this, see bug 678595. This patch adds support to gnome-shell to honor this new inhibit flag.
Review of attachment 216997 [details] [review]: Not a review, just a small comment (shouldn't be hard to fix) ::: gnome-shell/js/ui/automountManager.js.orig @@ +223,3 @@ + if (this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT) == 'true') + return; I would do this asynchronous as it's generally not a good idea to do sync I/O in the compositor process ... for example, if gnome-session crashes (or worse, locks up), then the compositor process hangs for the duration of the time-out (25 seconds)...
(In reply to comment #1) > Review of attachment 216997 [details] [review]: > > Not a review, just a small comment (shouldn't be hard to fix) > > ::: gnome-shell/js/ui/automountManager.js.orig > @@ +223,3 @@ > > + if (this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT) == > 'true') > + return; > > I would do this asynchronous as it's generally not a good idea to do sync I/O > in the compositor process ... for example, if gnome-session crashes (or worse, > locks up), then the compositor process hangs for the duration of the time-out > (25 seconds)... Ok I will change this to this._session.IsInhibitedRemote. Now I understand why that is used in other places :)
Review of attachment 216997 [details] [review]: ::: gnome-shell/js/ui/automountManager.js.orig @@ +222,3 @@ } + if (this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT) == 'true') Use true not 'true' and as David said sync is evil ;) Also please use "git format-patch" and add a proper subject / commit message.
Created attachment 217271 [details] [review] PATCH: add support for inhibiting automount (In reply to comment #3) > Review of attachment 216997 [details] [review]: > > ::: gnome-shell/js/ui/automountManager.js.orig > @@ +222,3 @@ > } > > + if (this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT) == > 'true') > > Use true not 'true' I'm afraid that that does not work. > and as David said sync is evil ;) The sync usage is fixed in this revision, but even with using IsInhibitedRemote, modelled after the CanShutdown usage js/ui/userMenu.js I still need to use result == 'true' using just result makes _inhibit always true, using == true rather then == 'true' makes _inhibit always false. I'm not sure what kind of object the dbus proxy is passing us for functions returning a boolean, but it is not a plain js boolean. Other then that this goes beyond my js skills. > Also please use "git format-patch" and add a proper subject / commit message. Done, note patch is against the 3.4 branch. p.s. About the == 'true', I've the feeling the CanShutdown usage is not working correctly either, I will investigate this further.
(In reply to comment #4) > About the == 'true', I've the feeling the CanShutdown usage is not working > correctly either, I will investigate this further. And indeed CanShutdown is not working properly, and I believe a full audit will likely turn up other places of the same issue, I've filed bug 678852 for this.
(In reply to comment #5) > (In reply to comment #4) > > About the == 'true', I've the feeling the CanShutdown usage is not working > > correctly either, I will investigate this further. > > And indeed CanShutdown is not working properly, and I believe a full audit will > likely turn up other places of the same issue, I've filed bug 678852 for this. Right, but your hack is still wrong :-) You should be able to use if (this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT)[0])
Or use destructuring assignment: let [isInhibited] = this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT)[0];
erm... that should be: let [isInhibited] = this._session.IsInhibitedSync(GNOME_SESSION_AUTOMOUNT_INHIBIT);
Created attachment 217814 [details] [review] PATCH: add support for inhibiting automount Here is a new version removing the == 'true' hack.
Review of attachment 217814 [details] [review]: Looks good.
(In reply to comment #10) > Review of attachment 217814 [details] [review]: > > Looks good. Thanks for the review, I don't have commit rights, can you commit this for me? Thanks, Hans
Comment on attachment 217814 [details] [review] PATCH: add support for inhibiting automount Pushed as 9745e97e14b
(In reply to comment #12) > (From update of attachment 217814 [details] [review]) > Pushed as 9745e97e14b Thanks, can you perhaps also push the related patch from bug 678595 ? (has been reviewed, acked).