From 348719f7598d66a937d82d124697b467a8a5a5bd Mon Sep 17 00:00:00 2001 From: Richard Hughes Date: Fri, 27 Mar 2020 12:06:46 +0000 Subject: [PATCH] Add fu_device_retry() helper functionality Sometimes plugins need to retry various commands send to hardware, either due to unreliable transfers (e.g. using USB bulk) or from slightly quirky hardware. Between them they seem to get various things wrong; either the error messages are repeated and thus difficult to parse, or they just get the memory handling of `g_propagate_prefixed_error()` wrong. Providing sane helpers we can reduce the amount of boilerplate. Additionally we can support a 'reset' function that can try to automatically recover the hardware for specific error domains and codes. --- libfwupdplugin/fu-device.c | 122 ++++++++++++++++++++++++++++++++- libfwupdplugin/fu-device.h | 15 +++- libfwupdplugin/fu-self-test.c | 91 ++++++++++++++++++++++++ libfwupdplugin/fwupdplugin.map | 2 + 4 files changed, 228 insertions(+), 2 deletions(-) diff --git a/libfwupdplugin/fu-device.c b/libfwupdplugin/fu-device.c index c5fad00b2..153d56fe2 100644 --- a/libfwupdplugin/fu-device.c +++ b/libfwupdplugin/fu-device.c @@ -1,5 +1,5 @@ /* - * Copyright (C) 2015-2018 Richard Hughes + * Copyright (C) 2015-2020 Richard Hughes * * SPDX-License-Identifier: LGPL-2.1+ */ @@ -57,8 +57,15 @@ typedef struct { gint open_refcount; /* atomic */ GType specialized_gtype; GPtrArray *possible_plugins; + GPtrArray *retry_recs; /* of FuDeviceRetryRecovery */ } FuDevicePrivate; +typedef struct { + GQuark domain; + gint code; + FuDeviceRetryFunc recovery_func; +} FuDeviceRetryRecovery; + enum { PROP_0, PROP_STATUS, @@ -168,6 +175,117 @@ fu_device_add_possible_plugin (FuDevice *self, const gchar *plugin) g_ptr_array_add (priv->possible_plugins, g_strdup (plugin)); } +/** + * fu_device_retry_add_recovery: + * @self: A #FuDevice + * @domain: A #GQuark, or %0 for all domains + * @code: A #GError code + * @func: (scope async): A function to recover the device + * + * Sets the optional function to be called when fu_device_retry() fails, which + * is possibly a device reset. + * + * Since: 1.4.0 + **/ +void +fu_device_retry_add_recovery (FuDevice *self, + GQuark domain, + gint code, + FuDeviceRetryFunc func) +{ + FuDevicePrivate *priv = GET_PRIVATE (self); + FuDeviceRetryRecovery *rec; + + g_return_if_fail (FU_IS_DEVICE (self)); + g_return_if_fail (domain != 0); + g_return_if_fail (func != NULL); + + rec = g_new (FuDeviceRetryRecovery, 1); + rec->domain = domain; + rec->code = code; + rec->recovery_func = func; + g_ptr_array_add (priv->retry_recs, rec); +} + +/** + * fu_device_retry: + * @self: A #FuDevice + * @func: (scope async): A function to execute + * @count: The number of tries to try the function + * @user_data: (nullable): a helper to pass to @user_data + * @error: A #GError + * + * Calls a specific function a number of times, optionally handling the error + * with a reset action. + * + * If fu_device_retry_add_recovery() has not been used then all errors are + * considered non-fatal until the last try. + * + * If the reset function returns %FALSE, then the function returns straight away + * without processing any pending retries. + * + * Since: 1.4.0 + **/ +gboolean +fu_device_retry (FuDevice *self, + FuDeviceRetryFunc func, + guint count, + gpointer user_data, + GError **error) +{ + FuDevicePrivate *priv = GET_PRIVATE (self); + + g_return_val_if_fail (FU_IS_DEVICE (self), FALSE); + g_return_val_if_fail (func != NULL, FALSE); + g_return_val_if_fail (count >= 1, FALSE); + g_return_val_if_fail (error != NULL, FALSE); + + for (guint i = 0; ; i++) { + g_autoptr(GError) error_local = NULL; + + /* run function, if success return success */ + if (func (self, user_data, &error_local)) + break; + + /* sanity check */ + if (error_local == NULL) { + g_set_error (error, + G_IO_ERROR, + G_IO_ERROR_FAILED, + "exec failed but no error set!"); + return FALSE; + } + + /* too many retries */ + if (i >= count - 1) { + g_propagate_prefixed_error (error, + g_steal_pointer (&error_local), + "failed after %u retries: ", + count); + return FALSE; + } + + /* show recoverable error on the console */ + if (priv->retry_recs->len == 0) { + g_debug ("failed on try %u of %u: %s", + i + 1, count, error_local->message); + continue; + } + + /* find the condition that matches */ + for (guint j = 0; j < priv->retry_recs->len; j++) { + FuDeviceRetryRecovery *rec = g_ptr_array_index (priv->retry_recs, j); + if (g_error_matches (error_local, rec->domain, rec->code)) { + if (!rec->recovery_func (self, user_data, error)) + return FALSE; + } + } + } + + /* success */ + return TRUE; +} + /** * fu_device_poll: * @self: A #FuDevice @@ -2843,6 +2961,7 @@ fu_device_init (FuDevice *self) priv->children = g_ptr_array_new_with_free_func ((GDestroyNotify) g_object_unref); priv->parent_guids = g_ptr_array_new_with_free_func (g_free); priv->possible_plugins = g_ptr_array_new_with_free_func (g_free); + priv->retry_recs = g_ptr_array_new_with_free_func (g_free); g_rw_lock_init (&priv->parent_guids_mutex); priv->metadata = g_hash_table_new_full (g_str_hash, g_str_equal, g_free, g_free); @@ -2869,6 +2988,7 @@ fu_device_finalize (GObject *object) g_ptr_array_unref (priv->children); g_ptr_array_unref (priv->parent_guids); g_ptr_array_unref (priv->possible_plugins); + g_ptr_array_unref (priv->retry_recs); g_free (priv->alternate_id); g_free (priv->equivalent_id); g_free (priv->physical_id); diff --git a/libfwupdplugin/fu-device.h b/libfwupdplugin/fu-device.h index c0a47477b..7bbdca819 100644 --- a/libfwupdplugin/fu-device.h +++ b/libfwupdplugin/fu-device.h @@ -1,5 +1,5 @@ /* - * Copyright (C) 2015-2018 Richard Hughes + * Copyright (C) 2015-2020 Richard Hughes * * SPDX-License-Identifier: LGPL-2.1+ */ @@ -100,6 +100,10 @@ typedef enum { */ #define FU_DEVICE_REMOVE_DELAY_USER_REPLUG 40000 +typedef gboolean (*FuDeviceRetryFunc) (FuDevice *device, + gpointer user_data, + GError **error); + FuDevice *fu_device_new (void); /* helpful casting macros */ @@ -293,3 +297,12 @@ gboolean fu_device_poll (FuDevice *self, GError **error); void fu_device_set_poll_interval (FuDevice *self, guint interval); +void fu_device_retry_add_recovery (FuDevice *self, + GQuark domain, + gint code, + FuDeviceRetryFunc func); +gboolean fu_device_retry (FuDevice *self, + FuDeviceRetryFunc func, + guint count, + gpointer user_data, + GError **error); diff --git a/libfwupdplugin/fu-self-test.c b/libfwupdplugin/fu-self-test.c index 60b4b18c6..3715117da 100644 --- a/libfwupdplugin/fu-self-test.c +++ b/libfwupdplugin/fu-self-test.c @@ -1626,6 +1626,94 @@ fu_efivar_func (void) g_assert_false (ret); } +typedef struct { + guint cnt_success; + guint cnt_failed; +} FuDeviceRetryHelper; + +static gboolean +fu_device_retry_success (FuDevice *device, gpointer user_data, GError **error) +{ + FuDeviceRetryHelper *helper = (FuDeviceRetryHelper *) user_data; + helper->cnt_success++; + return TRUE; +} + +static gboolean +fu_device_retry_failed (FuDevice *device, gpointer user_data, GError **error) +{ + FuDeviceRetryHelper *helper = (FuDeviceRetryHelper *) user_data; + helper->cnt_failed++; + g_set_error_literal (error, FWUPD_ERROR, FWUPD_ERROR_INTERNAL, "failed"); + return FALSE; +} + +static gboolean +fu_device_retry_success_3rd_try (FuDevice *device, gpointer user_data, GError **error) +{ + FuDeviceRetryHelper *helper = (FuDeviceRetryHelper *) user_data; + if (helper->cnt_failed == 2) { + helper->cnt_success++; + return TRUE; + } + helper->cnt_failed++; + g_set_error_literal (error, FWUPD_ERROR, FWUPD_ERROR_INTERNAL, "failed"); + return FALSE; +} + +static void +fu_device_retry_success_func (void) +{ + gboolean ret; + g_autoptr(FuDevice) device = fu_device_new (); + g_autoptr(GError) error = NULL; + FuDeviceRetryHelper helper = { + .cnt_success = 0, + .cnt_failed = 0, + }; + fu_device_retry_add_recovery (device, FWUPD_ERROR, FWUPD_ERROR_INTERNAL, fu_device_retry_failed); + ret = fu_device_retry (device, fu_device_retry_success, 3, &helper, &error); + g_assert_no_error (error); + g_assert_true (ret); + g_assert_cmpint (helper.cnt_success, ==, 1); + g_assert_cmpint (helper.cnt_failed, ==, 0); +} + +static void +fu_device_retry_failed_func (void) +{ + gboolean ret; + g_autoptr(FuDevice) device = fu_device_new (); + g_autoptr(GError) error = NULL; + FuDeviceRetryHelper helper = { + .cnt_success = 0, + .cnt_failed = 0, + }; + fu_device_retry_add_recovery (device, FWUPD_ERROR, FWUPD_ERROR_INTERNAL, fu_device_retry_success); + ret = fu_device_retry (device, fu_device_retry_failed, 3, &helper, &error); + g_assert_error (error, FWUPD_ERROR, FWUPD_ERROR_INTERNAL); + g_assert_true (!ret); + g_assert_cmpint (helper.cnt_success, ==, 2); /* do not reset for the last failure */ + g_assert_cmpint (helper.cnt_failed, ==, 3); +} + +static void +fu_device_retry_hardware_func (void) +{ + gboolean ret; + g_autoptr(FuDevice) device = fu_device_new (); + g_autoptr(GError) error = NULL; + FuDeviceRetryHelper helper = { + .cnt_success = 0, + .cnt_failed = 0, + }; + ret = fu_device_retry (device, fu_device_retry_success_3rd_try, 3, &helper, &error); + g_assert_no_error (error); + g_assert_true (ret); + g_assert_cmpint (helper.cnt_success, ==, 1); + g_assert_cmpint (helper.cnt_failed, ==, 2); +} + int main (int argc, char **argv) { @@ -1686,5 +1774,8 @@ main (int argc, char **argv) g_test_add_func ("/fwupd/device{metadata}", fu_device_metadata_func); g_test_add_func ("/fwupd/device{open-refcount}", fu_device_open_refcount_func); g_test_add_func ("/fwupd/device{version-format}", fu_device_version_format_func); + g_test_add_func ("/fwupd/device{retry-success}", fu_device_retry_success_func); + g_test_add_func ("/fwupd/device{retry-failed}", fu_device_retry_failed_func); + g_test_add_func ("/fwupd/device{retry-hardware}", fu_device_retry_hardware_func); return g_test_run (); } diff --git a/libfwupdplugin/fwupdplugin.map b/libfwupdplugin/fwupdplugin.map index 6bee6b89e..4e066dcea 100644 --- a/libfwupdplugin/fwupdplugin.map +++ b/libfwupdplugin/fwupdplugin.map @@ -548,6 +548,8 @@ LIBFWUPDPLUGIN_1.4.0 { fu_cabinet_set_size_max; fu_device_get_root; fu_device_locker_close; + fu_device_retry; + fu_device_retry_add_recovery; fu_device_set_version_bootloader; fu_device_set_version_format; fu_device_set_version_lowest;