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 712188 - [review] dcbw/default-wired-unsaved: convert NMDefaultWiredConnection into a normal NMSettingsConnection
[review] dcbw/default-wired-unsaved: convert NMDefaultWiredConnection into a ...
Status: RESOLVED FIXED
Product: NetworkManager
Classification: Platform
Component: general
unspecified
Other Linux
: Normal normal
: ---
Assigned To: NetworkManager maintainer(s)
NetworkManager maintainer(s)
Depends on:
Blocks:
 
 
Reported: 2013-11-13 00:21 UTC by Dan Williams
Modified: 2013-11-13 17:53 UTC
See Also:
GNOME target: ---
GNOME version: ---


Attachments
[PATCH] disconnect the signals for DefaultWiredConnection (6.42 KB, patch)
2013-11-13 15:31 UTC, Thomas Haller
none Details | Review

Description Dan Williams 2013-11-13 00:21:13 UTC
Because the NMDefaultWiredConnection was an in-memory only connection and never backed by a settings plugin connection, when the default wired connection was modified and saved, it required re-adding the connection to a settings plugin and removing the default wired connection object.  That triggered a device disconnect if a device was using that connection, since connection removal requires a device disconnect.

Now that we have unsaved connections, just use that functionality and make the default wired connection a normal NMSettingsConnection.  When it is deleted without having been modified, put the MAC address into no-auto-default like before.
Comment 1 Jiri Klimes 2013-11-13 14:19:46 UTC
* stray space after
signals[DBUS_UPDATED] =

* nm_setttings_device_added()
nm_log_info (LOGD_SETTINGS, "Added default wired connection '%s' for %s",
+	             nm_connection_get_id (NM_CONNECTION (added)),
+	             nm_device_get_udi (device));
 }

Do you want to print UDI instead of interface name?

Other than that it looks good. I will test shortly.
Comment 2 Dan Winship 2013-11-13 14:44:54 UTC
just nitpicks:

>+	nm_log_info (LOGD_SETTINGS, "Saved default wired connection '%s' to persistent storage",
>+	             nm_connection_get_id (NM_CONNECTION (connection)));

>+	nm_log_warn (LOGD_SETTINGS, "(%s) Couldn't save new default wired connection: %s",
>+	             nm_device_get_iface (device),

In both of these cases, "save" is not the right word is it? (In default_wired_dbus_updated(), the connection has been modified, but may still be unsaved. In nm_settings_device_added(), the connection is not being "saved" but merely "added".)
Comment 3 Thomas Haller 2013-11-13 15:31:13 UTC
Created attachment 259738 [details] [review]
[PATCH] disconnect the signals for DefaultWiredConnection
Comment 4 Thomas Haller 2013-11-13 15:32:30 UTC
(In reply to comment #3)
> Created an attachment (id=259738) [details] [review]
> [PATCH] disconnect the signals for DefaultWiredConnection

Notable change: this will also call nm_config_set_ethernet_no_auto_default for the DBUS_CHANGED signal... I think this is more correct.
Comment 5 Dan Williams 2013-11-13 17:35:38 UTC
I took thaller's patch except for always setting no-auto-default; I retained the previous behavior of only setting no-auto-default when the connection is explicitly deleted.
Comment 6 Dan Williams 2013-11-13 17:53:26 UTC
Merged to master.