fixup! libnm/client: don't reset properties when interface goes awayth/pr/1276

We want that when a D-Bus interface get removed (which in practice
means, the D-Bus object got removed altogether), that the properties are
frozen in time. Previously, they were reset to the default, which isn't
very useful.

... Except for the complex properties that refer other objects. We need
to break the reference cycles.

Previously, the patch only checked for the D-Bus types, and special
handled "o" and "ao" types. That seems wrong to me.

1) at the point where the check was performed, we didn't
  yet check whether the variant has the expected signature. If (due
  to a bug or an API break) the type changes, we still want to reset
  the property.
  For that, we would have to call_obj_handle_dbus_prop_changes() and
  not shortcut.

2) we have handlers, I think those handlers should be asked to handle
  the removal, and not the calling code decide that it's not necessary.

2b) nm_active_connection_get_specific_object_path() is D-Bus type "o",
  but the libnm property is a plain string. For this we should also
  freeze the value.

2b) is actually the only exception, where it really makes a difference.
Maybe that is a weak justification for this large patch, compared to the "simpler"
variant before. I think the "simpler" variant is however a bit "wrong", and
doing the wrong thing is usually simpler.

6 files changed, 88 insertions, 12 deletions

diff --git a/src/libnm-client-impl/nm-access-point.c b/src/libnm-client-impl/nm-access-point.c
index 94b38f0300..a30c9dbb4b 100644
--- a/src/libnm-client-impl/nm-access-point.c
+++ b/src/libnm-client-impl/nm-access-point.c
@@ -399,11 +399,15 @@ _notify_update_prop_hw_address(NMClient               *client,
                                NMLDBusObject          *dbobj,
                                const NMLDBusMetaIface *meta_iface,
                                guint                   dbus_property_idx,
+                               gboolean                is_removed,
                                GVariant               *value)
 {
     NMAccessPoint        *self = NM_ACCESS_POINT(dbobj->nmobj);
     NMAccessPointPrivate *priv = NM_ACCESS_POINT_GET_PRIVATE(self);
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     g_free(priv->bssid);
     priv->bssid = value ? g_variant_dup_string(value, NULL) : 0u;
     _notify(self, PROP_HW_ADDRESS);
diff --git a/src/libnm-client-impl/nm-client.c b/src/libnm-client-impl/nm-client.c
index 7935b4bb81..9ac2ee0131 100644
--- a/src/libnm-client-impl/nm-client.c
+++ b/src/libnm-client-impl/nm-client.c
@@ -2364,6 +2364,7 @@ _nml_dbus_notify_update_prop_ignore(NMClient               *self,
                                     NMLDBusObject          *dbobj,
                                     const NMLDBusMetaIface *meta_iface,
                                     guint                   dbus_property_idx,
+                                    gboolean                is_removed,
                                     GVariant               *value)
 {
     return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
@@ -2374,11 +2375,15 @@ _nml_dbus_notify_update_prop_o(NMClient               *self,
                                NMLDBusObject          *dbobj,
                                const NMLDBusMetaIface *meta_iface,
                                guint                   dbus_property_idx,
+                               gboolean                is_removed,
                                GVariant               *value)
 {
     const char   *path = NULL;
     NMRefString **p_property;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     if (value)
         path = g_variant_get_string(value, NULL);
 
@@ -2409,9 +2414,22 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
     const char                  *dbus_type_s;
     const GParamSpec            *param_spec;
     NMLDBusNotifyUpdatePropFlags notify_update_prop_flags;
+    const gboolean               is_removed = (!value);
 
     nm_assert(G_IS_OBJECT(dbobj->nmobj));
 
+#define _HANDLE_IS_REMOVED(is_removed)                                       \
+    G_STMT_START                                                             \
+    {                                                                        \
+        /* The D-Bus interface was removed, when @value was originally NULL.
+         * In that case, we only want to reset complex properties ("o"
+         * and "ao"), which refer to other objects. Plain properties
+         * are frozen at the last value, and we do nothing about them */ \
+        if (is_removed)                                                      \
+            return;                                                          \
+    }                                                                        \
+    G_STMT_END
+
     if (value && !g_variant_is_of_type(value, meta_property->dbus_type)) {
         NML_NMCLIENT_LOG_E(self,
                            "[%s] property %s.%s expected of type \"%s\" but is \"%s\". Ignore",
@@ -2424,8 +2442,12 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
     }
 
     if (meta_property->notify_update_prop) {
-        notify_update_prop_flags =
-            meta_property->notify_update_prop(self, dbobj, meta_iface, dbus_property_idx, value);
+        notify_update_prop_flags = meta_property->notify_update_prop(self,
+                                                                     dbobj,
+                                                                     meta_iface,
+                                                                     dbus_property_idx,
+                                                                     is_removed,
+                                                                     value);
         if (notify_update_prop_flags == NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE)
             return;
 
@@ -2451,6 +2473,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
     switch (dbus_type_s[0]) {
     case 'b':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((bool *) p_property) = g_variant_get_boolean(value);
         else if (param_spec->value_type == G_TYPE_BOOLEAN)
@@ -2462,6 +2485,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 'y':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((guint8 *) p_property) = g_variant_get_byte(value);
         else {
@@ -2471,6 +2495,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 'q':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((guint16 *) p_property) = g_variant_get_uint16(value);
         else {
@@ -2480,6 +2505,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 'i':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((gint32 *) p_property) = g_variant_get_int32(value);
         else if (param_spec->value_type == G_TYPE_INT)
@@ -2491,6 +2517,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 'u':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((guint32 *) p_property) = g_variant_get_uint32(value);
         else {
@@ -2500,6 +2527,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 'x':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((gint64 *) p_property) = g_variant_get_int64(value);
         else if (param_spec->value_type == G_TYPE_INT64)
@@ -2511,6 +2539,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 't':
         nm_assert(dbus_type_s[1] == '\0');
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((guint64 *) p_property) = g_variant_get_uint64(value);
         else {
@@ -2521,6 +2550,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
     case 's':
         nm_assert(dbus_type_s[1] == '\0');
         nm_clear_g_free((char **) p_property);
+        _HANDLE_IS_REMOVED(is_removed);
         if (value)
             *((char **) p_property) = g_variant_dup_string(value, NULL);
         else {
@@ -2530,6 +2560,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         break;
     case 'o':
         nm_assert(dbus_type_s[1] == '\0');
+        /* Don't call _HANDLE_IS_REMOVED(), because we want to break reference cycles. */
         notify_update_prop_flags = nml_dbus_property_o_notify(self,
                                                               p_property,
                                                               dbobj,
@@ -2541,6 +2572,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
         switch (dbus_type_s[1]) {
         case 'y':
             nm_assert(dbus_type_s[2] == '\0');
+            _HANDLE_IS_REMOVED(is_removed);
             {
                 gconstpointer v;
                 gsize         l;
@@ -2563,6 +2595,8 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
             nm_assert(dbus_type_s[2] == '\0');
             nm_assert(param_spec->value_type == G_TYPE_STRV);
 
+            _HANDLE_IS_REMOVED(is_removed);
+
             g_strfreev(*((char ***) p_property));
             if (value)
                 *((char ***) p_property) = g_variant_dup_strv(value, NULL);
@@ -2571,6 +2605,7 @@ _obj_handle_dbus_prop_changes(NMClient            *self,
             break;
         case 'o':
             nm_assert(dbus_type_s[2] == '\0');
+            /* Don't call _HANDLE_IS_REMOVED(), because we want to break reference cycles. */
             notify_update_prop_flags = nml_dbus_property_ao_notify(self,
                                                                    p_property,
                                                                    dbobj,
@@ -2630,16 +2665,7 @@ _obj_handle_dbus_iface_changes(NMClient            *self,
 
     if (is_removed) {
         for (i_prop = 0; i_prop < db_iface_data->dbus_iface.meta->n_dbus_properties; i_prop++) {
-            const GVariantType *dbus_type =
-                db_iface_data->dbus_iface.meta->dbus_properties[i_prop].dbus_type;
-
-            /* Unset properties that can potentially contain objects, to release them,
-             * but keep the rest around, because it might still make sense to know what
-             * they were (e.g. when a device has been removed we'd like know what interface
-             * name it had, or keep the state to avoid spurious state change into UNKNOWN). */
-            if (g_variant_type_is_array(dbus_type)
-                || g_variant_type_equal(dbus_type, G_VARIANT_TYPE_OBJECT_PATH))
-                _obj_handle_dbus_prop_changes(self, dbobj, db_iface_data, i_prop, NULL);
+            _obj_handle_dbus_prop_changes(self, dbobj, db_iface_data, i_prop, NULL);
         }
     } else {
         while ((db_prop_data = c_list_first_entry(&db_iface_data->changed_prop_lst_head,
@@ -6201,6 +6227,7 @@ _notify_update_prop_dns_manager_configuration(NMClient               *self,
                                               NMLDBusObject          *dbobj,
                                               const NMLDBusMetaIface *meta_iface,
                                               guint                   dbus_property_idx,
+                                              gboolean                is_removed,
                                               GVariant               *value)
 {
     NMClientPrivate             *priv              = NM_CLIENT_GET_PRIVATE(self);
@@ -6209,6 +6236,9 @@ _notify_update_prop_dns_manager_configuration(NMClient               *self,
 
     nm_assert(G_OBJECT(self) == dbobj->nmobj);
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     if (value) {
         GVariant    *entry_var_tmp;
         GVariantIter iter;
@@ -6304,12 +6334,16 @@ _notify_update_prop_nm_capabilities(NMClient               *self,
                                     NMLDBusObject          *dbobj,
                                     const NMLDBusMetaIface *meta_iface,
                                     guint                   dbus_property_idx,
+                                    gboolean                is_removed,
                                     GVariant               *value)
 {
     NMClientPrivate *priv = NM_CLIENT_GET_PRIVATE(self);
 
     nm_assert(G_OBJECT(self) == dbobj->nmobj);
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     nm_clear_g_free(&priv->nm.capabilities_arr);
     priv->nm.capabilities_len = 0;
 
diff --git a/src/libnm-client-impl/nm-device.c b/src/libnm-client-impl/nm-device.c
index 238e7c1709..65d6000a73 100644
--- a/src/libnm-client-impl/nm-device.c
+++ b/src/libnm-client-impl/nm-device.c
@@ -190,6 +190,7 @@ _notify_update_prop_state_reason(NMClient               *client,
                                  NMLDBusObject          *dbobj,
                                  const NMLDBusMetaIface *meta_iface,
                                  guint                   dbus_property_idx,
+                                 gboolean                is_removed,
                                  GVariant               *value)
 {
     NMDevice        *self      = NM_DEVICE(dbobj->nmobj);
@@ -197,6 +198,9 @@ _notify_update_prop_state_reason(NMClient               *client,
     guint32          new_state = NM_DEVICE_STATE_UNKNOWN;
     guint32          reason    = NM_DEVICE_STATE_REASON_NONE;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     /* We ignore the "State" property and the "StateChanged" signal of the device.
      * This information is redundant to the "StateReason" property, and we rely
      * on that one alone. In the best case, the information is identical. If it
@@ -234,6 +238,7 @@ _notify_update_prop_lldp_neighbors(NMClient               *client,
                                    NMLDBusObject          *dbobj,
                                    const NMLDBusMetaIface *meta_iface,
                                    guint                   dbus_property_idx,
+                                   gboolean                is_removed,
                                    GVariant               *value)
 {
     NMDevice                    *self = NM_DEVICE(dbobj->nmobj);
@@ -243,6 +248,9 @@ _notify_update_prop_lldp_neighbors(NMClient               *client,
     GVariantIter *attrs_iter;
     GVariantIter  iter;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     new = g_ptr_array_new_with_free_func((GDestroyNotify) nm_lldp_neighbor_unref);
 
     if (value) {
@@ -1300,6 +1308,7 @@ _nm_device_notify_update_prop_hw_address(NMClient               *client,
                                          NMLDBusObject          *dbobj,
                                          const NMLDBusMetaIface *meta_iface,
                                          guint                   dbus_property_idx,
+                                         gboolean                is_removed,
                                          GVariant               *value)
 {
     NMDevice        *self    = NM_DEVICE(dbobj->nmobj);
@@ -1307,6 +1316,9 @@ _nm_device_notify_update_prop_hw_address(NMClient               *client,
     gboolean         is_new  = (meta_iface == &_nml_dbus_meta_iface_nm_device);
     gboolean         changed = FALSE;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     if (!is_new && priv->hw_address_is_new) {
         /* once the instance is marked to honor the new property, the
          * changed signal for the old variant gets ignored. */
@@ -1364,6 +1376,7 @@ _nm_device_notify_update_prop_ports(NMClient               *client,
                                     NMLDBusObject          *dbobj,
                                     const NMLDBusMetaIface *meta_iface,
                                     guint                   dbus_property_idx,
+                                    gboolean                is_removed,
                                     GVariant               *value)
 {
     const NMLDBusMetaProperty *meta_property =
diff --git a/src/libnm-client-impl/nm-dhcp-config.c b/src/libnm-client-impl/nm-dhcp-config.c
index 1f90849101..485f3663b0 100644
--- a/src/libnm-client-impl/nm-dhcp-config.c
+++ b/src/libnm-client-impl/nm-dhcp-config.c
@@ -34,11 +34,15 @@ _notify_update_prop_options(NMClient               *client,
                             NMLDBusObject          *dbobj,
                             const NMLDBusMetaIface *meta_iface,
                             guint                   dbus_property_idx,
+                            gboolean                is_removed,
                             GVariant               *value)
 {
     NMDhcpConfig        *self = NM_DHCP_CONFIG(dbobj->nmobj);
     NMDhcpConfigPrivate *priv = NM_DHCP_CONFIG_GET_PRIVATE(self);
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     g_hash_table_remove_all(priv->options);
 
     if (value) {
diff --git a/src/libnm-client-impl/nm-ip-config.c b/src/libnm-client-impl/nm-ip-config.c
index 9aa2e778ce..837f3dfbab 100644
--- a/src/libnm-client-impl/nm-ip-config.c
+++ b/src/libnm-client-impl/nm-ip-config.c
@@ -55,6 +55,7 @@ _notify_update_prop_addresses(NMClient               *client,
                               NMLDBusObject          *dbobj,
                               const NMLDBusMetaIface *meta_iface,
                               guint                   dbus_property_idx,
+                              gboolean                is_removed,
                               GVariant               *value)
 {
     NMIPConfig                  *self          = NM_IP_CONFIG(dbobj->nmobj);
@@ -64,6 +65,9 @@ _notify_update_prop_addresses(NMClient               *client,
     int      addr_family = meta_iface == &_nml_dbus_meta_iface_nm_ip4config ? AF_INET : AF_INET6;
     gboolean new_style;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     new_style =
         (((const char *) meta_iface->dbus_properties[dbus_property_idx].dbus_type)[2] == '{');
 
@@ -95,6 +99,7 @@ _notify_update_prop_routes(NMClient               *client,
                            NMLDBusObject          *dbobj,
                            const NMLDBusMetaIface *meta_iface,
                            guint                   dbus_property_idx,
+                           gboolean                is_removed,
                            GVariant               *value)
 {
     NMIPConfig                  *self       = NM_IP_CONFIG(dbobj->nmobj);
@@ -104,6 +109,9 @@ _notify_update_prop_routes(NMClient               *client,
     int      addr_family = meta_iface == &_nml_dbus_meta_iface_nm_ip4config ? AF_INET : AF_INET6;
     gboolean new_style;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     new_style =
         (((const char *) meta_iface->dbus_properties[dbus_property_idx].dbus_type)[2] == '{');
 
@@ -135,6 +143,7 @@ _notify_update_prop_nameservers(NMClient               *client,
                                 NMLDBusObject          *dbobj,
                                 const NMLDBusMetaIface *meta_iface,
                                 guint                   dbus_property_idx,
+                                gboolean                is_removed,
                                 GVariant               *value)
 {
     NMIPConfig        *self            = NM_IP_CONFIG(dbobj->nmobj);
@@ -143,6 +152,9 @@ _notify_update_prop_nameservers(NMClient               *client,
     gboolean           new_style       = TRUE;
     int addr_family = meta_iface == &_nml_dbus_meta_iface_nm_ip4config ? AF_INET : AF_INET6;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     if (addr_family == AF_INET) {
         new_style =
             (((const char *) meta_iface->dbus_properties[dbus_property_idx].dbus_type)[1] == 'a');
@@ -205,6 +217,7 @@ _notify_update_prop_wins_servers(NMClient               *client,
                                  NMLDBusObject          *dbobj,
                                  const NMLDBusMetaIface *meta_iface,
                                  guint                   dbus_property_idx,
+                                 gboolean                is_removed,
                                  GVariant               *value)
 {
     NMIPConfig        *self             = NM_IP_CONFIG(dbobj->nmobj);
@@ -212,6 +225,9 @@ _notify_update_prop_wins_servers(NMClient               *client,
     gs_strfreev char **wins_servers_new = NULL;
     gboolean           new_style;
 
+    if (is_removed)
+        return NML_DBUS_NOTIFY_UPDATE_PROP_FLAGS_NONE;
+
     new_style =
         (((const char *) meta_iface->dbus_properties[dbus_property_idx].dbus_type)[1] == 's');
 
diff --git a/src/libnm-client-impl/nm-libnm-utils.h b/src/libnm-client-impl/nm-libnm-utils.h
index da13c615cb..1856f7abaf 100644
--- a/src/libnm-client-impl/nm-libnm-utils.h
+++ b/src/libnm-client-impl/nm-libnm-utils.h
@@ -325,12 +325,14 @@ NMLDBusNotifyUpdatePropFlags _nml_dbus_notify_update_prop_ignore(NMClient
                                                                  NMLDBusObject          *dbobj,
                                                                  const NMLDBusMetaIface *meta_iface,
                                                                  guint     dbus_property_idx,
+                                                                 gboolean  is_removed,
                                                                  GVariant *value);
 
 NMLDBusNotifyUpdatePropFlags _nml_dbus_notify_update_prop_o(NMClient               *client,
                                                             NMLDBusObject          *dbobj,
                                                             const NMLDBusMetaIface *meta_iface,
                                                             guint     dbus_property_idx,
+                                                            gboolean  is_removed,
                                                             GVariant *value);
 
 NMLDBusNotifyUpdatePropFlags nml_dbus_property_ao_notify(NMClient               *self,
@@ -348,6 +350,7 @@ typedef struct {
                                                        NMLDBusObject          *dbobj,
                                                        const NMLDBusMetaIface *meta_iface,
                                                        guint                   dbus_property_idx,
+                                                       gboolean                is_removed,
                                                        GVariant               *value);
 
     guint16 prop_struct_offset;
@@ -1043,12 +1046,14 @@ _nm_device_notify_update_prop_hw_address(NMClient               *client,
                                          NMLDBusObject          *dbobj,
                                          const NMLDBusMetaIface *meta_iface,
                                          guint                   dbus_property_idx,
+                                         gboolean                is_removed,
                                          GVariant               *value);
 
 NMLDBusNotifyUpdatePropFlags _nm_device_notify_update_prop_ports(NMClient               *client,
                                                                  NMLDBusObject          *dbobj,
                                                                  const NMLDBusMetaIface *meta_iface,
                                                                  guint     dbus_property_idx,
+                                                                 gboolean  is_removed,
                                                                  GVariant *value);
 
 /*****************************************************************************/