Created attachment 327712 [details] [review]
sharing: Use systemd to track running services

Instead of launching network services by hand, and having no way to
track them if gnome-settings-daemon crashes, leaving the services
dangling open when they should have been switched off, use systemd
--user to track the services.

This means that:
- services will not be left on when switching to a network where they
  should be disabled
- services will be restarted if they crash

gsd_sharing_manager_handle_service seems to be ignoring the method argument in the patch, always calling StartUnit instead.

Created attachment 327798 [details] [review]
sharing: Use systemd to track running services

Instead of launching network services by hand, and having no way to
track them if gnome-settings-daemon crashes, leaving the services
dangling open when they should have been switched off, use systemd
--user to track the services.

This means that:
- services will not be left on when switching to a network where they
  should be disabled
- services will be restarted if they crash

(In reply to zbyszek from comment #2)
> gsd_sharing_manager_handle_service seems to be ignoring the method argument
> in the patch, always calling StartUnit instead.

Yeah. This is what happens with untested patches. Thanks.

Review of attachment 327798 [details] [review]:

So my first thought was, is there a way we can cut g-s-d out of the picture entirely ?  For instance, have vino et al start from systemd --user directly.  The thought being "systemd is good at starting services, so it should be the one doing it."  Unfortunately, I don't think that's a possibility, since the services need to run conditionally, and those conditions aren't something systemd knows how to check.  As I understand it, the way this works right now is, anytime a network is joined the user can go to the control-center panel and whitelist it for specific services.  Until they're whitelisted, new networks don't get access. This is implemented by putting the network names (well uuids) in an enabled-connections list in DConf.  If a user joins a network and its whitelisted for some services, then g-s-d starts those services.  If the user leaves a network it stops the services. systemd, of course, has no notion of wireless access point connections, doesn't assign them uuids, and has no way to monitor DConf for changes. It can make services start and stop when files get created, deleted, or modified (using .path units and ConditionPathExists= directives), but that doesn't really help us. So we really do need some bridge code here.  I do thing the individual services themselves should commit hari kari if they're on a network they shouldn't be (or only bind to network interfaces that are kosher, until they're no longer kosher).  That would clean up one problem: if a user is plugged into a wired network and connects to a non-whitelisted wireless network at the same time, the services currently keeps running.  Ideally the running service would disallow access from the wireless network.  If the services themselves, knew what they were allowed to do, too, then if someone systemctl --user enabled' them behind g-s-d's back then they wouldn't serve networks the user told them not to in the control-center.  We could then ship the services as enabled by default, and the bridge code in the sharing plugin would only be an optimization to make sure the services aren't running when they have nothing to do.

But on to the patch itself...

::: plugins/sharing/gsd-sharing-manager.c
@@ +127,3 @@
+               gpointer      user_data)
+{
+        handle_unit_cb (source_object, res, "start", user_data);

It's a little icky that we have start_unit_cb and stop_unit_cb middlemen just to handle passing the state needed for writing a g_warning to the console in the event of a failure. Especially, since I think the failures should get logged to the journal by systemd anyway…  It's a shame we can't extract the method name from the GAsyncResult object.  Maybe just pass the method name as user_data to g_dbus_connection_call ? then you can drop start_unit_cb and stop_unit_cb entirely.  If you don't drop them, I'd at least call them handle_start_unit_cb and handle_stop_unit_cb to show the clear relationship to handle_service

@@ +153,3 @@
+                                method,
+                                /* Don't replace existing, already started
+                                 * services, so "fail" if already queued */

this comment is in the wrong place. It's confusing because the word replace happens on the very next line.

It should say something like

/* We use StartUnit, not StartUnitReplace, since the latter would cancel any pending start we already have going from an earlier start_service call */

and be in gsd_manager_start_service

Created attachment 334422 [details] [review]
sharing: Use systemd to track running services

Instead of launching network services by hand, and having no way to
track them if gnome-settings-daemon crashes, leaving the services
dangling open when they should have been switched off, use systemd
--user to track the services.

This means that:
- services will not be left on when switching to a network where they
  should be disabled
- services will be restarted if they crash

Attachment 334422 [details] pushed as e0b7f41 - sharing: Use systemd to track running services