libqmi-glib,net-port-manager-qmiwwan: simplify add/del operations

The original logic assumed that after the sysfs add_mux/del_mux write
operation, the link interface would require some time to show up in
sysfs, and so we would attempt to always run one by one these
operations, so that e.g. we could keep track of what mux_id was
requested and what link name was generated.

But this doesn't seem to be true; add_mux/del_mux seem to guarantee
that as soon as the write operation finishes successfully, the new
link is already available (for add_mux) or the given link was already
removed (for del_mux).

So just simplify the logic, and run the whole add/del logic in a sync
way without forcing an artificial 250ms timeout to complete each
operation.

1 files changed, 107 insertions, 351 deletions

diff --git a/src/libqmi-glib/qmi-net-port-manager-qmiwwan.c b/src/libqmi-glib/qmi-net-port-manager-qmiwwan.c
index dc6903d..f6d1466 100644
--- a/src/libqmi-glib/qmi-net-port-manager-qmiwwan.c
+++ b/src/libqmi-glib/qmi-net-port-manager-qmiwwan.c
@@ -38,13 +38,6 @@ struct _QmiNetPortManagerQmiwwanPrivate {
     gchar *add_mux_sysfs_path;
     gchar *del_mux_sysfs_path;
 
-    /* We don't allow running link operations in parallel, because the qmi_wwan
-     * add_mux/del_mux may be a bit racy. The races may already happen if there
-     * are additional programs trying to do the same, but that's something we'll
-     * try to live with. */
-    gboolean  running;
-    GList    *pending_tasks;
-
     /* mux id tracking table */
     GHashTable *mux_id_map;
 };
@@ -243,108 +236,6 @@ get_first_free_mux_id (QmiNetPortManagerQmiwwan  *self,
 
 /*****************************************************************************/
 
-#define LINK_OPERATION_TIMEOUT_STEP_MS 250
-
-typedef enum {
-    LINK_OPERATION_TYPE_ADD,
-    LINK_OPERATION_TYPE_DEL,
-} LinkOperationType;
-
-typedef struct {
-    LinkOperationType  type;
-    guint              timeout_ms;
-    guint              timeout_ms_elapsed;
-    GSource           *timeout_source;
-    GPtrArray         *links_before;
-    gchar             *mux_id;
-    gchar             *link_iface;
-} LinkOperationContext;
-
-static void
-link_operation_context_free (LinkOperationContext *ctx)
-{
-    g_free (ctx->mux_id);
-    g_free (ctx->link_iface);
-    if (ctx->links_before)
-        g_ptr_array_unref (ctx->links_before);
-    if (ctx->timeout_source) {
-        g_source_destroy (ctx->timeout_source);
-        g_source_unref (ctx->timeout_source);
-    }
-    g_slice_free (LinkOperationContext, ctx);
-}
-
-static void run_add_link (GTask *task);
-static void run_del_link (GTask *task);
-
-static void
-link_operation_completed (QmiNetPortManagerQmiwwan *self)
-{
-    LinkOperationContext *ctx;
-    GTask                *task;
-
-    g_assert (self->priv->running);
-    self->priv->running = FALSE;
-
-    if (!self->priv->pending_tasks)
-        return;
-
-    task = self->priv->pending_tasks->data;
-    self->priv->pending_tasks = g_list_delete_link (self->priv->pending_tasks, self->priv->pending_tasks);
-    g_assert (task);
-
-    self->priv->running = TRUE;
-
-    ctx = g_task_get_task_data (task);
-    if (ctx->type == LINK_OPERATION_TYPE_ADD)
-        run_add_link (task);
-    else if (ctx->type == LINK_OPERATION_TYPE_DEL)
-        run_del_link (task);
-    else
-        g_assert_not_reached ();
-}
-
-static GTask *
-link_operation_new (QmiNetPortManagerQmiwwan *self,
-                    const gchar              *base_ifname,
-                    guint                     timeout,
-                    GCancellable             *cancellable,
-                    GAsyncReadyCallback       callback,
-                    gpointer                  user_data)
-{
-    GTask                *task;
-    LinkOperationContext *ctx;
-
-    task = g_task_new (self, cancellable, callback, user_data);
-
-    /* validate base ifname before doing anything else */
-    if (base_ifname && !g_str_equal (base_ifname, self->priv->iface)) {
-        g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_INVALID_ARGS,
-                                 "Invalid base interface given: '%s' (must be '%s')",
-                                 base_ifname, self->priv->iface);
-        g_object_unref (task);
-        return NULL;
-    }
-
-    /* Setup a completion signal handler so that we process the pending tasks
-     * list once a task has been completed. The self pointer is guaranteed to
-     * be valid because the task holds a full reference. */
-    g_signal_connect_swapped (task,
-                              "notify::completed",
-                              G_CALLBACK (link_operation_completed),
-                              self);
-
-    ctx = g_slice_new0 (LinkOperationContext);
-    ctx->type = base_ifname ? LINK_OPERATION_TYPE_ADD : LINK_OPERATION_TYPE_DEL;;
-    ctx->timeout_ms = timeout * 1000;
-
-    g_task_set_task_data (task, ctx, (GDestroyNotify)link_operation_context_free);
-
-    return task;
-}
-
-/*****************************************************************************/
-
 static gchar *
 net_port_manager_add_link_finish (QmiNetPortManager  *self,
                                   guint              *mux_id,
@@ -357,171 +248,101 @@ net_port_manager_add_link_finish (QmiNetPortManager  *self,
     if (!link_name)
         return NULL;
 
-    if (mux_id) {
-        LinkOperationContext *ctx;
-
-        ctx = g_task_get_task_data (G_TASK (res));
-        *mux_id = strtoul (ctx->mux_id, NULL, 16);
-    }
+    if (mux_id)
+        *mux_id = GPOINTER_TO_UINT (g_task_get_task_data (G_TASK (res)));
 
     return link_name;
 }
 
-static gboolean
-add_link_check_timeout (GTask *task)
+static void
+net_port_manager_add_link (QmiNetPortManager   *_self,
+                           guint                mux_id,
+                           const gchar         *base_ifname,
+                           const gchar         *ifname_prefix,
+                           guint                timeout,
+                           GCancellable        *cancellable,
+                           GAsyncReadyCallback  callback,
+                           gpointer             user_data)
 {
-    QmiNetPortManagerQmiwwan *self;
-    LinkOperationContext     *ctx;
+    QmiNetPortManagerQmiwwan *self = QMI_NET_PORT_MANAGER_QMIWWAN (_self);
+    GTask                    *task;
     GError                   *error = NULL;
+    g_autoptr(GPtrArray)      links_before = NULL;
     g_autoptr(GPtrArray)      links_after = NULL;
     g_autofree gchar         *link_name = NULL;
+    g_autofree gchar         *mux_id_str = NULL;
 
-    if (g_task_return_error_if_cancelled (task)) {
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    self = g_task_get_source_object (task);
-    ctx  = g_task_get_task_data (task);
-
-    if (!qmi_helpers_list_links (self->priv->sysfs_file,
-                                 g_task_get_cancellable (task),
-                                 ctx->links_before,
-                                 &links_after,
-                                 &error)) {
-        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory: ");
-        g_task_return_error (task, error);
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    if (!lookup_mux_id_in_links (links_after, ctx->mux_id, &link_name, &error)) {
-        g_debug ("Couldn't find mux_id in network link: %s", error->message);
-        g_clear_error (&error);
-
-        /* Now, assume this is because the mux_id attribute was added in a newer
-         * kernel. As a fallback, we'll try to detect the first new link listed,
-         * even if this is definitely very racy. */
-        link_name = lookup_first_new_link (ctx->links_before, links_after);
-        if (link_name)
-            g_debug ("Found first new link '%s' (unknown mux id)", link_name);
-    } else
-        g_debug ("Found link '%s' associated to mux id '%s'", link_name, ctx->mux_id);
-
-    if (link_name) {
-        if (!track_mux_id (self, link_name, ctx->mux_id, &error)) {
-            g_warning ("Couldn't track mux id: %s", error->message);
-            g_clear_error (&error);
-        }
-        g_task_return_pointer (task, g_steal_pointer (&link_name), g_free);
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    g_debug ("Link not yet found, rescheduling...");
-    ctx->timeout_ms_elapsed += LINK_OPERATION_TIMEOUT_STEP_MS;
-    if (ctx->timeout_ms_elapsed > ctx->timeout_ms) {
-        g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_TIMEOUT,
-                                 "No new link detected for mux id %s", ctx->mux_id);
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    /* retry */
-    return G_SOURCE_CONTINUE;
-}
-
-static void
-run_add_link (GTask *task)
-{
-    QmiNetPortManagerQmiwwan *self;
-    LinkOperationContext     *ctx;
-    GError                   *error = NULL;
-
+    g_debug ("Net port manager based on qmi_wwan ignores the ifname prefix '%s'", ifname_prefix);
     g_debug ("Running add link operation...");
 
-    self = g_task_get_source_object (task);
-    ctx = g_task_get_task_data (task);
+    task = g_task_new (self, cancellable, callback, user_data);
 
     if (!qmi_helpers_list_links (self->priv->sysfs_file,
-                                 g_task_get_cancellable (task),
+                                 cancellable,
                                  NULL,
-                                 &ctx->links_before,
+                                 &links_before,
                                  &error)) {
-        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory: ");
+        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory before link addition: ");
         g_task_return_error (task, error);
         g_object_unref (task);
         return;
     }
 
-    if (!ctx->mux_id) {
-        guint new_mux_id;
-
-        new_mux_id = get_first_free_mux_id (self, ctx->links_before, &error);
-        if (new_mux_id == QMI_DEVICE_MUX_ID_UNBOUND) {
-            g_prefix_error (&error, "Couldn't add create link with automatic mux id: ");
+    if (mux_id == QMI_DEVICE_MUX_ID_AUTOMATIC) {
+        mux_id = get_first_free_mux_id (self, links_before, &error);
+        if (mux_id == QMI_DEVICE_MUX_ID_UNBOUND) {
+            g_prefix_error (&error, "Couldn't add link with automatic mux id: ");
             g_task_return_error (task, error);
             g_object_unref (task);
             return;
         }
-        g_debug ("Using mux id %u", new_mux_id);
-        ctx->mux_id = g_strdup_printf ("0x%02x", new_mux_id);
+        g_debug ("Using mux id %u", mux_id);
     }
 
-    if (!qmi_helpers_write_sysfs_file (self->priv->add_mux_sysfs_path, ctx->mux_id, &error)) {
-        g_prefix_error (&error, "Couldn't add create link with mux id %s: ", ctx->mux_id);
+    g_task_set_task_data (task, GUINT_TO_POINTER (mux_id), NULL);
+    mux_id_str = g_strdup_printf ("0x%02x", mux_id);
+
+    if (!qmi_helpers_write_sysfs_file (self->priv->add_mux_sysfs_path, mux_id_str, &error)) {
+        g_prefix_error (&error, "Couldn't add create link with mux id %s: ", mux_id_str);
         g_task_return_error (task, error);
         g_object_unref (task);
         return;
     }
 
-    ctx->timeout_source = g_timeout_source_new (LINK_OPERATION_TIMEOUT_STEP_MS);
-    g_source_set_callback (ctx->timeout_source,
-                           (GSourceFunc) add_link_check_timeout,
-                           task,
-                           NULL);
-    g_source_attach (ctx->timeout_source, g_main_context_get_thread_default ());
-}
-
-static void
-net_port_manager_add_link (QmiNetPortManager   *_self,
-                           guint                mux_id,
-                           const gchar         *base_ifname,
-                           const gchar         *ifname_prefix,
-                           guint                timeout,
-                           GCancellable        *cancellable,
-                           GAsyncReadyCallback  callback,
-                           gpointer             user_data)
-{
-    QmiNetPortManagerQmiwwan *self = QMI_NET_PORT_MANAGER_QMIWWAN (_self);
-    GTask                    *task;
-    LinkOperationContext     *ctx;
-
-    g_debug ("Net port manager based on qmi_wwan ignores the ifname prefix '%s'", ifname_prefix);
-
-    task = link_operation_new (QMI_NET_PORT_MANAGER_QMIWWAN (self),
-                               base_ifname,
-                               timeout,
-                               cancellable,
-                               callback,
-                               user_data);
-    if (!task)
+    if (!qmi_helpers_list_links (self->priv->sysfs_file,
+                                 cancellable,
+                                 links_before,
+                                 &links_after,
+                                 &error)) {
+        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory after link addition: ");
+        g_task_return_error (task, error);
+        g_object_unref (task);
         return;
+    }
 
-    ctx = g_task_get_task_data (task);
+    if (!lookup_mux_id_in_links (links_after, mux_id_str, &link_name, NULL)) {
+        /* Now, assume this is because the mux_id attribute was added in a newer
+         * kernel. As a fallback, we'll try to detect the first new link listed,
+         * even if this is definitely very racy. */
+        link_name = lookup_first_new_link (links_before, links_after);
+        if (!link_name) {
+            g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_FAILED,
+                                     "No new link detected for mux id %s", mux_id_str);
+            g_object_unref (task);
+            return;
+        }
+        g_debug ("Found first new link '%s' (unknown mux id)", link_name);
+    } else
+        g_debug ("Found link '%s' associated to mux id '%s'", link_name, mux_id_str);
 
-    if (mux_id != QMI_DEVICE_MUX_ID_AUTOMATIC)
-        ctx->mux_id = g_strdup_printf ("0x%02x", mux_id);
 
-    /* If there is another task running, queue the new one */
-    if (self->priv->running) {
-        g_debug ("Queueing add link operation...");
-        self->priv->pending_tasks = g_list_append (self->priv->pending_tasks, task);
-        return;
+    if (!track_mux_id (self, link_name, mux_id_str, &error)) {
+        g_warning ("Couldn't track mux id: %s", error->message);
+        g_clear_error (&error);
     }
-    self->priv->running = TRUE;
 
-    run_add_link (task);
+    g_task_return_pointer (task, g_steal_pointer (&link_name), g_free);
+    g_object_unref (task);
 }
 
 /*****************************************************************************/
@@ -534,160 +355,95 @@ net_port_manager_del_link_finish (QmiNetPortManager  *self,
     return g_task_propagate_boolean (G_TASK (res), error);
 }
 
-static gboolean
-del_link_check_timeout (GTask *task)
-{
-    QmiNetPortManagerQmiwwan *self;
-    LinkOperationContext     *ctx;
-    GError                   *error = NULL;
-    g_autoptr(GPtrArray)      links_after = NULL;
-
-    if (g_task_return_error_if_cancelled (task)) {
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    self = g_task_get_source_object (task);
-    ctx  = g_task_get_task_data (task);
-
-    if (!qmi_helpers_list_links (self->priv->sysfs_file,
-                                 g_task_get_cancellable (task),
-                                 ctx->links_before,
-                                 &links_after,
-                                 &error)) {
-        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory: ");
-        g_task_return_error (task, error);
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    if (!links_after || !g_ptr_array_find_with_equal_func (links_after, ctx->link_iface, g_str_equal, NULL)) {
-        if (!untrack_mux_id (self, ctx->link_iface, &error)) {
-            g_debug ("couldn't untrack mux id: %s", error->message);
-            g_clear_error (&error);
-        }
-        g_task_return_boolean (task, TRUE);
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    ctx->timeout_ms_elapsed += LINK_OPERATION_TIMEOUT_STEP_MS;
-    if (ctx->timeout_ms_elapsed > ctx->timeout_ms) {
-        g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_TIMEOUT,
-                                 "link '%s' still detected", ctx->link_iface);
-        g_object_unref (task);
-        return G_SOURCE_REMOVE;
-    }
-
-    /* retry */
-    return G_SOURCE_CONTINUE;
-}
-
 static void
-run_del_link (GTask *task)
+net_port_manager_del_link (QmiNetPortManager   *_self,
+                           const gchar         *ifname,
+                           guint                mux_id,
+                           guint                timeout,
+                           GCancellable        *cancellable,
+                           GAsyncReadyCallback  callback,
+                           gpointer             user_data)
 {
-    QmiNetPortManagerQmiwwan *self;
-    LinkOperationContext     *ctx;
+    QmiNetPortManagerQmiwwan *self = QMI_NET_PORT_MANAGER_QMIWWAN (_self);
+    GTask                    *task;
     GError                   *error = NULL;
+    g_autoptr(GPtrArray)      links_before = NULL;
+    g_autoptr(GPtrArray)      links_after = NULL;
+    g_autofree gchar         *mux_id_str = NULL;
 
-    self = g_task_get_source_object (task);
-    ctx = g_task_get_task_data (task);
+    g_debug ("Running del link (%s) operation...", ifname);
 
-    g_debug ("Running del link operation...");
+    task = g_task_new (self, cancellable, callback, user_data);
 
     if (!qmi_helpers_list_links (self->priv->sysfs_file,
-                                 g_task_get_cancellable (task),
+                                 cancellable,
                                  NULL,
-                                 &ctx->links_before,
+                                 &links_before,
                                  &error)) {
-        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory: ");
+        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory before link deletion: ");
         g_task_return_error (task, error);
         g_object_unref (task);
         return;
     }
 
-    if (!ctx->links_before || !g_ptr_array_find_with_equal_func (ctx->links_before, ctx->link_iface, g_str_equal, NULL)) {
+    if (!links_before || !g_ptr_array_find_with_equal_func (links_before, ifname, g_str_equal, NULL)) {
         g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_INVALID_ARGS,
                                  "Cannot delete link '%s': interface not found",
-                                 ctx->link_iface);
+                                 ifname);
         g_object_unref (task);
         return;
     }
 
     /* Try to guess mux id if not given as input */
-    if (!ctx->mux_id) {
-        ctx->mux_id = read_link_mux_id (ctx->link_iface, &error);
-        if (!ctx->mux_id) {
-            const gchar *mux_id;
-
-            g_debug ("Couldn't read mux id from sysfs: %s", error->message);
-            g_clear_error (&error);
-
-            mux_id = get_tracked_mux_id (self, ctx->link_iface, &error);
-            if (!mux_id) {
-                g_debug ("Couldn't get tracked mux id: %s", error->message);
-                g_clear_error (&error);
-
+    if (mux_id != QMI_DEVICE_MUX_ID_UNBOUND)
+        mux_id_str = g_strdup_printf ("0x%02x", mux_id);
+    else {
+        mux_id_str = read_link_mux_id (ifname, NULL);
+        if (!mux_id_str) {
+            mux_id_str = g_strdup (get_tracked_mux_id (self, ifname, NULL));
+            if (!mux_id_str) {
                 /* This unsupported error allows us to flag when del_all_links()
                  * needs to switch to the fallback mechanism */
                 g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_UNSUPPORTED,
                                          "Cannot delete link '%s': unknown mux id",
-                                         ctx->link_iface);
+                                         ifname);
                 g_object_unref (task);
                 return;
             }
         }
     }
 
-    if (!qmi_helpers_write_sysfs_file (self->priv->del_mux_sysfs_path, ctx->mux_id, &error)) {
-        g_prefix_error (&error, "Couldn't delete link with mux id %s: ", ctx->mux_id);
+    if (!qmi_helpers_write_sysfs_file (self->priv->del_mux_sysfs_path, mux_id_str, &error)) {
+        g_prefix_error (&error, "Couldn't delete link with mux id %s: ", mux_id_str);
         g_task_return_error (task, error);
         g_object_unref (task);
         return;
     }
 
-    ctx->timeout_source = g_timeout_source_new (LINK_OPERATION_TIMEOUT_STEP_MS);
-    g_source_set_callback (ctx->timeout_source,
-                           (GSourceFunc) del_link_check_timeout,
-                           task,
-                           NULL);
-    g_source_attach (ctx->timeout_source, g_main_context_get_thread_default ());
-}
+    if (!qmi_helpers_list_links (self->priv->sysfs_file,
+                                 cancellable,
+                                 links_before,
+                                 &links_after,
+                                 &error)) {
+        g_prefix_error (&error, "Couldn't enumerate files in the sysfs directory after link deletion: ");
+        g_task_return_error (task, error);
+        g_object_unref (task);
+        return;
+    }
 
-static void
-net_port_manager_del_link (QmiNetPortManager   *_self,
-                           const gchar         *ifname,
-                           guint                mux_id,
-                           guint                timeout,
-                           GCancellable        *cancellable,
-                           GAsyncReadyCallback  callback,
-                           gpointer             user_data)
-{
-    QmiNetPortManagerQmiwwan *self = QMI_NET_PORT_MANAGER_QMIWWAN (_self);
-    GTask                    *task;
-    LinkOperationContext     *ctx;
-
-    task = link_operation_new (QMI_NET_PORT_MANAGER_QMIWWAN (self),
-                               NULL, /* no base ifname, it's a delete operation */
-                               timeout,
-                               cancellable,
-                               callback,
-                               user_data);
-    g_assert (task);
-
-    ctx = g_task_get_task_data (task);
-    ctx->link_iface = g_strdup (ifname);
-    ctx->mux_id = (mux_id != QMI_DEVICE_MUX_ID_UNBOUND) ? g_strdup_printf ("0x%02x", mux_id) : NULL;
-
-    /* If there is another task running, queue the new one */
-    if (self->priv->running) {
-        g_debug ("Queueing del link operation...");
-        self->priv->pending_tasks = g_list_append (self->priv->pending_tasks, task);
+    if (links_after && g_ptr_array_find_with_equal_func (links_after, ifname, g_str_equal, NULL)) {
+        g_task_return_new_error (task, QMI_CORE_ERROR, QMI_CORE_ERROR_FAILED,
+                                 "link '%s' still detected", ifname);
+        g_object_unref (task);
         return;
     }
-    self->priv->running = TRUE;
 
-    run_del_link (task);
+    if (!untrack_mux_id (self, ifname, &error)) {
+        g_debug ("couldn't untrack mux id: %s", error->message);
+        g_clear_error (&error);
+    }
+    g_task_return_boolean (task, TRUE);
+    g_object_unref (task);
 }
 
 /*****************************************************************************/