After an evaluation, GNOME has moved from Bugzilla to GitLab. Learn more about GitLab.
No new issues can be reported in GNOME Bugzilla anymore.
To report an issue in a GNOME project, go to GNOME GitLab.
Do not go to GNOME Gitlab for: Bluefish, Doxygen, GnuCash, GStreamer, java-gnome, LDTP, NetworkManager, Tomboy.
Bug 748050 - [review] make ignore-carrier configuration reloadable
[review] make ignore-carrier configuration reloadable
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
git master
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2015-04-17 12:57 UTC by Thomas Haller
Modified: 2015-05-05 14:59 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
config: make ignore-carrier option reloadable (5.14 KB, patch)
2015-04-17 12:58 UTC, Thomas Haller
accepted-commit_now Details | Review
device: allow reloading of the ignore-carrier flag (3.34 KB, patch)
2015-04-17 12:58 UTC, Thomas Haller
none Details | Review

Description Thomas Haller 2015-04-17 12:57:57 UTC
Seems a nice and easy feature. Especially since we have NM-config-server that brings this configuration. Maybe %post install of the configuration should send SIGHUP to NetworkManager.
Comment 1 Thomas Haller 2015-04-17 12:58:25 UTC
Created attachment 301830 [details] [review]
config: make ignore-carrier option reloadable

Only move the ignore-carrier option from NMConfig to
NMConfigData. The ignore-carrier option is still
immutable after startup.
Comment 2 Thomas Haller 2015-04-17 12:58:29 UTC
Created attachment 301831 [details] [review]
device: allow reloading of the ignore-carrier flag

Now on SIGHUP, when reloading NetworkManager configuration, also reload
the ignore-carrier flag.

While a device is activated, the reload is ignored until the device
deactivates.

Maybe it would be simpler just not to cache ignore_carrer and let it
take effect immediately. But not caching ignore_carrer has the
additional downside that every call to is_available must check the
specs -- which in sum is potentially expensive for something that
almost never changes.
Comment 3 Dan Williams 2015-05-05 14:40:29 UTC
Review of attachment 301830 [details] [review]:

LGTM
Comment 4 Dan Williams 2015-05-05 14:42:22 UTC
Review of attachment 301831 [details] [review]:

Just a comment fix, the rest looks good.

::: src/devices/nm-device.c
@@ +7991,3 @@
 
+		if (device_has_capability (self, NM_DEVICE_CAP_CARRIER_DETECT)) {
+			/* We cache the ignore_carrier state to don't react on config-reloads while the connection

"to not" or "so that we don't"
Comment 5 Thomas Haller 2015-05-05 14:59:26 UTC
Thanks,


both patches merged to master:

http://cgit.freedesktop.org/NetworkManager/NetworkManager/commit/?id=9847ab7147465de089b0093ad74bd8b07f5d1b27


(after resolving conflicts while rebasing)