GNOME Bugzilla – Bug 779627
[review] lr/active-connection-state-changed: Improve error reporting on connection activation
Last modified: 2017-03-17 09:26:06 UTC
Introduces a StateChanged signal to ActiveConnection and makes nmcli use it to produce better error messages. https://cgit.freedesktop.org/NetworkManager/NetworkManager/log/?h=lr/active-connection-state-changed
+state_changed_proxy (NMDBusActiveConnectionProxy *proxy, + NMActiveConnectionState state, indention + proxy = _nm_object_get_proxy (NM_OBJECT (object), NM_DBUS_INTERFACE_ACTIVE_CONNECTION); + g_signal_connect (proxy, "state-changed", + G_CALLBACK (state_changed_proxy), object); + g_object_unref (proxy); unref seems wrong. I think we should try to disconnect the signal in dispose. rest lgtm
> active-connection: emit a StateChanged signal on state changes static void _set_vpn_state (NMVpnConnection *self, VpnState vpn_state, NMVpnConnectionStateReason reason, gboolean quitting) { [...] /* Update active connection base class state */ nm_active_connection_set_state (NM_ACTIVE_CONNECTION (self), - _state_to_ac_state (vpn_state)); + _state_to_ac_state (vpn_state), + (NMActiveConnectionStateReason) reason); I'm a bit scared that when we'll add a new enum value to NMVpnConnectionStateReason, this will silently break. > cli/connections: decide about activation success and failure in single place +active_connection_state_reason_to_string (NMActiveConnectionStateReason reason) Missing 'default' label in switch.
(In reply to Beniamino Galvani from comment #2) > +active_connection_state_reason_to_string (NMActiveConnectionStateReason > reason) > > Missing 'default' label in switch. Ah right. I think omitting a "default" case in a switch is a feature. You should not *add* it. Instead, all enum values should be handled explicitly. That is, the "return _("unknown")" should be outside the switch, not in the default label.
(In reply to Thomas Haller from comment #1) > +state_changed_proxy (NMDBusActiveConnectionProxy *proxy, > + NMActiveConnectionState state, > > indention Fixed. > + proxy = _nm_object_get_proxy (NM_OBJECT (object), > NM_DBUS_INTERFACE_ACTIVE_CONNECTION); > + g_signal_connect (proxy, "state-changed", > + G_CALLBACK (state_changed_proxy), object); > + g_object_unref (proxy); > > unref seems wrong. Yeah. It copies semantics of g_dbus_object_manager_get_interface() which "seems" equally wrong. > I think we should try to disconnect the signal in dispose. Fixed. (In reply to Beniamino Galvani from comment #2) > > active-connection: emit a StateChanged signal on state changes > > static void > _set_vpn_state (NMVpnConnection *self, > VpnState vpn_state, > NMVpnConnectionStateReason reason, > gboolean quitting) > { > [...] > /* Update active connection base class state */ > nm_active_connection_set_state (NM_ACTIVE_CONNECTION (self), > - _state_to_ac_state (vpn_state)); > + _state_to_ac_state (vpn_state), > + (NMActiveConnectionStateReason) > reason); > > I'm a bit scared that when we'll add a new enum value to > NMVpnConnectionStateReason, this will silently break. Added a few commits that try to address this. Pushed an updated branch.
1e4f1892e052c69983245b14e17a88dec6e5d138