GNOME Bugzilla – Bug 765910
Port to libnm 1.2
Last modified: 2016-05-29 18:40:53 UTC
Note that the network panel port isn't finished, or reviewed, or maybe even functional. It's just the first one that compiles and links. Don't waste your time reading it.
Created attachment 327163 [details] [review] network: Remove bridge support It is supported by Cockpit already: https://github.com/cockpit-project/cockpit/issues/459
Created attachment 327164 [details] [review] network: Remove Bond support It is supported by Cockpit already: https://github.com/cockpit-project/cockpit/issues/458
Created attachment 327165 [details] [review] network: Remove Team support It will be supported by Cockpit in the near future: https://github.com/cockpit-project/cockpit/issues/4330
Created attachment 327166 [details] [review] network: Remove VLAN support It is supported by Cockpit already: https://github.com/cockpit-project/cockpit/issues/1007
Created attachment 327167 [details] [review] network: Remove HAVE_NM_UNSTABLE check We don't have any code that relies on it anymore.
Created attachment 327168 [details] [review] network: Remove now unused virtual devices support
Created attachment 327169 [details] [review] network: Don't use deprecated NM_SETTING_WIRELESS_SEC property aka 802-11-wireless.security. The security is identified by presence of a 802-11-wireless-security (NMSettingWirelessSecurity) setting.
Created attachment 327170 [details] [review] power: Port to libnm 1.2
Created attachment 327171 [details] [review] XXX network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature). With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
Created attachment 327219 [details] [review] network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature). With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
Comment on attachment 327169 [details] [review] network: Don't use deprecated NM_SETTING_WIRELESS_SEC property FTR I don't remember if this change is actually legitimate before the libnm port...
Comment on attachment 327170 [details] [review] power: Port to libnm 1.2 >- priv->nm_client = nm_client_new (); >+ priv->nm_client = nm_client_new (NULL, NULL); > g_signal_connect (priv->nm_client, "notify", nm_client_new() can, at least theoretically, return NULL and a GError (though mostly only in the case of D-Bus errors, which probably can't happen if you have a desktop that is functioning well enough to get the control center running...)
Comment on attachment 327219 [details] [review] network: Port to libnm 1.2 >+++ b/panels/network/cc-network-panel.c >- iface = nm_connection_get_virtual_iface_name (connection); >+ iface = nm_connection_get_virtual_device_description (connection); >+ //FIXME IIRC nm_connection_get_interface_name() is what you want >+++ b/panels/network/connection-editor/ce-page-ip4.c >- add_address_row (page, address, network, gateway); >+ add_address_row (page, >+ nm_ip_address_get_address (addr), >+ network, >+ nm_setting_ip_config_get_gateway (page->setting)); OK, so, libnm-util/libnm-glib basically had a buggy data model, which nm-connection-editor (and then gnome-control-center) copied, in which each manually-configured IP address has an associated gateway address. In reality, NM always just took the first non-empty gateway value from the address array, and completely ignored any other gateway values. libnm represents this more accurately, by having a single gateway value which is separate from the address array. Ideally, the editors should show it this way as well (eg, like nmtui does). Failing that, it would be nice to at least make it so that only the first row in the address table can have a non-empty gateway value. >+ gchar network[INET_ADDRSTRLEN + 1]; >+ snprintf (network, sizeof (network), "%u", nm_ip_route_get_prefix (route)); it's confusing to call that "network", and confusing to have it be INET_ADDRSTRLEN-sized when it just contains a number from 0-32. >+ dns_servers = g_ptr_array_new (); ... >+ g_ptr_array_add (dns_servers, g_strdup (text)); ... >+ g_ptr_array_free (dns_servers, TRUE); leaking the individual strings >+ NM_SETTING_IP_CONFIG_ADDRESSES, addresses, >+ NM_SETTING_IP_CONFIG_ROUTES, routes, These have a different type in libnm than libnm-util but you didn't update the code. (They are GPtrArrays of NMIPAddress / NMIPRoute, not GPtrArrays of GArrays of guint32s.) >+++ b/panels/network/connection-editor/ce-page-ip6.c The comments about the IP4 page also apply to the IP6 page. In general, the two pages should have nearly identical code when using libnm, other than a lot of s/4/6/. (And the fact that they have a different set of available methods, and there are a few extra options in NMSettingIP6Config.) >+++ b/panels/network/connection-editor/ce-page.c > g_signal_emit (page, signals[INITIALIZED], 0, error); >+ //FIXME free error? probably, yes >+ if (error) { >+ dbus_err = g_dbus_error_get_remote_error (error); >+ ignore_error = !g_strcmp0 (dbus_err, "org.freedesktop.NetworkManager.Settings.InvalidSetting") >+ || !g_strcmp0 (dbus_err, "org.freedesktop.NetworkManager.Settings.Connection.SettingNotFound") >+ || !g_strcmp0 (dbus_err, "org.freedesktop.NetworkManager.AgentManager.NoSecrets"); D-Bus errors are mapped correctly in libnm so you can just use g_error_matches(). Though the set of errors has also changed slightly. In this case you want to be checking for NM_CONNECTION_ERROR_SETTING_NOT_FOUND (which is returned if you requested the secrets for an NMSetting that wasn't present in the connection) or NM_SECRET_AGENT_ERROR_NO_SECRETS. >+++ b/panels/network/net-device-ethernet.c >- path1 = nm_active_connection_get_connection (aconn); >+ path1 = nm_active_connection_get_specific_object_path (aconn); No, that's something different... there's no API to return the path of the NMConnection associated with an NMActiveConnection directly... > path2 = nm_connection_get_path (connection); > active = g_strcmp0 (path1, path2) == 0; but you can change the code to compare UUIDs rather than paths >+++ b/panels/network/net-device-mobile.c > /* is this already activated? */ > if (active_connection != NULL && > g_strcmp0 (nm_connection_get_path (connection), >- nm_active_connection_get_connection (active_connection)) == 0) { >+ nm_active_connection_get_specific_object_path (active_connection)) == 0) { likewise here. (and again later in device_off_toggled(), and again in net-device-simple.c, and net-vpn.c... you should not be adding any new references to specific_objects where the code does not already refer to them.) >+++ b/panels/network/net-device.c > ac = nm_device_get_active_connection (device->priv->nm_device); > if (ac) { >- return (NMConnection*)nm_remote_settings_get_connection_by_path (remote_settings, >- nm_active_connection_get_connection (ac)); >+ return (NMConnection*)ac; return (NMConnection*)nm_active_connection_get_connection (ac) (Yeah, confusing. An NMActiveConnection isn't an NMConnection, it's the binding between an NMConnection and an NMDevice.) >+++ b/panels/network/panel-common.c >+panel_get_ip4_address_as_string (NMIPConfig *ip4_config, const char *what) >+ } else if (!strcmp (what, "netmask")) { >+ GPtrArray *array; >+ NMIPRoute *route; no... this code is wrong, but nothing ever calls this function with what=="netmask" anyway so just kill it >+ array = nm_ip_config_get_addresses (ip6_config); >+ if (array == NULL || array->len < 1) FTR, all libnm functions that return GPtrArrays *always* return an array (possibly zero-length), not NULL. (This was not true with libnm-glib.)
Created attachment 327272 [details] [review] power: Port to libnm 1.2 And make the NMClient instantiation async now that the API permits it.
(In reply to Dan Winship from comment #13) > Comment on attachment 327219 [details] [review] [review] > network: Port to libnm 1.2 > > >+++ b/panels/network/cc-network-panel.c > > >- iface = nm_connection_get_virtual_iface_name (connection); > >+ iface = nm_connection_get_virtual_device_description (connection); > >+ //FIXME > > IIRC nm_connection_get_interface_name() is what you want Fixed. > >+++ b/panels/network/connection-editor/ce-page-ip4.c > > >- add_address_row (page, address, network, gateway); > >+ add_address_row (page, > >+ nm_ip_address_get_address (addr), > >+ network, > >+ nm_setting_ip_config_get_gateway (page->setting)); > > OK, so, libnm-util/libnm-glib basically had a buggy data model, which > nm-connection-editor (and then gnome-control-center) copied, in which each > manually-configured IP address has an associated gateway address. In > reality, NM always just took the first non-empty gateway value from the > address array, and completely ignored any other gateway values. libnm > represents this more accurately, by having a single gateway value which is > separate from the address array. Ideally, the editors should show it this > way as well (eg, like nmtui does). Failing that, it would be nice to at > least make it so that only the first row in the address table can have a > non-empty gateway value. This requires UI changes I would think. I've filed: https://bugzilla.gnome.org/show_bug.cgi?id=765969 about it > >+ gchar network[INET_ADDRSTRLEN + 1]; > > >+ snprintf (network, sizeof (network), "%u", nm_ip_route_get_prefix (route)); > > it's confusing to call that "network", and confusing to have it be > INET_ADDRSTRLEN-sized when it just contains a number from 0-32. I think that was me trying to minimally port the existing code. Fixed. > >+ dns_servers = g_ptr_array_new (); > ... > >+ g_ptr_array_add (dns_servers, g_strdup (text)); > ... > >+ g_ptr_array_free (dns_servers, TRUE); > > leaking the individual strings Fixed. > >+ NM_SETTING_IP_CONFIG_ADDRESSES, addresses, > >+ NM_SETTING_IP_CONFIG_ROUTES, routes, > > These have a different type in libnm than libnm-util but you didn't update > the code. (They are GPtrArrays of NMIPAddress / NMIPRoute, not GPtrArrays of > GArrays of guint32s.) Done, much nicer. > >+++ b/panels/network/connection-editor/ce-page-ip6.c > > The comments about the IP4 page also apply to the IP6 page. In general, the > two pages should have nearly identical code when using libnm, other than a > lot of s/4/6/. (And the fact that they have a different set of available > methods, and there are a few extra options in NMSettingIP6Config.) There wasn't much to do there. I've filed: https://bugzilla.gnome.org/show_bug.cgi?id=765975 though > >+++ b/panels/network/connection-editor/ce-page.c > > > g_signal_emit (page, signals[INITIALIZED], 0, error); > >+ //FIXME free error? > > probably, yes Done. > >+ if (error) { > >+ dbus_err = g_dbus_error_get_remote_error (error); > >+ ignore_error = !g_strcmp0 (dbus_err, "org.freedesktop.NetworkManager.Settings.InvalidSetting") > >+ || !g_strcmp0 (dbus_err, "org.freedesktop.NetworkManager.Settings.Connection.SettingNotFound") > >+ || !g_strcmp0 (dbus_err, "org.freedesktop.NetworkManager.AgentManager.NoSecrets"); > > D-Bus errors are mapped correctly in libnm so you can just use > g_error_matches(). Though the set of errors has also changed slightly. In > this case you want to be checking for NM_CONNECTION_ERROR_SETTING_NOT_FOUND > (which is returned if you requested the secrets for an NMSetting that wasn't > present in the connection) or NM_SECRET_AGENT_ERROR_NO_SECRETS. Fixed. > >+++ b/panels/network/net-device-ethernet.c > > >- path1 = nm_active_connection_get_connection (aconn); > >+ path1 = nm_active_connection_get_specific_object_path (aconn); > > No, that's something different... there's no API to return the path of the > NMConnection associated with an NMActiveConnection directly... > > > path2 = nm_connection_get_path (connection); > > active = g_strcmp0 (path1, path2) == 0; > > but you can change the code to compare UUIDs rather than paths Done. > >+++ b/panels/network/net-device-mobile.c > > /* is this already activated? */ > > if (active_connection != NULL && > > g_strcmp0 (nm_connection_get_path (connection), > >- nm_active_connection_get_connection (active_connection)) == 0) { > >+ nm_active_connection_get_specific_object_path (active_connection)) == 0) { > > likewise here. (and again later in device_off_toggled(), and again in > net-device-simple.c, and net-vpn.c... you should not be adding any new > references to specific_objects where the code does not already refer to > them.) Fixed all this to compare UUIDs. > >+++ b/panels/network/net-device.c > > > ac = nm_device_get_active_connection (device->priv->nm_device); > > if (ac) { > >- return (NMConnection*)nm_remote_settings_get_connection_by_path (remote_settings, > >- nm_active_connection_get_connection (ac)); > >+ return (NMConnection*)ac; > > return (NMConnection*)nm_active_connection_get_connection (ac) > > (Yeah, confusing. An NMActiveConnection isn't an NMConnection, it's the > binding between an NMConnection and an NMDevice.) That returns a NMRemoteConnection *, not an NMConnection *. > >+++ b/panels/network/panel-common.c > > >+panel_get_ip4_address_as_string (NMIPConfig *ip4_config, const char *what) > > >+ } else if (!strcmp (what, "netmask")) { > >+ GPtrArray *array; > >+ NMIPRoute *route; > > no... this code is wrong, but nothing ever calls this function with > what=="netmask" anyway so just kill it Removed, thanks. > >+ array = nm_ip_config_get_addresses (ip6_config); > >+ if (array == NULL || array->len < 1) > > FTR, all libnm functions that return GPtrArrays *always* return an array > (possibly zero-length), not NULL. (This was not true with libnm-glib.) Fixed. (In reply to Dan Winship from comment #12) > Comment on attachment 327170 [details] [review] [review] > power: Port to libnm 1.2 > > >- priv->nm_client = nm_client_new (); > >+ priv->nm_client = nm_client_new (NULL, NULL); > > g_signal_connect (priv->nm_client, "notify", > > nm_client_new() can, at least theoretically, return NULL and a GError > (though mostly only in the case of D-Bus errors, which probably can't happen > if you have a desktop that is functioning well enough to get the control > center running...) I've made it async in the new patch, and checked for errors. (In reply to Dan Winship from comment #11) > Comment on attachment 327169 [details] [review] [review] > network: Don't use deprecated NM_SETTING_WIRELESS_SEC property > > FTR I don't remember if this change is actually legitimate before the libnm > port... I'll merge this into the 1.2 port patch and be done with it.
(In reply to Bastien Nocera from comment #15) > (In reply to Dan Winship from comment #13) > > Comment on attachment 327219 [details] [review] [review] [review] > > >+++ b/panels/network/net-device.c > > > > > ac = nm_device_get_active_connection (device->priv->nm_device); > > > if (ac) { > > >- return (NMConnection*)nm_remote_settings_get_connection_by_path (remote_settings, > > >- nm_active_connection_get_connection (ac)); > > >+ return (NMConnection*)ac; > > > > return (NMConnection*)nm_active_connection_get_connection (ac) > > > > (Yeah, confusing. An NMActiveConnection isn't an NMConnection, it's the > > binding between an NMConnection and an NMDevice.) > > That returns a NMRemoteConnection *, not an NMConnection *. I've fixed this by searching for the connection with the same UUID as the ActiveConnection, attached to the NMDevice.
Created attachment 327293 [details] [review] network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature). With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
(In reply to Bastien Nocera from comment #16) > (In reply to Bastien Nocera from comment #15) > > (In reply to Dan Winship from comment #13) > > > Comment on attachment 327219 [details] [review] [review] [review] [review] > > > >+++ b/panels/network/net-device.c > > > > > > > ac = nm_device_get_active_connection (device->priv->nm_device); > > > > if (ac) { > > > >- return (NMConnection*)nm_remote_settings_get_connection_by_path (remote_settings, > > > >- nm_active_connection_get_connection (ac)); > > > >+ return (NMConnection*)ac; > > > > > > return (NMConnection*)nm_active_connection_get_connection (ac) > > > > > > (Yeah, confusing. An NMActiveConnection isn't an NMConnection, it's the > > > binding between an NMConnection and an NMDevice.) > > > > That returns a NMRemoteConnection *, not an NMConnection *. > > I've fixed this by searching for the connection with the same UUID as the > ActiveConnection, attached to the NMDevice. It implements the same interface, but the docs are incomplete (bug 765983), so fixed this up as you advised.
Created attachment 327294 [details] [review] network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature), and the use of the deprecated NM_SETTING_WIRELESS_SEC property. With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
Created attachment 327297 [details] [review] network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature), and the use of the deprecated NM_SETTING_WIRELESS_SEC property. With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
Created attachment 327301 [details] [review] network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature), and the use of the deprecated NM_SETTING_WIRELESS_SEC property. With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
Fixed a whole bunch of problems with the data types for SSID, BSSID, MAC and cloned MAC.
Comment on attachment 327272 [details] [review] power: Port to libnm 1.2 Making the initialization async means you now have to worry about priv->nm_client still being NULL in other parts of the code...
Comment on attachment 327301 [details] [review] network: Port to libnm 1.2 >+++ b/panels/network/cc-network-panel.c > if (NM_IS_VPN_CONNECTION (connection)) >- g_debug (" VPN base connection: %s", nm_active_connection_get_specific_object (connection)); >+ g_debug (" VPN base connection: %s", nm_active_connection_get_uuid (connection)); hah. sorry, when I said you didn't want to add *new* references to specific_object, I meant you should keep the existing one here (though the function is now called nm_active_connection_get_specific_object_path(). [It was renamed from get_specific_object() to allow for the possibility of a method returning the actual NMObject itself in the future rather than just its path.]) ("specific object" == "other NMObject indirectly associated with this connection". In the case of a VPN connection, it points to the NMRemoteConnection that the VPN is layered on top of. In the case of a Wi-Fi connection, it points to an NMAccessPoint.) >+++ b/panels/network/connection-editor/ce-page-details.c >- p1 = ac ? nm_active_connection_get_connection (ac) : NULL; >+ p1 = ac ? nm_active_connection_get_id (ac) : NULL; > p2 = nm_connection_get_path (CE_PAGE (page)->connection); > if (g_strcmp0 (p1, p2) == 0) { missed this before but this is another place to use uuids >+++ b/panels/network/connection-editor/ce-page-ip4.c > gchar network[INET_ADDRSTRLEN + 1]; >+ snprintf (network, sizeof (network), "%u", nm_ip_address_get_prefix (addr)); still weird (not a network, not INET_ADDRSTRLEN-sized) oh... I see. The existing code was displaying a netmask ("255.255.255.0") rather than a prefix length ("24") in that column. OK, so either you have to do it like the old code did: >- tmp_addr.s_addr = nm_utils_ip4_prefix_to_netmask (nm_ip4_address_get_prefix (addr)); >- (void) inet_ntop (AF_INET, &tmp_addr, &network[0], sizeof (network)); or change it to use the prefix rather than the netmask, like the IPv6 page does. (Although the IPv6 page still uses an addrstrlen-sized field named "network" for it. Sigh.) FTR, nm-connection-editor appears to still show netmask, but switching to use prefix instead would really be better. Also note that whatever you do here, you have to make ui_to_setting() match. (Likewise for the netmask/prefix in routes.) >- guint32 address, netmask, gateway, metric; >+ gint64 metric; > metric = 0; > if (*text_metric) { > errno = 0; >- metric = strtoul (text_metric, NULL, 10); >+ metric = g_ascii_strtoull (text_metric, NULL, 10); metric is a gint64 because it can either be any guint32 value, or -1 for "unspecified". So if the field is non-empty, it still has to be in the range 0 - G_MAXUINT32. The code currently considers it to be an error if text_metric is empty, but you could make it set the metric to -1 in that case instead. (Also update add_route_row() to match...) >+++ b/panels/network/connection-editor/ce-page.c >+ce_page_ethernet_trim_address (const gchar *addr) probably shouldn't have "_ethernet_" in the name since it's a CEPage function, not a CEPageEthernet >+ce_page_ethernet_address_is_valid (const gchar *addr) likewise >+++ b/panels/network/connection-editor/net-connection-editor.c >- settings = nm_connection_to_hash (editor->connection, NM_SETTING_HASH_FLAG_ALL); >- nm_connection_replace_settings (editor->orig_connection, settings, NULL); >- g_hash_table_destroy (settings); >+ settings = nm_connection_to_dbus (editor->orig_connection, NM_CONNECTION_SERIALIZE_ALL); >+ nm_connection_replace_settings (editor->connection, settings, NULL); >+ g_variant_unref (settings); you swapped connection and orig_connection there >+ if (nm_remote_connection_commit_changes_finish (NM_REMOTE_CONNECTION (source_object), >+ res, &error)) { >+ g_warning ("Failed to commit changes: %s", error->message); Missing "!"; (The finish func returns TRUE on success, FALSE on error.) >+ } else { >+ nm_connection_clear_secrets (NM_CONNECTION (source_object)); >+ } The existing code does that on either success or failure. >+ if (!nm_client_add_connection_finish (NM_CLIENT (source_object), res, &error)) { > g_warning ("Failed to add connection: %s", error->message); >+ success = FALSE; > /* Leave the editor open */ >- return; >+ // return; FIXME return if cancelled > } leaks error >+++ b/panels/network/net-device-wifi.c >+ secrets = nm_remote_connection_get_secrets_finish (NM_REMOTE_CONNECTION (source_object), res, &error); >+ if (!secrets) { >+ //FIXME ignore cancelled >+ //return; >+ } Need to do something here since you expect secrets to be non-NULL below
Created attachment 328441 [details] [review] power: Port to libnm 1.2 And make the NMClient instantiation async now that the API permits it.
(In reply to Dan Winship from comment #24) > Comment on attachment 327301 [details] [review] [review] > network: Port to libnm 1.2 > > >+++ b/panels/network/cc-network-panel.c > > > if (NM_IS_VPN_CONNECTION (connection)) > >- g_debug (" VPN base connection: %s", nm_active_connection_get_specific_object (connection)); > >+ g_debug (" VPN base connection: %s", nm_active_connection_get_uuid (connection)); > > hah. sorry, when I said you didn't want to add *new* references to > specific_object, I meant you should keep the existing one here (though the > function is now called nm_active_connection_get_specific_object_path(). Fixed. > [It > was renamed from get_specific_object() to allow for the possibility of a > method returning the actual NMObject itself in the future rather than just > its path.]) > > ("specific object" == "other NMObject indirectly associated with this > connection". In the case of a VPN connection, it points to the > NMRemoteConnection that the VPN is layered on top of. In the case of a Wi-Fi > connection, it points to an NMAccessPoint.) Thanks for the details. > >+++ b/panels/network/connection-editor/ce-page-details.c > > >- p1 = ac ? nm_active_connection_get_connection (ac) : NULL; > >+ p1 = ac ? nm_active_connection_get_id (ac) : NULL; > > p2 = nm_connection_get_path (CE_PAGE (page)->connection); > > if (g_strcmp0 (p1, p2) == 0) { > > missed this before but this is another place to use uuids Fixed. > >+++ b/panels/network/connection-editor/ce-page-ip4.c > > > gchar network[INET_ADDRSTRLEN + 1]; > > >+ snprintf (network, sizeof (network), "%u", nm_ip_address_get_prefix (addr)); > > still weird (not a network, not INET_ADDRSTRLEN-sized) > > oh... I see. The existing code was displaying a netmask ("255.255.255.0") > rather than a prefix length ("24") in that column. OK, so either you have to > do it like the old code did: > > >- tmp_addr.s_addr = nm_utils_ip4_prefix_to_netmask (nm_ip4_address_get_prefix (addr)); > >- (void) inet_ntop (AF_INET, &tmp_addr, &network[0], sizeof (network)); > > or change it to use the prefix rather than the netmask, like the IPv6 page > does. (Although the IPv6 page still uses an addrstrlen-sized field named > "network" for it. Sigh.) FTR, nm-connection-editor appears to still show > netmask, but switching to use prefix instead would really be better. Also > note that whatever you do here, you have to make ui_to_setting() match. > > (Likewise for the netmask/prefix in routes.) I think that we want netmask for IPv4, and prefix for IPv6 (netmask just isn't usable for IPv6). I've fixed both in netmask for addresses and routes in IPv4. I don't think that I need to change anything in the ui_to_setting() as it was expecting a netmask already, but I've filed a bug about being able to transform prefixes to netmask automatically (bug 766903). > >- guint32 address, netmask, gateway, metric; > >+ gint64 metric; > > > metric = 0; > > if (*text_metric) { > > errno = 0; > >- metric = strtoul (text_metric, NULL, 10); > >+ metric = g_ascii_strtoull (text_metric, NULL, 10); > > metric is a gint64 because it can either be any guint32 value, or -1 for > "unspecified". So if the field is non-empty, it still has to be in the range > 0 - G_MAXUINT32. The code currently considers it to be an error if > text_metric is empty, but you could make it set the metric to -1 in that > case instead. (Also update add_route_row() to match...) So 0 is a valid value? I fixed add_empty_route_row() to pass -1 by default, and only filled in the text field if metric is >= 0 in add_route_row(). > >+++ b/panels/network/connection-editor/ce-page.c > > >+ce_page_ethernet_trim_address (const gchar *addr) > > probably shouldn't have "_ethernet_" in the name since it's a CEPage > function, not a CEPageEthernet > > >+ce_page_ethernet_address_is_valid (const gchar *addr) > > likewise Fixed both cases. > >+++ b/panels/network/connection-editor/net-connection-editor.c > > >- settings = nm_connection_to_hash (editor->connection, NM_SETTING_HASH_FLAG_ALL); > >- nm_connection_replace_settings (editor->orig_connection, settings, NULL); > >- g_hash_table_destroy (settings); > >+ settings = nm_connection_to_dbus (editor->orig_connection, NM_CONNECTION_SERIALIZE_ALL); > >+ nm_connection_replace_settings (editor->connection, settings, NULL); > >+ g_variant_unref (settings); > > you swapped connection and orig_connection there Oops. Fixed. > >+ if (nm_remote_connection_commit_changes_finish (NM_REMOTE_CONNECTION (source_object), > >+ res, &error)) { > >+ g_warning ("Failed to commit changes: %s", error->message); > > Missing "!"; (The finish func returns TRUE on success, FALSE on error.) Fixed. > >+ } else { > >+ nm_connection_clear_secrets (NM_CONNECTION (source_object)); > >+ } > > The existing code does that on either success or failure. Let's do that then. > >+ if (!nm_client_add_connection_finish (NM_CLIENT (source_object), res, &error)) { > > g_warning ("Failed to add connection: %s", error->message); > >+ success = FALSE; > > /* Leave the editor open */ > >- return; > >+ // return; FIXME return if cancelled > > } > > leaks error Done, and above as well. > >+++ b/panels/network/net-device-wifi.c > > >+ secrets = nm_remote_connection_get_secrets_finish (NM_REMOTE_CONNECTION (source_object), res, &error); > >+ if (!secrets) { > >+ //FIXME ignore cancelled > >+ //return; > >+ } > > Need to do something here since you expect secrets to be non-NULL below Returns now, and frees the error. The inter-diff is here: http://paste.fedoraproject.org/371074/46426569 Full patch below.
Created attachment 328557 [details] [review] network: Port to libnm 1.2 We also remove support for WiMAX (now unsupported by NetworkManager), and InfiniBand (Enterprise feature), and the use of the deprecated NM_SETTING_WIRELESS_SEC property. With help from network-manager-applet patches by Jiří Klimeš and Dan Winship.
(In reply to Bastien Nocera from comment #26) > I think that we want netmask for IPv4 Congratulations, you're officially a UNIX greybeard. :-) (Sticking with netmask is fine, but it's the old-school choice.) > > metric is a gint64 because it can either be any guint32 value, or -1 for > > "unspecified". So if the field is non-empty, it still has to be in the range > > 0 - G_MAXUINT32. The code currently considers it to be an error if > > text_metric is empty, but you could make it set the metric to -1 in that > > case instead. (Also update add_route_row() to match...) > > So 0 is a valid value? Yes > The inter-diff is here: > http://paste.fedoraproject.org/371074/46426569 > > Full patch below. inter-diff looks good. I don't think I can slog through the full patch a third time. :-)
(In reply to Dan Winship from comment #28) > (In reply to Bastien Nocera from comment #26) > > I think that we want netmask for IPv4 > > Congratulations, you're officially a UNIX greybeard. :-) > > (Sticking with netmask is fine, but it's the old-school choice.) Weirdly enough, that's what's used in iOS and in the new Network mockups from Allan. Maybe they're all old as well? :) > > > metric is a gint64 because it can either be any guint32 value, or -1 for > > > "unspecified". So if the field is non-empty, it still has to be in the range > > > 0 - G_MAXUINT32. The code currently considers it to be an error if > > > text_metric is empty, but you could make it set the metric to -1 in that > > > case instead. (Also update add_route_row() to match...) > > > > So 0 is a valid value? > > Yes > > > The inter-diff is here: > > http://paste.fedoraproject.org/371074/46426569 > > > > Full patch below. > > inter-diff looks good. I don't think I can slog through the full patch a > third time. :-) Fair enough. That's exactly why I posted the interdiff ;)
Thanks for all the help Dan! Attachment 328441 [details] pushed as 8400d3e - power: Port to libnm 1.2 Attachment 328557 [details] pushed as 9183d34 - network: Port to libnm 1.2
Attachment 328441 [details] adds libnm to POWER_PANEL and causes configure to fail on systems that don't have NetworkManager. Is this an expected change? It can be successfully built without NetworkManager.
(In reply to Ting-Wei Lan from comment #31) > Attachment 328441 [details] adds libnm to POWER_PANEL and causes configure > to fail on systems that don't have NetworkManager. Is this an expected > change? It can be successfully built without NetworkManager. I hope you mean "without NetworkManager on a non-Linux system", because we don't support not compiling with NetworkManager on Linux. commit e39ef0eea78876bcbe1a6d0735e9d39fdd7a54ef Author: Bastien Nocera <hadess@hadess.net> Date: Sat May 28 21:50:53 2016 +0200 power: Fix build on non-Linux systems In 8400d3e, we ported the power panel to libnm 1.2, but at the same time, made libnm a hard requirement. This is a problem on non-Linux systems. See https://bugzilla.gnome.org/show_bug.cgi?id=765910#c31
(In reply to Bastien Nocera from comment #32) > (In reply to Ting-Wei Lan from comment #31) > > Attachment 328441 [details] adds libnm to POWER_PANEL and causes configure > > to fail on systems that don't have NetworkManager. Is this an expected > > change? It can be successfully built without NetworkManager. > > I hope you mean "without NetworkManager on a non-Linux system", because we > don't support not compiling with NetworkManager on Linux. Yes, "systems that don't have NetworkManager" means FreeBSD and OpenBSD here.