+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