Attachment 77806 Details for Bug 63326 – Version 3 of the proposed change

[patch] Version 3 of the proposed change

telepathy-haze-password-request-V3.mbox (text/plain), 19.57 KB, created by Stefan Becker on 2013-04-11 12:51:12 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Stefan Becker

Created: 2013-04-11 12:51:12 UTC

Size: 19.57 KB

patch

obsolete

>From 2766e6cb8113fe778a256fb110ef4222b75b02ea Mon Sep 17 00:00:00 2001
>From: Stefan Becker <chemobejk@gmail.com>
>Date: Tue, 9 Apr 2013 19:30:34 +0300
>Subject: [PATCH 1/3] request: handle purple_account_request_password()
>
>This is needed for libpurple plugins with optional password, e.g. SIPE
>since 1.14.1. That libpurple API call boils down to a
>purple_request_fields() call.
>
>The flagging for --enable-leaky-request-stubs was refactored so that
>this new code is always compiled in.
>---
> src/connection.c | 45 +++++++++++++++++++++++++++++++++-----
> src/connection.h |  3 +++
> src/main.c       |  2 --
> src/request.c    | 66 ++++++++++++++++++++++++++++++++++++++++++++++++++------
> src/request.h    |  3 +++
> 5 files changed, 105 insertions(+), 14 deletions(-)
>
>diff --git a/src/connection.c b/src/connection.c
>index 3ec4c8e..c005207 100644
>--- a/src/connection.c
>+++ b/src/connection.c
>@@ -42,6 +42,7 @@
> #include "connection-avatars.h"
> #include "connection-mail.h"
> #include "extensions/extensions.h"
>+#include "request.h"
> 
> #include "connection-capabilities.h"
> 
>@@ -135,6 +136,9 @@ struct _HazeConnectionPrivate
>     gchar *prpl_id;
>     PurplePluginProtocolInfo *prpl_info;
> 
>+    /* Set if purple_account_request_password() was called */
>+    gpointer password_request;
>+
>     /* Set if purple_account_disconnect has been called or is scheduled to be
>      * called, so should not be called again.
>      */
>@@ -443,6 +447,11 @@ _haze_connection_password_manager_prompt_cb (GObject *source,
>     {
>       DEBUG ("Simple password manager failed: %s", error->message);
> 
>+      if (priv->password_request)
>+        {
>+          haze_request_password_cb(priv->password_request, NULL);
>+        }
>+
>       if (base_conn->status != TP_CONNECTION_STATUS_DISCONNECTED)
>         {
>           tp_base_connection_disconnect_with_dbus_error (base_conn,
>@@ -451,7 +460,13 @@ _haze_connection_password_manager_prompt_cb (GObject *source,
>         }
> 
>       /* no need to call purple_account_disconnect because _connect
>-       * was never called */
>+       * was never called ...
>+       * ... unless we had a dynamic password request */
>+      if (priv->password_request)
>+        {
>+          priv->disconnecting = TRUE;
>+          purple_account_disconnect (self->account);
>+        }
> 
>       g_error_free (error);
>       return;
>@@ -460,11 +475,17 @@ _haze_connection_password_manager_prompt_cb (GObject *source,
>   g_free (priv->password);
>   priv->password = g_strdup (password->str);
> 
>-  purple_account_set_password (self->account, priv->password);
>+  if (priv->password_request)
>+    {
>+      haze_request_password_cb (priv->password_request, priv->password);
>+    } else
>+    {
>+      purple_account_set_password (self->account, priv->password);
> 
>-  purple_account_set_enabled(self->account, UI_ID, TRUE);
>-  purple_account_connect (self->account);
>-  priv->connect_called = TRUE;
>+      purple_account_set_enabled(self->account, UI_ID, TRUE);
>+      purple_account_connect (self->account);
>+      priv->connect_called = TRUE;
>+    }
> }
> 
> static gboolean
>@@ -513,6 +534,20 @@ _haze_connection_start_connecting (TpBaseConnection *base,
>     return TRUE;
> }
> 
>+void haze_connection_request_password (PurpleAccount *account,
>+                                       void *user_data)
>+{
>+    HazeConnection *self = ACCOUNT_GET_HAZE_CONNECTION (account);
>+    HazeConnectionPrivate *priv = self->priv;
>+
>+    priv->password_request = user_data;
>+
>+    /* pop up auth channel */
>+    tp_simple_password_manager_prompt_async (self->password_manager,
>+                                             _haze_connection_password_manager_prompt_cb,
>+                                             self);
>+}
>+
> static void
> _haze_connection_shut_down (TpBaseConnection *base)
> {
>diff --git a/src/connection.h b/src/connection.h
>index a75732a..ae587ab 100644
>--- a/src/connection.h
>+++ b/src/connection.h
>@@ -109,6 +109,9 @@ const gchar *haze_get_fallback_group (void);
> const gchar **haze_connection_get_implemented_interfaces (void);
> const gchar **haze_connection_get_guaranteed_interfaces (void);
> 
>+void haze_connection_request_password (PurpleAccount *account,
>+                                       gpointer user_data);
>+
> G_END_DECLS
> 
> #endif /* #ifndef __HAZE_CONNECTION_H__*/
>diff --git a/src/main.c b/src/main.c
>index afa0a02..4cd171c 100644
>--- a/src/main.c
>+++ b/src/main.c
>@@ -141,9 +141,7 @@ haze_ui_init (void)
>     purple_accounts_set_ui_ops (haze_get_account_ui_ops ());
>     purple_conversations_set_ui_ops (haze_get_conv_ui_ops ());
>     purple_connections_set_ui_ops (haze_get_connection_ui_ops ());
>-#ifdef ENABLE_LEAKY_REQUEST_STUBS
>     purple_request_set_ui_ops (haze_request_get_ui_ops ());
>-#endif
>     purple_notify_set_ui_ops (haze_notify_get_ui_ops ());
>     purple_privacy_set_ui_ops (haze_get_privacy_ui_ops ());
> }
>diff --git a/src/request.c b/src/request.c
>index 408678b..dae859d 100644
>--- a/src/request.c
>+++ b/src/request.c
>@@ -18,6 +18,8 @@
>  *
>  */
> 
>+#include "config.h"
>+
> #include <glib-object.h>
> 
> #include <libpurple/account.h>
>@@ -25,7 +27,9 @@
> 
> #include "debug.h"
> #include "request.h"
>+#include "connection.h"
> 
>+#ifdef ENABLE_LEAKY_REQUEST_STUBS
> static gpointer
> haze_request_input (const char *title,
>                     const char *primary,
>@@ -95,6 +99,30 @@ haze_request_action (const char *title,
> 
>     return NULL;
> }
>+#endif
>+
>+struct password_data {
>+    PurpleRequestFields *fields;
>+    PurpleRequestField *password;
>+    GCallback ok_cb;
>+    GCallback cancel_cb;
>+    void *user_data;
>+};
>+
>+void haze_request_password_cb (gpointer user_data,
>+                               const gchar *password)
>+{
>+    struct password_data *pd = user_data;
>+
>+    if (password) {
>+        purple_request_field_string_set_value(pd->password, password);
>+        ((PurpleRequestFieldsCb)pd->ok_cb)(pd->user_data, pd->fields);
>+    } else {
>+        ((PurpleRequestFieldsCb)pd->cancel_cb)(pd->user_data, pd->fields);
>+    }
>+
>+    g_free(pd);
>+}
> 
> static gpointer
> haze_request_fields (const char *title,
>@@ -110,14 +138,36 @@ haze_request_fields (const char *title,
>                      PurpleConversation *conv,
>                      void *user_data)
> {
>-    DEBUG ("ignoring request:");
>-    DEBUG ("    title: %s", (title ? title : "(null)"));
>-    DEBUG ("    primary: %s", (primary ? primary : "(null)"));
>-    DEBUG ("    secondary: %s", (secondary ? secondary : "(null)"));
>+    /*
>+     * We must support purple_account_request_password() which boils down
>+     * to purple_request_fields() with certain parameters. I'm not sure
>+     * if this the best way of doing this, but it works.
>+     */
>+    if (purple_request_fields_exists(fields, "password") &&
>+        purple_request_fields_exists(fields, "remember")) {
>+        struct password_data *pd = g_new0(struct password_data, 1);
>+
>+        DEBUG ("triggering password request");
>+
>+        pd->fields    = fields;
>+        pd->password  = purple_request_fields_get_field(fields, "password");
>+        pd->ok_cb     = ok_cb;
>+        pd->cancel_cb = cancel_cb;
>+        pd->user_data = user_data;
>+
>+        haze_connection_request_password(account, pd);
>+
>+    } else {
>+        DEBUG ("ignoring request:");
>+        DEBUG ("    title: %s", (title ? title : "(null)"));
>+        DEBUG ("    primary: %s", (primary ? primary : "(null)"));
>+        DEBUG ("    secondary: %s", (secondary ? secondary : "(null)"));
>+    }
> 
>     return NULL;
> }
> 
>+#ifdef ENABLE_LEAKY_REQUEST_STUBS
> static gpointer
> haze_request_file (const char *title,
>                    const char *filename,
>@@ -152,7 +202,7 @@ haze_request_folder (const char *title,
> 
>     return NULL;
> }
>-
>+#endif
> 
> /*
> 	void (*close_request)(PurpleRequestType type, void *ui_handle);
>@@ -160,12 +210,14 @@ haze_request_folder (const char *title,
> 
> static PurpleRequestUiOps request_uiops =
> {
>+#ifdef ENABLE_LEAKY_REQUEST_STUBS
>     .request_input = haze_request_input,
>     .request_choice = haze_request_choice,
>     .request_action = haze_request_action,
>-    .request_fields = haze_request_fields,
>     .request_file = haze_request_file,
>-    .request_folder = haze_request_folder
>+    .request_folder = haze_request_folder,
>+#endif
>+    .request_fields = haze_request_fields
> };
> 
> PurpleRequestUiOps *
>diff --git a/src/request.h b/src/request.h
>index d896cc6..cc572d6 100644
>--- a/src/request.h
>+++ b/src/request.h
>@@ -20,4 +20,7 @@
> 
> #include <libpurple/request.h>
> 
>+void haze_request_password_cb (gpointer user_data,
>+                               const gchar *password);
>+
> PurpleRequestUiOps *haze_request_get_ui_ops (void);
>-- 
>1.8.1.4
>
>
>From bd32b0af473c7e98ca81b8826fbab9c930b0ad9a Mon Sep 17 00:00:00 2001
>From: Stefan Becker <chemobejk@gmail.com>
>Date: Wed, 10 Apr 2013 16:13:04 +0300
>Subject: [PATCH 2/3] request: fix resource leakage
>
>It is the responsibility of the UI code to free the "fields" parameter
>after haze_request_fields() has been called. This has to be done with
>purple_request_close() and handled in the close_request() UI operation.
>---
> src/request.c | 65 +++++++++++++++++++++++++++++++++++++----------------------
> 1 file changed, 41 insertions(+), 24 deletions(-)
>
>diff --git a/src/request.c b/src/request.c
>index dae859d..db54317 100644
>--- a/src/request.c
>+++ b/src/request.c
>@@ -101,7 +101,7 @@ haze_request_action (const char *title,
> }
> #endif
> 
>-struct password_data {
>+struct fields_data {
>     PurpleRequestFields *fields;
>     PurpleRequestField *password;
>     GCallback ok_cb;
>@@ -109,21 +109,40 @@ struct password_data {
>     void *user_data;
> };
> 
>+static void haze_close_request(PurpleRequestType type, void *ui_handle)
>+{
>+    struct fields_data *fd = ui_handle;
>+
>+    purple_request_fields_destroy(fd->fields);
>+    g_free(fd);
>+}
>+
> void haze_request_password_cb (gpointer user_data,
>                                const gchar *password)
> {
>-    struct password_data *pd = user_data;
>+    struct fields_data *fd = user_data;
> 
>     if (password) {
>-        purple_request_field_string_set_value(pd->password, password);
>-        ((PurpleRequestFieldsCb)pd->ok_cb)(pd->user_data, pd->fields);
>+        purple_request_field_string_set_value(fd->password, password);
>+        ((PurpleRequestFieldsCb)fd->ok_cb)(fd->user_data, fd->fields);
>     } else {
>-        ((PurpleRequestFieldsCb)pd->cancel_cb)(pd->user_data, pd->fields);
>+        ((PurpleRequestFieldsCb)fd->cancel_cb)(fd->user_data, fd->fields);
>     }
> 
>-    g_free(pd);
>+    purple_request_close(PURPLE_REQUEST_FIELDS, fd);
> }
> 
>+static gboolean haze_request_fields_destroy(gpointer user_data)
>+{
>+    purple_request_close(PURPLE_REQUEST_FIELDS, user_data);
>+    return FALSE;
>+}
>+
>+/*
>+ * We must support purple_account_request_password() which boils down
>+ * to purple_request_fields() with certain parameters. I'm not sure
>+ * if this the best way of doing this, but it works.
>+ */
> static gpointer
> haze_request_fields (const char *title,
>                      const char *primary,
>@@ -138,33 +157,34 @@ haze_request_fields (const char *title,
>                      PurpleConversation *conv,
>                      void *user_data)
> {
>-    /*
>-     * We must support purple_account_request_password() which boils down
>-     * to purple_request_fields() with certain parameters. I'm not sure
>-     * if this the best way of doing this, but it works.
>-     */
>+    struct fields_data *fd = g_new0(struct fields_data, 1);
>+
>+    /* it is our responsibility to destroy this data */
>+    fd->fields = fields;
>+
>     if (purple_request_fields_exists(fields, "password") &&
>         purple_request_fields_exists(fields, "remember")) {
>-        struct password_data *pd = g_new0(struct password_data, 1);
> 
>         DEBUG ("triggering password request");
> 
>-        pd->fields    = fields;
>-        pd->password  = purple_request_fields_get_field(fields, "password");
>-        pd->ok_cb     = ok_cb;
>-        pd->cancel_cb = cancel_cb;
>-        pd->user_data = user_data;
>+        fd->password  = purple_request_fields_get_field(fields, "password");
>+        fd->ok_cb     = ok_cb;
>+        fd->cancel_cb = cancel_cb;
>+        fd->user_data = user_data;
> 
>-        haze_connection_request_password(account, pd);
>+        haze_connection_request_password(account, fd);
> 
>     } else {
>         DEBUG ("ignoring request:");
>         DEBUG ("    title: %s", (title ? title : "(null)"));
>         DEBUG ("    primary: %s", (primary ? primary : "(null)"));
>         DEBUG ("    secondary: %s", (secondary ? secondary : "(null)"));
>+
>+	/* Avoid leaking of "fields" */
>+	g_idle_add(haze_request_fields_destroy, fd);
>     }
> 
>-    return NULL;
>+    return fd;
> }
> 
> #ifdef ENABLE_LEAKY_REQUEST_STUBS
>@@ -204,10 +224,6 @@ haze_request_folder (const char *title,
> }
> #endif
> 
>-/*
>-	void (*close_request)(PurpleRequestType type, void *ui_handle);
>-*/
>-
> static PurpleRequestUiOps request_uiops =
> {
> #ifdef ENABLE_LEAKY_REQUEST_STUBS
>@@ -217,7 +233,8 @@ static PurpleRequestUiOps request_uiops =
>     .request_file = haze_request_file,
>     .request_folder = haze_request_folder,
> #endif
>-    .request_fields = haze_request_fields
>+    .request_fields = haze_request_fields,
>+    .close_request  = haze_close_request
> };
> 
> PurpleRequestUiOps *
>-- 
>1.8.1.4
>
>
>From 3cb78610afdd638cb2ebb9cf907d9acad6f05a8d Mon Sep 17 00:00:00 2001
>From: Stefan Becker <chemobejk@gmail.com>
>Date: Thu, 11 Apr 2013 15:45:34 +0300
>Subject: [PATCH 3/3] request: fix more resource leaks
>
>Triggered by Will's review comments.
>
>- check callback pointers to be non-NULL before calling them
>- call cancel_cb also in destroy idle callback
>- reset connection's password request field on closing
>- coding style cleanup
>---
> src/connection.c | 19 ++++++++++---
> src/connection.h |  1 +
> src/request.c    | 87 +++++++++++++++++++++++++++++++++++++-------------------
> 3 files changed, 73 insertions(+), 34 deletions(-)
>
>diff --git a/src/connection.c b/src/connection.c
>index c005207..10450c2 100644
>--- a/src/connection.c
>+++ b/src/connection.c
>@@ -449,7 +449,7 @@ _haze_connection_password_manager_prompt_cb (GObject *source,
> 
>       if (priv->password_request)
>         {
>-          haze_request_password_cb(priv->password_request, NULL);
>+          haze_request_password_cb (priv->password_request, NULL);
>         }
> 
>       if (base_conn->status != TP_CONNECTION_STATUS_DISCONNECTED)
>@@ -478,7 +478,8 @@ _haze_connection_password_manager_prompt_cb (GObject *source,
>   if (priv->password_request)
>     {
>       haze_request_password_cb (priv->password_request, priv->password);
>-    } else
>+    }
>+  else
>     {
>       purple_account_set_password (self->account, priv->password);
> 
>@@ -534,8 +535,9 @@ _haze_connection_start_connecting (TpBaseConnection *base,
>     return TRUE;
> }
> 
>-void haze_connection_request_password (PurpleAccount *account,
>-                                       void *user_data)
>+void
>+haze_connection_request_password (PurpleAccount *account,
>+                                  void *user_data)
> {
>     HazeConnection *self = ACCOUNT_GET_HAZE_CONNECTION (account);
>     HazeConnectionPrivate *priv = self->priv;
>@@ -548,6 +550,15 @@ void haze_connection_request_password (PurpleAccount *account,
>                                              self);
> }
> 
>+void
>+haze_connection_cancel_password_request (PurpleAccount *account)
>+{
>+    HazeConnection *self = ACCOUNT_GET_HAZE_CONNECTION (account);
>+    HazeConnectionPrivate *priv = self->priv;
>+
>+    priv->password_request = NULL;
>+}
>+
> static void
> _haze_connection_shut_down (TpBaseConnection *base)
> {
>diff --git a/src/connection.h b/src/connection.h
>index ae587ab..0637f6c 100644
>--- a/src/connection.h
>+++ b/src/connection.h
>@@ -111,6 +111,7 @@ const gchar **haze_connection_get_guaranteed_interfaces (void);
> 
> void haze_connection_request_password (PurpleAccount *account,
>                                        gpointer user_data);
>+void haze_connection_cancel_password_request (PurpleAccount *account);
> 
> G_END_DECLS
> 
>diff --git a/src/request.c b/src/request.c
>index db54317..8aa7bc9 100644
>--- a/src/request.c
>+++ b/src/request.c
>@@ -102,39 +102,62 @@ haze_request_action (const char *title,
> #endif
> 
> struct fields_data {
>+    PurpleAccount *account;
>     PurpleRequestFields *fields;
>     PurpleRequestField *password;
>-    GCallback ok_cb;
>-    GCallback cancel_cb;
>+    PurpleRequestFieldsCb ok_cb;
>+    PurpleRequestFieldsCb cancel_cb;
>     void *user_data;
> };
> 
>-static void haze_close_request(PurpleRequestType type, void *ui_handle)
>+static void
>+haze_close_request (PurpleRequestType type,
>+                    void *ui_handle)
> {
>     struct fields_data *fd = ui_handle;
> 
>-    purple_request_fields_destroy(fd->fields);
>-    g_free(fd);
>+    haze_connection_cancel_password_request (fd->account);
>+    purple_request_fields_destroy (fd->fields);
>+    g_slice_free (struct fields_data, fd);
> }
> 
>-void haze_request_password_cb (gpointer user_data,
>-                               const gchar *password)
>+void
>+haze_request_password_cb (gpointer user_data,
>+                          const gchar *password)
> {
>     struct fields_data *fd = user_data;
> 
>-    if (password) {
>-        purple_request_field_string_set_value(fd->password, password);
>-        ((PurpleRequestFieldsCb)fd->ok_cb)(fd->user_data, fd->fields);
>-    } else {
>-        ((PurpleRequestFieldsCb)fd->cancel_cb)(fd->user_data, fd->fields);
>-    }
>-
>-    purple_request_close(PURPLE_REQUEST_FIELDS, fd);
>+    if (password)
>+      {
>+        purple_request_field_string_set_value (fd->password, password);
>+        if (fd->ok_cb)
>+          {
>+            (fd->ok_cb) (fd->user_data, fd->fields);
>+          }
>+      }
>+    else
>+      {
>+        if (fd->cancel_cb)
>+          {
>+            (fd->cancel_cb) (fd->user_data, fd->fields);
>+          }
>+      }
>+
>+    purple_request_close (PURPLE_REQUEST_FIELDS, fd);
> }
> 
>-static gboolean haze_request_fields_destroy(gpointer user_data)
>+static gboolean
>+haze_request_fields_destroy (gpointer user_data)
> {
>-    purple_request_close(PURPLE_REQUEST_FIELDS, user_data);
>+    struct fields_data *fd = user_data;
>+
>+    if (fd->cancel_cb)
>+      {
>+        (fd->cancel_cb) (fd->user_data, fd->fields);
>+      }
>+
>+    purple_request_close (PURPLE_REQUEST_FIELDS, user_data);
>+
>     return FALSE;
> }
> 
>@@ -157,32 +180,36 @@ haze_request_fields (const char *title,
>                      PurpleConversation *conv,
>                      void *user_data)
> {
>-    struct fields_data *fd = g_new0(struct fields_data, 1);
>+    struct fields_data *fd = g_slice_new0 (struct fields_data);
> 
>     /* it is our responsibility to destroy this data */
>-    fd->fields = fields;
>+    fd->account   = account;
>+    fd->fields    = fields;
>+    fd->cancel_cb = (PurpleRequestFieldsCb) cancel_cb;
>+    fd->user_data = user_data;
> 
>-    if (purple_request_fields_exists(fields, "password") &&
>-        purple_request_fields_exists(fields, "remember")) {
>+    if (purple_request_fields_exists (fields, "password") &&
>+        purple_request_fields_exists (fields, "remember"))
>+      {
> 
>         DEBUG ("triggering password request");
> 
>-        fd->password  = purple_request_fields_get_field(fields, "password");
>-        fd->ok_cb     = ok_cb;
>-        fd->cancel_cb = cancel_cb;
>-        fd->user_data = user_data;
>+        fd->password = purple_request_fields_get_field (fields, "password");
>+        fd->ok_cb    = (PurpleRequestFieldsCb) ok_cb;
> 
>-        haze_connection_request_password(account, fd);
>+        haze_connection_request_password (account, fd);
> 
>-    } else {
>+      }
>+    else
>+      {
>         DEBUG ("ignoring request:");
>         DEBUG ("    title: %s", (title ? title : "(null)"));
>         DEBUG ("    primary: %s", (primary ? primary : "(null)"));
>         DEBUG ("    secondary: %s", (secondary ? secondary : "(null)"));
> 
>-	/* Avoid leaking of "fields" */
>-	g_idle_add(haze_request_fields_destroy, fd);
>-    }
>+        /* Avoid leaking of "fields" and "user_data" */
>+        g_idle_add (haze_request_fields_destroy, fd);
>+      }
> 
>     return fd;
> }
>-- 
>1.8.1.4
>

Actions: View | Splinter Review

Attachments on bug 63326: 77679 | 77742 | 77806

Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.