From b4f14e8f0f242c2367a80f9e1768723ff261e34b Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Tue, 31 Mar 2020 13:11:10 +0100 Subject: [PATCH] Only set the parent ID when adopting children We use the ParentGuid quirk key to logically 'tie-together' different discrete devices into one logical device, for instance making the USB soundcard in a hub the child of the USB controller on the same PCB. Setting the discrete child is sometimes correct, for instance when rebooting the hub, the audio device also goes away -- but it's also sometimes wrong. If we set the child for a discrete device and the parent does *not* go away then we get to a situation where the child reference may no longer be valid if it comes back as a different object. When we try to remove this no-longer-valid device with the removal timeout the daemon segfaults. This is realy bad. Continue to allow using fu_device_add_child() in plugins, where we know the child lifecycle is is matched by the the parent. In the engine just set the parent ID directly and let the client use this information to show the tree of logical devices correctly. There's no benefit to setting up the children as referenced objects anyway. --- libfwupdplugin/fu-device.c | 3 +-- libfwupdplugin/fu-device.h | 2 ++ src/fu-engine.c | 4 ++-- src/fu-self-test.c | 7 +++---- 4 files changed, 8 insertions(+), 8 deletions(-) diff --git a/libfwupdplugin/fu-device.c b/libfwupdplugin/fu-device.c index f0a5262e0..59c6245ff 100644 --- a/libfwupdplugin/fu-device.c +++ b/libfwupdplugin/fu-device.c @@ -658,8 +658,7 @@ fu_device_set_parent (FuDevice *self, FuDevice *parent) priv->parent = parent; /* this is what goes over D-Bus */ - fwupd_device_set_parent_id (FWUPD_DEVICE (self), - parent != NULL ? fu_device_get_id (parent) : NULL); + fu_device_set_parent_id (self, parent != NULL ? fu_device_get_id (parent) : NULL); } /** diff --git a/libfwupdplugin/fu-device.h b/libfwupdplugin/fu-device.h index dc8665b31..c9a2e8cbc 100644 --- a/libfwupdplugin/fu-device.h +++ b/libfwupdplugin/fu-device.h @@ -117,6 +117,7 @@ FuDevice *fu_device_new (void); #define fu_device_set_description(d,v) fwupd_device_set_description(FWUPD_DEVICE(d),v) #define fu_device_set_flags(d,v) fwupd_device_set_flags(FWUPD_DEVICE(d),v) #define fu_device_set_modified(d,v) fwupd_device_set_modified(FWUPD_DEVICE(d),v) +#define fu_device_set_parent_id(d,v) fwupd_device_set_parent_id(FWUPD_DEVICE(d),v) #define fu_device_set_plugin(d,v) fwupd_device_set_plugin(FWUPD_DEVICE(d),v) #define fu_device_set_serial(d,v) fwupd_device_set_serial(FWUPD_DEVICE(d),v) #define fu_device_set_summary(d,v) fwupd_device_set_summary(FWUPD_DEVICE(d),v) @@ -142,6 +143,7 @@ FuDevice *fu_device_new (void); #define fu_device_get_serial(d) fwupd_device_get_serial(FWUPD_DEVICE(d)) #define fu_device_get_summary(d) fwupd_device_get_summary(FWUPD_DEVICE(d)) #define fu_device_get_id(d) fwupd_device_get_id(FWUPD_DEVICE(d)) +#define fu_device_get_parent_id(d) fwupd_device_get_parent_id(FWUPD_DEVICE(d)) #define fu_device_get_plugin(d) fwupd_device_get_plugin(FWUPD_DEVICE(d)) #define fu_device_get_update_error(d) fwupd_device_get_update_error(FWUPD_DEVICE(d)) #define fu_device_get_update_state(d) fwupd_device_get_update_state(FWUPD_DEVICE(d)) diff --git a/src/fu-engine.c b/src/fu-engine.c index f3f2a5b27..575e14aba 100644 --- a/src/fu-engine.c +++ b/src/fu-engine.c @@ -4361,7 +4361,7 @@ fu_engine_adopt_children (FuEngine *self, FuDevice *device) fu_device_get_id (device), fu_device_get_name (device_tmp), fu_device_get_id (device_tmp)); - fu_device_add_child (device_tmp, device); + fu_device_set_parent_id (device, fu_device_get_id (device_tmp)); break; } } @@ -4381,7 +4381,7 @@ fu_engine_adopt_children (FuEngine *self, FuDevice *device) fu_device_get_id (device_tmp), fu_device_get_name (device), fu_device_get_id (device)); - fu_device_add_child (device, device_tmp); + fu_device_set_parent_id (device_tmp, fu_device_get_id (device)); } } } diff --git a/src/fu-self-test.c b/src/fu-self-test.c index 40decb5cf..54900d5fa 100644 --- a/src/fu-self-test.c +++ b/src/fu-self-test.c @@ -956,9 +956,8 @@ fu_engine_device_parent_func (gconstpointer user_data) /* verify both children were adopted */ g_assert (fu_device_get_parent (device3) == device2); - g_assert (fu_device_get_parent (device1) == device2); g_assert_cmpstr (fu_device_get_vendor (device3), ==, "oem"); - g_assert_cmpstr (fu_device_get_vendor (device1), ==, "oem"); + g_assert_cmpstr (fu_device_get_parent_id (device1), ==, fu_device_get_id (device2)); /* verify order */ g_assert_cmpint (fu_device_get_order (device1), ==, 0); @@ -2738,11 +2737,11 @@ fu_plugin_composite_func (gconstpointer user_data) } else if (g_strcmp0 (fu_device_get_id (device), "c0a0a4aa6480ac28eea1ce164fbb466ca934e1ff") == 0) { g_assert_cmpstr (fu_device_get_version (device), ==, "1"); - g_assert_nonnull (fu_device_get_parent (device)); + g_assert_nonnull (fu_device_get_parent_id (device)); } else if (g_strcmp0 (fu_device_get_id (device), "bf455e9f371d2608d1cb67660fd2b335d3f6ef73") == 0) { g_assert_cmpstr (fu_device_get_version (device), ==, "10"); - g_assert_nonnull (fu_device_get_parent (device)); + g_assert_nonnull (fu_device_get_parent_id (device)); } }