GNOME Bugzilla – Bug 731726
sharing: Implement Sharing manager
Last modified: 2014-06-24 09:42:56 UTC
Not finished yet, but here for early comments. We need to find a fallback for when NetworkManager isn't available.
Created attachment 278542 [details] [review] sharing: Add stub for new sharing plugin This plugin will be used to activate select sharing network services depending on the network used.
Created attachment 278543 [details] [review] sharing: Implement Sharing manager properties This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - b NetworkIsAvailable, whether the machine is connected to a network - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi
Created attachment 278696 [details] [review] sharing: Implement Sharing manager This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - b NetworkIsAvailable, whether the machine is connected to a network - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi Services supported are one of: - rygel - vino-server - gnome-user-share-webdav It offers 3 methods, one to enable (EnableService) or disable services (DisableService) on the current network, and one to list the networks on which each service is enabled (ListNetworks).
It's not building: make[3]: Entering directory `/home/rmatos/Source/git.gnome.org/gnome-settings-daemon/plugins/sharing' CC libsharing_la-gsd-sharing-manager.lo make[3]: *** No rule to make target `test-sharing.c', needed by `gsd_test_sharing-test-sharing.o'. Stop.
Another initial comment: I'd prefer to have both patches merged instead of adding things are are immediately removed and/or moved around in the next patch.
Created attachment 278705 [details] [review] sharing: Implement Sharing manager This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - b NetworkIsAvailable, whether the machine is connected to a network - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi Services supported are one of: - rygel - vino-server - gnome-user-share-webdav It offers 3 methods, one to enable (EnableService) or disable services (DisableService) on the current network, and one to list the networks on which each service is enabled (ListNetworks).
Created attachment 278784 [details] [review] sharing: Implement Sharing manager This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" If empty, we're offline - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could be sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi Services supported are one of: - rygel - vino-server - gnome-user-share-webdav It offers 3 methods, one to enable (EnableService) or disable services (DisableService) on the current network, and one to list the networks on which each service is enabled (ListNetworks).
Created attachment 278785 [details] [review] sharing: Implement Sharing manager This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" If empty, we're offline - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could be sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi Services supported are one of: - rygel - vino-server - gnome-user-share-webdav It offers 3 methods, one to enable (EnableService) services on the current network, one to disable services (DisableService) on particular networks, and one to list the networks on which each service is enabled (ListNetworks).
Review of attachment 278785 [details] [review]: I think that in both handle_get_property() and handle_method_call() we should immediately fail if (manager->priv->client == NULL). i.e. I think that having NM working correctly at runtime is a fair requirement here. Related to that, what happens if NM disappears? Does NMClient tell us about it? Perhaps we should also disable all services in that case? ::: plugins/sharing/gsd-sharing-manager.c @@ +112,3 @@ + g_free (desktop); + + exec = g_desktop_app_info_get_string (app, "Exec"); g_app_info_get_commandline() ? @@ +122,3 @@ + } + + service->started = g_spawn_async_with_pipes (NULL, You're not making use of the pipes here, so the simpler g_spawn_async() would look tidier. But, is there any reason to avoid GSubprocess? You'd then have a nice interface to know the process status, send unix signals to it, etc. @@ +135,3 @@ + + if (!service->started) { + g_warning ("Could not parse command-line '%s': %s", exec, error->message); Copy/pasted warning output :-) @@ +181,3 @@ + kill ((pid_t) service->pid, SIGTERM); + service->pid = 0; + service->started = 0; Yeah, GSubprocess would make this kind of stuff cleaner @@ +321,3 @@ + g_strfreev (connections); + + gsd_sharing_manager_stop_service (manager, service); Shouldn't this only be done only if (g_str_equal (network_name, manager->priv->current_network)) ? @@ +358,3 @@ + + /* Disable sharing on open Wi-Fi + * XXX: Also do this for WEP networks? */ I think we should mark WEP as low security yes. @@ +541,3 @@ + + if (!a_con) { + manager->priv->sharing_status = "offline"; Perhaps making ->sharing_status an enum and then having a simple table to get the strings from would be cleaner? @@ +542,3 @@ + if (!a_con) { + manager->priv->sharing_status = "offline"; + } else if (g_strcmp0 (manager->priv->carrier_type, "bluetooth") == 0) { You use g_str_equal() below. I'd use it here as well @@ +621,3 @@ + if (manager->priv->cancellable != NULL) { + g_debug ("Already started"); + gnome_settings_profile_end (NULL); Other plugins don't do this. In any case ->introspection_data is leaked on this path @@ +691,3 @@ + + manager->priv = GSD_SHARING_MANAGER_GET_PRIVATE (manager); + manager->priv->services = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, service_free); g_free, but the keys inserted below are const char* @@ +705,3 @@ + service->name = services[i]; + path = g_strdup_printf ("/org/gnome/settings-daemon/plugins/sharing/%s/", services[i]); + service->settings = g_settings_new_with_path ("org.gnome.settings-daemon.plugins.sharing.service", path); We don't handle the enabled-connections gsetting being changed externally. Fine with me but just pointing it out in case you didn't do that on purpose.
(In reply to comment #9) > Review of attachment 278785 [details] [review]: > > I think that in both handle_get_property() and handle_method_call() we should > immediately fail if (manager->priv->client == NULL). i.e. I think that having > NM working correctly at runtime is a fair requirement here. Done. > Related to that, what happens if NM disappears? Does NMClient tell us about it? > Perhaps we should also disable all services in that case? I have no idea. But I would expect the primary-connection to be NULL, so that we'll bring the connections down. > ::: plugins/sharing/gsd-sharing-manager.c > @@ +112,3 @@ > + g_free (desktop); > + > + exec = g_desktop_app_info_get_string (app, "Exec"); > > g_app_info_get_commandline() ? Done. > @@ +122,3 @@ > + } > + > + service->started = g_spawn_async_with_pipes (NULL, > > You're not making use of the pipes here, so the simpler g_spawn_async() would > look tidier. > > But, is there any reason to avoid GSubprocess? You'd then have a nice interface > to know the process status, send unix signals to it, etc. Done. > @@ +135,3 @@ > + > + if (!service->started) { > + g_warning ("Could not parse command-line '%s': %s", exec, > error->message); > > Copy/pasted warning output :-) Fixed. > @@ +181,3 @@ > + kill ((pid_t) service->pid, SIGTERM); > + service->pid = 0; > + service->started = 0; > > Yeah, GSubprocess would make this kind of stuff cleaner Done. > @@ +321,3 @@ > + g_strfreev (connections); > + > + gsd_sharing_manager_stop_service (manager, service); > > Shouldn't this only be done only if (g_str_equal (network_name, > manager->priv->current_network)) ? Yes, indeed. My original code had a DisableService(s service_name), which didn't allow services to be disabled for connections other than the current one. Fixed. > @@ +358,3 @@ > + > + /* Disable sharing on open Wi-Fi > + * XXX: Also do this for WEP networks? */ > > I think we should mark WEP as low security yes. That's a TODO item for later. The code necessary to do this is non-trivial, and I'd like to test it. > @@ +541,3 @@ > + > + if (!a_con) { > + manager->priv->sharing_status = "offline"; > > Perhaps making ->sharing_status an enum and then having a simple table to get > the strings from would be cleaner? Done. > @@ +542,3 @@ > + if (!a_con) { > + manager->priv->sharing_status = "offline"; > + } else if (g_strcmp0 (manager->priv->carrier_type, "bluetooth") == 0) > { > > You use g_str_equal() below. I'd use it here as well Done. > @@ +621,3 @@ > + if (manager->priv->cancellable != NULL) { > + g_debug ("Already started"); > + gnome_settings_profile_end (NULL); > > Other plugins don't do this. In any case ->introspection_data is leaked on this > path I've removed it. > @@ +691,3 @@ > + > + manager->priv = GSD_SHARING_MANAGER_GET_PRIVATE (manager); > + manager->priv->services = g_hash_table_new_full (g_str_hash, > g_str_equal, g_free, service_free); > > g_free, but the keys inserted below are const char* Fixed. > @@ +705,3 @@ > + service->name = services[i]; > + path = g_strdup_printf > ("/org/gnome/settings-daemon/plugins/sharing/%s/", services[i]); > + service->settings = g_settings_new_with_path > ("org.gnome.settings-daemon.plugins.sharing.service", path); > > We don't handle the enabled-connections gsetting being changed externally. Fine > with me but just pointing it out in case you didn't do that on purpose. That's completely expected. I want the configuration storage to be private to the implementation. In the future, we might want to split it off from gnome-settings-daemon, for example using a session-wide NM dispatcher instead.
Created attachment 278986 [details] [review] sharing: Implement Sharing manager This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" If empty, we're offline - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could be sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi Services supported are one of: - rygel - vino-server - gnome-user-share-webdav It offers 3 methods, one to enable (EnableService) services on the current network, one to disable services (DisableService) on particular networks, and one to list the networks on which each service is enabled (ListNetworks).
Created attachment 278987 [details] [review] sharing: Disable rygel, for <= 3.12 upgrades Disable rygel which might have got autostarted by setups made prior to 3.14.
Review of attachment 278986 [details] [review]: Only the Makefile issue is critical, the others aren't so important ::: plugins/sharing/Makefile.am @@ +24,3 @@ +libsharing_la_LDFLAGS = $(GSD_PLUGIN_LDFLAGS) + +libsharing_la_LIBADD = $(SETTINGS_PLUGIN_LIBS) missing $(SHARING_LIBS) ::: plugins/sharing/gsd-sharing-manager.c @@ +130,3 @@ + service->started = FALSE; + } else { + service->started = TRUE; We could make this more sophisticated and do a g_subprocess_wait_async() here to be able to restart the service in case it crashes. But that can go in a followup patch or we can just leave it as is for now and wait for systemd in user sessions to take care of it *sigh* @@ +171,3 @@ + g_debug ("About to stop %s", service->name); + + g_subprocess_force_exit (service->process); This is SIGKILL though. g_subprocess_send_signal (process, SIGTERM) is what we want here
Review of attachment 278987 [details] [review]: I don't have a good solution but I don't feel confortable with this ::: plugins/sharing/gsd-sharing-manager.c @@ +622,3 @@ + + path = g_build_filename (g_get_user_config_dir (), "autostart", + "rygel.desktop", NULL); How was this file created in the first place? If someone created it manually I don't think we should silently just delete it. At the very least we should have a g_warning about this. IIUC, rygel has been modified for this to not be needed anymore, right? I think rygel itself should clean this stuff in that case. @@ +626,3 @@ + g_free (path); + + connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); _sync :-(
(In reply to comment #14) > Review of attachment 278987 [details] [review]: > > I don't have a good solution but I don't feel confortable with this > > ::: plugins/sharing/gsd-sharing-manager.c > @@ +622,3 @@ > + > + path = g_build_filename (g_get_user_config_dir (), "autostart", > + "rygel.desktop", NULL); > > How was this file created in the first place? If someone created it manually I > don't think we should silently just delete it. At the very least we should have > a g_warning about this. > > IIUC, rygel has been modified for this to not be needed anymore, right? I think > rygel itself should clean this stuff in that case. It's not rygel creating it, it's gnome-control-center creating a symlink in the user's autostart directory. > @@ +626,3 @@ > + g_free (path); > + > + connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); > > _sync :-( Doing this async would be a major PITA. It should also be really quick, just a ref, as gnome-control-center is a GtkApplication so will already have instantiated a GDBusConnection.
(In reply to comment #15) > (In reply to comment #14) <snip> > > @@ +626,3 @@ > > + g_free (path); > > + > > + connection = g_bus_get_sync (G_BUS_TYPE_SESSION, NULL, NULL); > > > > _sync :-( > > Doing this async would be a major PITA. It should also be really quick, just a > ref, as gnome-control-center is a GtkApplication so will already have > instantiated a GDBusConnection. Huh, gnome-settings-daemon already connects to the session bus, not gnome-control-center.
(In reply to comment #13) > Review of attachment 278986 [details] [review]: > > Only the Makefile issue is critical, the others aren't so important > > ::: plugins/sharing/Makefile.am > @@ +24,3 @@ > +libsharing_la_LDFLAGS = $(GSD_PLUGIN_LDFLAGS) > + > +libsharing_la_LIBADD = $(SETTINGS_PLUGIN_LIBS) > > missing $(SHARING_LIBS) Fixed. > ::: plugins/sharing/gsd-sharing-manager.c > @@ +130,3 @@ > + service->started = FALSE; > + } else { > + service->started = TRUE; > > We could make this more sophisticated and do a g_subprocess_wait_async() here > to be able to restart the service in case it crashes. But that can go in a > followup patch or we can just leave it as is for now and wait for systemd in > user sessions to take care of it *sigh* Yeah, this is just something that works for now. I'm hoping that systemd user sessions will be there soon enough to remove that code. > @@ +171,3 @@ > + g_debug ("About to stop %s", service->name); > + > + g_subprocess_force_exit (service->process); > > This is SIGKILL though. g_subprocess_send_signal (process, SIGTERM) is what we > want here Sure.
Created attachment 279023 [details] [review] sharing: Implement Sharing manager This D-Bus service tracks the current primary mean of connecting to the Internet, or the network (NetworkManager's "primary-connection" property). Properties exported are: - s CurrentNetwork, the NetworkManager unique identifier for the connection used. This name will be in the output of "nmcli c" If empty, we're offline - s CarrierType, the connection (and therefore media) used to access the network. It corresponds to the TYPE column in the output of "nmcli c" - s SharingStatus, the current status of sharing. It is one of: - offline, there's no network so no sharing possible - available, there is a network connection, so we could be sharing something - disabled-mobile-broadband, sharing is not possible because the main network access is through a mobile device - disabled-low-security, the network used is too insecure to offer sharing, such as unencrypted Wi-Fi Services supported are one of: - rygel - vino-server - gnome-user-share-webdav It offers 3 methods, one to enable (EnableService) services on the current network, one to disable services (DisableService) on particular networks, and one to list the networks on which each service is enabled (ListNetworks).
(In reply to comment #15) > It's not rygel creating it, it's gnome-control-center creating a symlink in the > user's autostart directory. I see. Perhaps just delete it if it's a symlink? I'd still prefer if rygel took care of it. > Doing this async would be a major PITA. It should also be really quick, just a > ref, as gnome-control-center is a GtkApplication so will already have > instantiated a GDBusConnection. True. Ok
(In reply to comment #19) > (In reply to comment #15) > > It's not rygel creating it, it's gnome-control-center creating a symlink in the > > user's autostart directory. > > I see. Perhaps just delete it if it's a symlink? That's racy, but I guess this would be better than what we have currently. > I'd still prefer if rygel took > care of it. Rygel isn't involved at all in this. This is a GNOME specific problem, created by the code in gnome-control-center's Sharing panel.
Created attachment 279032 [details] [review] sharing: Disable rygel, for <= 3.12 upgrades Disable rygel which might have got autostarted by setups made prior to 3.14.