GNOME Bugzilla – Bug 768969
NM dispatcher supports 'connectivity-changed' events
Last modified: 2016-07-28 20:30:25 UTC
Sometimes the 'up' action that trigger the execution of the NMD's scripts [1] are not good enough events to implement some hooks in the OS, since we might need to know when we actually get fully connected to the internet, and 'up' would be executed even when that's not yet true (e.g. captive portal). At the moment, this information is only exposed via the CheckConnectivity method of the org.freedesktop.NetworkManager D-Bus interface [2] (also accessible via nmcli [3]) but it would be great if the NM dispatcher would also dispatch scripts when the connectivity state changes (e.g. a new 'connectivity-changed' action?), so that we could easily implement these other use cases that only make sense when either a limited or a full internet connection is available. [1] https://developer.gnome.org/NetworkManager/stable/NetworkManager.html [2] https://developer.gnome.org/NetworkManager/stable/gdbus-org.freedesktop.NetworkManager.html [3] https://developer.gnome.org/NetworkManager/unstable/nmcli.html
Created attachment 331797 [details] [review] nm-dispatcher: Added new action for 'connectivity-changed' events The purpose of this action is to provide a hook for the OS to act on global changes regarding to connectivity, which can currently be one of 'none', 'portal', 'limited', 'full' or 'unknown'. Check the documentation for a more information on each of those states: https://developer.gnome.org/NetworkManager/stable/nm-dbus-types.html#NMConnectivityState
Created attachment 331815 [details] [review] nm-dispatcher: Added new action for 'connectivity-changed' events Sorry, I've uploaded the wrong patch yesterday. This is the right one now.
Created attachment 331841 [details] [review] nm-dispatcher: Added new action for 'connectivity-change' events The purpose of this action is to provide a hook for the OS to act on global changes regarding to connectivity, which can currently be one of 'none', 'portal', 'limited', 'full' or 'unknown'. Check the documentation for a more information on each of those states: https://developer.gnome.org/NetworkManager/stable/nm-dbus-types.html#NMConnectivityState
Created attachment 331842 [details] [review] nm-dispatcher: Added new 'connectivity-state' parameter to private D-Bus API In order to pass the connectivity state to the relevant hooks along with the event itself, we need to add this parameter for the 'Action' method of then internal 'org.freedesktop.nm_dispatcher' interface, which will be sent by the network manager main process to the dispatcher.
Created attachment 331843 [details] [review] nm-dispatcher: Handle the 'connectivity-state' parameter for 'Action' Actually handle this new parameter in the nm-dispatcher process, setting the value of a new CONNECTIVITY_STATE environment variable accordingly. This patch also adapts the test infrastructure accordingly and updates tests and their expectations, so that 'make check' continues to work.
Created attachment 331844 [details] [review] man: Added documentation for the connectivity-change dispatcher hook Also added documentation for the new CONNECTIVITY_STATE environment variable, that will also be avaiblable in those hooks.
Review of attachment 331841 [details] [review]: ::: callouts/nm-dispatcher-utils.c @@ +355,3 @@ + /* Hostname and connectivity changes don't require a device nor contain a connection */ + if (!strcmp (action, NMD_ACTION_HOSTNAME) + || !strcmp (action, NMD_ACTION_CONNECTIVITY_CHANGE)) { Just a nitpick: usually we shift the first condition to the right so that it's aligned with the second one (just insert 3 spaces before !strcmp (..HOSTNAME)). ::: src/nm-dispatcher.c @@ +486,3 @@ + /* All actions except 'hostname' and 'connectivity-change' require a device */ + if (action == DISPATCHER_ACTION_HOSTNAME + || action == DISPATCHER_ACTION_CONNECTIVITY_CHANGE) { The same here.
Review of attachment 331843 [details] [review]: ::: callouts/nm-dispatcher-utils.c @@ +471,3 @@ + if (connectivity_state && strlen (connectivity_state)) + items = g_slist_prepend (items, g_strdup_printf ("CONNECTIVITY_STATE=%s", connectivity_state)); + For actions different from "connectivity-changed" CONNECTIVITY_STATE=none is always set in the environment, which in my opinion is misleading. Probably it would be better to omit the variable if not relevant (and document this); or in alternative, always set it but with the actual value.
Thanks for the really quick review. See my comments below... (In reply to Beniamino Galvani from comment #7) > Review of attachment 331841 [details] [review] [review]: > > ::: callouts/nm-dispatcher-utils.c > @@ +355,3 @@ > + /* Hostname and connectivity changes don't require a device nor contain a > connection */ > + if (!strcmp (action, NMD_ACTION_HOSTNAME) > + || !strcmp (action, NMD_ACTION_CONNECTIVITY_CHANGE)) { > > Just a nitpick: usually we shift the first condition to the right so > that it's aligned with the second one (just insert 3 spaces before > !strcmp (..HOSTNAME)). Ah! I saw that and thought it was a typo :). I'll change it, no worries > ::: src/nm-dispatcher.c > @@ +486,3 @@ > + /* All actions except 'hostname' and 'connectivity-change' require a > device */ > + if (action == DISPATCHER_ACTION_HOSTNAME > + || action == DISPATCHER_ACTION_CONNECTIVITY_CHANGE) { > > The same here. Ok (In reply to Beniamino Galvani from comment #8) > Review of attachment 331843 [details] [review] [review]: > > ::: callouts/nm-dispatcher-utils.c > @@ +471,3 @@ > + if (connectivity_state && strlen (connectivity_state)) > + items = g_slist_prepend (items, g_strdup_printf ("CONNECTIVITY_STATE=%s", > connectivity_state)); > + > > For actions different from "connectivity-changed" CONNECTIVITY_STATE=none is > always set in the environment, which in my opinion is misleading. Probably > it would be better to omit the variable if not relevant (and document this); > or in alternative, always set it but with the actual value. Oh! That's a very good point. In my initial versions of this patch I had it written in a way that the connectivity state was always passed to the dispatcher which I think is why I did not notice than in the current setup (where we send the value when is changed in nm-connectivity.c) that does not make much sense, really. I myself don't have any strong preference for one option or the other, but considering the other actions supported I kind of feel that this CONNECTIVITY_STATE envvar will only be of interest for the connectivity-change event, and so I think I kind of lean more towards only setting it when it's relevant.
Created attachment 331870 [details] [review] nm-dispatcher: Added new action for 'connectivity-change' events The purpose of this action is to provide a hook for the OS to act on global changes regarding to connectivity, which can currently be one of 'none', 'portal', 'limited', 'full' or 'unknown'. Check the documentation for a more information on each of those states: https://developer.gnome.org/NetworkManager/stable/nm-dbus-types.html#NMConnectivityState
Created attachment 331871 [details] [review] nm-dispatcher: Added new 'connectivity-state' parameter to private D-Bus API In order to pass the connectivity state to the relevant hooks along with the event itself, we need to add this parameter for the 'Action' method of then internal 'org.freedesktop.nm_dispatcher' interface, which will be sent by the network manager main process to the dispatcher.
Created attachment 331872 [details] [review] nm-dispatcher: Handle the 'connectivity-state' parameter for 'Action' Actually handle this new parameter in the nm-dispatcher process, setting the value of a new CONNECTIVITY_STATE environment variable accordingly. This patch also includes new tests to check the different cases.
Created attachment 331873 [details] [review] man: Added documentation for the connectivity-change dispatcher hook Also added documentation for the new CONNECTIVITY_STATE environment variable, that will also be avaiblable in those hooks.
Beniamino, I think I have addressed all the issues you pointed out now, and also added specific unit tests for connectivity-change action this time (instead of reusing the existent ones) to make sure that, among other things, the variable is only set when the state is not 'UNKNOWN'. It passes make check and it works (tested locally), let me know what you think.
(In reply to Mario Sánchez Prada from comment #14) > Beniamino, I think I have addressed all the issues you pointed out now, and > also added specific unit tests for connectivity-change action this time > (instead of reusing the existent ones) to make sure that, among other > things, the variable is only set when the state is not 'UNKNOWN'. > > It passes make check and it works (tested locally), let me know what you > think. The new callouts/tests/dispatcher-connectivity-* files must be added to callouts/tests/Makefile.am, otherwise 'make distcheck' will fail. And maybe just a couple of tests are enough ('unknown' and another one), since what changes is only the value. Otherwise looks good!
Created attachment 332030 [details] [review] nm-dispatcher: Handle the 'connectivity-state' parameter for 'Action' Actually handle this new parameter in the nm-dispatcher process, setting the value of a new CONNECTIVITY_STATE environment variable accordingly. This patch also includes new tests to check the different cases.
(In reply to Beniamino Galvani from comment #15) > [...] > The new callouts/tests/dispatcher-connectivity-* files must be added to > callouts/tests/Makefile.am, otherwise 'make distcheck' will fail. And maybe > just a couple of tests are enough ('unknown' and another one), since what > changes is only the value. Oh! right... totally forgot about make distcheck, my fault. And yes, I agree that unknown + a non-unknown test should be enough, will change that too. > Otherwise looks good! Thanks, I've just uploaded a new version of the patch that added the new tests, incorporating your suggestions. I can't commit directly to fdo repositories, so please feel free to land them if you feel like they're good, otherwise let me know and I'll do any changes that might be still needed.
Patches LGTM. Thanks!
Dan Williams: Thanks for the feedback. I'll work on integrating them downstream right now then, but I'd need someone else with commit access to fdo git repos to push this for me, as I only have access to git.gnome.org for now.
Patches applied, thanks. 283562e nm-dispatcher: Added new action for 'connectivity-change' events dfd9d85 nm-dispatcher: Added new 'connectivity-state' parameter to private D-Bus API 3359368 man: Added documentation for the connectivity-change dispatcher hook 570e8ee nm-dispatcher: Handle the 'connectivity-state' parameter for 'Action'