From f89cb3850b784db9102e6701b27be2a5966c39ff Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Fri, 9 Jul 2021 14:14:30 +0100 Subject: [PATCH] Add a device flag to control child removal At the moment the device list auto-removes children when the parent is removed. This was originally working around the bug that the child devices had no backend ID, and thus were not matched like the parent in fu_engine_backend_device_removed_cb() and removed automatically. Using the workaround means that the children are potentially removed before the parent depending on the order of the device list -- which might be modified by replug events. I think the old cleanup is probably an anti-pattern, and going forward composite devices should probably define _NO_AUTO_REMOVE_CHILDREN but we don't want to change this behaviour for plugins already expecting the old flow, or for devices with no parent backend IDs. --- libfwupdplugin/fu-device.c | 4 +++ libfwupdplugin/fu-device.h | 2 ++ src/fu-device-list.c | 62 +++++++++++++++++++++----------------- src/fu-self-test.c | 34 +++++++++++++++++++++ 4 files changed, 74 insertions(+), 28 deletions(-) diff --git a/libfwupdplugin/fu-device.c b/libfwupdplugin/fu-device.c index c57f32630..c55bf1f94 100644 --- a/libfwupdplugin/fu-device.c +++ b/libfwupdplugin/fu-device.c @@ -232,6 +232,8 @@ fu_device_internal_flag_to_string (FuDeviceInternalFlags flag) return "attach-extra-reset"; if (flag == FU_DEVICE_INTERNAL_FLAG_INHIBIT_CHILDREN) return "inhibit-children"; + if (flag == FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN) + return "no-auto-remove-children"; return NULL; } @@ -278,6 +280,8 @@ fu_device_internal_flag_from_string (const gchar *flag) return FU_DEVICE_INTERNAL_FLAG_ATTACH_EXTRA_RESET; if (g_strcmp0 (flag, "inhibit-children") == 0) return FU_DEVICE_INTERNAL_FLAG_INHIBIT_CHILDREN; + if (g_strcmp0 (flag, "no-auto-remove-children") == 0) + return FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN; return FU_DEVICE_INTERNAL_FLAG_UNKNOWN; } diff --git a/libfwupdplugin/fu-device.h b/libfwupdplugin/fu-device.h index 387d52970..ad2e08188 100644 --- a/libfwupdplugin/fu-device.h +++ b/libfwupdplugin/fu-device.h @@ -238,6 +238,7 @@ FuDevice *fu_device_new (void); * @FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN: Automatically assign the parent for children of this device * @FU_DEVICE_INTERNAL_FLAG_ATTACH_EXTRA_RESET: Device needs resetting twice for attach after the firmware update * @FU_DEVICE_INTERNAL_FLAG_INHIBIT_CHILDREN: Children of the device are inhibited by the parent + * @FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN: Do not auto-remove clildren in the device list * * The device internal flags. **/ @@ -258,6 +259,7 @@ typedef enum { FU_DEVICE_INTERNAL_FLAG_AUTO_PARENT_CHILDREN = (1llu << 12), /* Since: 1.6.2 */ FU_DEVICE_INTERNAL_FLAG_ATTACH_EXTRA_RESET = (1llu << 13), /* Since: 1.6.2 */ FU_DEVICE_INTERNAL_FLAG_INHIBIT_CHILDREN = (1llu << 14), /* Since: 1.6.2 */ + FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN = (1llu << 15), /* Since: 1.6.2 */ /*< private >*/ FU_DEVICE_INTERNAL_FLAG_UNKNOWN = G_MAXUINT64, } FuDeviceInternalFlags; diff --git a/src/fu-device-list.c b/src/fu-device-list.c index f7cacac79..c62430afe 100644 --- a/src/fu-device-list.c +++ b/src/fu-device-list.c @@ -373,26 +373,29 @@ fu_device_list_device_delayed_remove_cb (gpointer user_data) { FuDeviceItem *item = (FuDeviceItem *) user_data; FuDeviceList *self = FU_DEVICE_LIST (item->self); - GPtrArray *children; /* no longer valid */ item->remove_id = 0; /* remove any children associated with device */ - children = fu_device_get_children (item->device); - for (guint j = 0; j < children->len; j++) { - FuDevice *child = g_ptr_array_index (children, j); - FuDeviceItem *child_item = fu_device_list_find_by_id (self, - fu_device_get_id (child), - NULL); - if (child_item == NULL) { - g_debug ("device %s not found", fu_device_get_id (child)); - continue; + if (!fu_device_has_internal_flag (item->device, + FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN)) { + GPtrArray *children = fu_device_get_children (item->device); + for (guint j = 0; j < children->len; j++) { + FuDevice *child = g_ptr_array_index (children, j); + FuDeviceItem *child_item; + child_item = fu_device_list_find_by_id (self, + fu_device_get_id (child), + NULL); + if (child_item == NULL) { + g_debug ("device %s not found", fu_device_get_id (child)); + continue; + } + fu_device_list_emit_device_removed (self, child); + g_rw_lock_writer_lock (&self->devices_mutex); + g_ptr_array_remove (self->devices, child_item); + g_rw_lock_writer_unlock (&self->devices_mutex); } - fu_device_list_emit_device_removed (self, child); - g_rw_lock_writer_lock (&self->devices_mutex); - g_ptr_array_remove (self->devices, child_item); - g_rw_lock_writer_unlock (&self->devices_mutex); } /* just remove now */ @@ -440,7 +443,6 @@ void fu_device_list_remove (FuDeviceList *self, FuDevice *device) { FuDeviceItem *item; - GPtrArray *children; g_return_if_fail (FU_IS_DEVICE_LIST (self)); g_return_if_fail (FU_IS_DEVICE (device)); @@ -465,20 +467,24 @@ fu_device_list_remove (FuDeviceList *self, FuDevice *device) } /* remove any children associated with device */ - children = fu_device_get_children (device); - for (guint j = 0; j < children->len; j++) { - FuDevice *child = g_ptr_array_index (children, j); - FuDeviceItem *child_item = fu_device_list_find_by_id (self, - fu_device_get_id (child), - NULL); - if (child_item == NULL) { - g_debug ("device %s not found", fu_device_get_id (child)); - continue; + if (!fu_device_has_internal_flag (item->device, + FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN)) { + GPtrArray *children = fu_device_get_children (device); + for (guint j = 0; j < children->len; j++) { + FuDevice *child = g_ptr_array_index (children, j); + FuDeviceItem *child_item; + child_item = fu_device_list_find_by_id (self, + fu_device_get_id (child), + NULL); + if (child_item == NULL) { + g_debug ("device %s not found", fu_device_get_id (child)); + continue; + } + fu_device_list_emit_device_removed (self, child); + g_rw_lock_writer_lock (&self->devices_mutex); + g_ptr_array_remove (self->devices, child_item); + g_rw_lock_writer_unlock (&self->devices_mutex); } - fu_device_list_emit_device_removed (self, child); - g_rw_lock_writer_lock (&self->devices_mutex); - g_ptr_array_remove (self->devices, child_item); - g_rw_lock_writer_unlock (&self->devices_mutex); } /* remove right now */ diff --git a/src/fu-self-test.c b/src/fu-self-test.c index 10d2f7ce2..a6451d7ce 100644 --- a/src/fu-self-test.c +++ b/src/fu-self-test.c @@ -2083,6 +2083,38 @@ _device_list_count_cb (FuDeviceList *device_list, FuDevice *device, gpointer use (*cnt)++; } +static void +fu_device_list_no_auto_remove_children_func (gconstpointer user_data) +{ + g_autoptr(FuDevice) child = fu_device_new (); + g_autoptr(FuDevice) parent = fu_device_new (); + g_autoptr(FuDeviceList) device_list = fu_device_list_new (); + g_autoptr(GPtrArray) active1 = NULL; + g_autoptr(GPtrArray) active2 = NULL; + g_autoptr(GPtrArray) active3 = NULL; + + /* normal behavior, remove child with parent */ + fu_device_set_id (parent, "parent"); + fu_device_set_id (child, "child"); + fu_device_add_child (parent, child); + fu_device_list_add (device_list, parent); + fu_device_list_add (device_list, child); + fu_device_list_remove (device_list, parent); + active1 = fu_device_list_get_active (device_list); + g_assert_cmpint (active1->len, ==, 0); + + /* new-style behavior, do not remove child */ + fu_device_add_internal_flag (parent, FU_DEVICE_INTERNAL_FLAG_NO_AUTO_REMOVE_CHILDREN); + fu_device_list_add (device_list, parent); + fu_device_list_add (device_list, child); + fu_device_list_remove (device_list, parent); + active2 = fu_device_list_get_active (device_list); + g_assert_cmpint (active2->len, ==, 1); + fu_device_list_remove (device_list, child); + active3 = fu_device_list_get_active (device_list); + g_assert_cmpint (active3->len, ==, 0); +} + static void fu_device_list_delay_func (gconstpointer user_data) { @@ -3296,6 +3328,8 @@ main (int argc, char **argv) fu_device_list_func); g_test_add_data_func ("/fwupd/device-list{delay}", self, fu_device_list_delay_func); + g_test_add_data_func ("/fwupd/device-list{no-auto-remove-children}", self, + fu_device_list_no_auto_remove_children_func); g_test_add_data_func ("/fwupd/device-list{compatible}", self, fu_device_list_compatible_func); g_test_add_data_func ("/fwupd/device-list{remove-chain}", self,