GNOME Bugzilla – Bug 754409
[review] Changing the Metered connection property doesn't update it straight away [bg/update-metered-on-the-fly-bgo724041]
Last modified: 2015-09-22 14:55:21 UTC
1. Find the name of the connection you're using: $ nmcli connection 2. Set the connection as a metered one: nmcli connection modify "connection-name-from-1" connection.metered yes 3. Launch "gio/tests/network-monitor --watch" in glib's tree: Monitoring via GNetworkMonitorNM Network is up Connectivity is 4 Metered is 0 The metered property will only be changed after disconnecting and reconnecting the network, which is a problem when: - users expect metadata to be applied straight away, without the need to disconnect from other network services they might be using - applications expect the property to change on the fly (for example, PackageKit/gnome-software expect the property to potentially change when they've already started downloads) - the value of the property is dependent on factors outside the user's knowledge or reach (whether the connection will depend on the OS running on their phone for example)
Created attachment 310521 [details] [review] [PATCH] device: update metered property when connection settings change In general, changes to a connection are not propagated immediately to the device and a re-activation is required (see also bug 724041). But I agree that we should make an exception for the metered property as it is useful for applications to see the up-to-date value.
(In reply to Beniamino Galvani from comment #1) > Created attachment 310521 [details] [review] [review] > [PATCH] device: update metered property when connection settings change > > In general, changes to a connection are not propagated immediately to > the device and a re-activation is required (see also bug 724041). > > But I agree that we should make an exception for the metered property > as it is useful for applications to see the up-to-date value. LGTM. Thinking about this... Note that we currently have the documented behavior, that changes to an activated connection don't take effect until the connection re-activates. Note that there is a long open bug, that we violate this (which has bad consequences and might even lead to a crash). lr/applied-connection-bgo724041 will fix that. Note that we also have the future desire, to support something like live-reconfig. Which means, to modify a currently active connection and apply the changes immediately. e.g. adding a static IP address without need to reactivate the connection. Note that for firewall-zone, we do already propagate the update immediately: see connection_updated() in nm-policy.c. Note that there is a NM_SETTINGS_SIGNAL_CONNECTION_UPDATED and NM_SETTINGS_SIGNAL_CONNECTION_UPDATED_BY_USER. Maybe metered should only propagate in the latter case? Maybe firewall-changes too? I think the full solution eventually should be: - if the user updates a connection (on D-Bus level), he can specify a "mode" argument, with 3 values: - APPLY_NONE: no changes to the connection propagate to the currently active device - APPLY_DEFAULT: only certain changes propagate immediately. i.e. firewall-zone and metered. - APPLY_ALL: this first updates the connection, and then tries to apply all changes. Naturally, this second step might fail if the connection changes in incompatible ways. So, yes, this patch probably works well for now. And it will also integrate into a later, full-featured solution with applied-connection split and live-reconfig.
(In reply to Thomas Haller from comment #2) > (In reply to Beniamino Galvani from comment #1) > > Created attachment 310521 [details] [review] [review] [review] > > [PATCH] device: update metered property when connection settings change > > > > In general, changes to a connection are not propagated immediately to > > the device and a re-activation is required (see also bug 724041). > > > > But I agree that we should make an exception for the metered property > > as it is useful for applications to see the up-to-date value. > > LGTM. > > > > Thinking about this... > > > Note that we currently have the documented behavior, that changes to an > activated connection don't take effect until the connection re-activates. > Note that there is a long open bug, that we violate this (which has bad > consequences and might even lead to a crash). Reference? > lr/applied-connection-bgo724041 will fix that. I don't think there's much reasons to start scaffolding an architecture when what we're discussing is metadata. It's not configuration, and it doesn't change the way that NM itself behaves. The only thing it will change is the behaviour of outside applications. IMO, documenting the fact that this property can change on-the-fly should be enough.
(In reply to Bastien Nocera from comment #3) > (In reply to Thomas Haller from comment #2) > > (In reply to Beniamino Galvani from comment #1) > > > Created attachment 310521 [details] [review] [review] [review] [review] > > > [PATCH] device: update metered property when connection settings change > > > > > > In general, changes to a connection are not propagated immediately to > > > the device and a re-activation is required (see also bug 724041). > > > > > > But I agree that we should make an exception for the metered property > > > as it is useful for applications to see the up-to-date value. > > > > LGTM. > > > > > > > > Thinking about this... > > > > > > Note that we currently have the documented behavior, that changes to an > > activated connection don't take effect until the connection re-activates. > > Note that there is a long open bug, that we violate this (which has bad > > consequences and might even lead to a crash). > > Reference? Cannot find it, so it's undocumented behavior... But that's how it works up to now for every property (except firewall-zone). This is about defining how it should be, and how it will be after bug 724041, and how ~immediate propagation of metered property~ integrates in that bigger picture.
(In reply to Thomas Haller from comment #2) > Note that for firewall-zone, we do already propagate the update immediately: > see connection_updated() in nm-policy.c. > Note that there is a NM_SETTINGS_SIGNAL_CONNECTION_UPDATED and > NM_SETTINGS_SIGNAL_CONNECTION_UPDATED_BY_USER. Maybe metered should only > propagate in the latter case? Maybe firewall-changes too? Ah, I missed that. Probably it's a good idea to handle all changes to connections in a uniform way. I pushed branch bg/update-metered-on-the-fly-bgo724041 which uses the existing callback in nm-policy to update the device property. Regarding UPDATED vs UPDATED_BY_USER, I guess it's ok to use the latter for both the firewall zone and the metered property, since the signal is emitted when Update() is called on a NMSettingsConnection, i.e. a client modifies the connection. UPDATED instead is more generic and gets called every time a setting of a connection changes. > I think the full solution eventually should be: > > - if the user updates a connection (on D-Bus level), he can specify a > "mode" > argument, with 3 values: > - APPLY_NONE: no changes to the connection propagate to the currently > active device > - APPLY_DEFAULT: only certain changes propagate immediately. i.e. > firewall-zone and metered. > - APPLY_ALL: this first updates the connection, and then tries to > apply > all changes. Naturally, this second step might fail if the > connection > changes in incompatible ways. Looks reasonable. (In reply to Thomas Haller from comment #4) > This is about defining how it should be, and how it will be after bug > 724041, and how ~immediate propagation of metered property~ integrates in > that bigger picture. I haven't looked in detail at the branch posted for bug 724041, but shouldn't this be only a matter of propagation selected properties to the applied connection (i.e. implementing the APPLY_DEFAULT case in comment 2)?
(In reply to Beniamino Galvani from comment #5) > (In reply to Thomas Haller from comment #2) > > Note that for firewall-zone, we do already propagate the update immediately: > > see connection_updated() in nm-policy.c. > > Note that there is a NM_SETTINGS_SIGNAL_CONNECTION_UPDATED and > > NM_SETTINGS_SIGNAL_CONNECTION_UPDATED_BY_USER. Maybe metered should only > > propagate in the latter case? Maybe firewall-changes too? > > Ah, I missed that. Probably it's a good idea to handle all changes to > connections in a uniform way. I pushed branch > bg/update-metered-on-the-fly-bgo724041 which uses the existing > callback in nm-policy to update the device property. > > Regarding UPDATED vs UPDATED_BY_USER, I guess it's ok to use the > latter for both the firewall zone and the metered property, since the > signal is emitted when Update() is called on a NMSettingsConnection, > i.e. a client modifies the connection. UPDATED instead is more generic > and gets called every time a setting of a connection changes. bg/update-metered-on-the-fly-bgo724041 LGTM
Looks good.
bg/update-metered-on-the-fly-bgo724041 got merged upstream master: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=69315953eb366706a6ebf1e8af14d486f7a94149 Note that a later commit refactored this code already: http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=06da3532428e3498c1e808ff8be1af48b540a6ff -- but the original patch might proof useful to backport for nm-1-0. Closing BZ as resolved