On 06/01/2012 05:50 PM, Jakub Filak wrote:
Signed-off-by: Jakub Filak<jfilak(a)redhat.com>
---
configure.ac | 1 +
libreport.spec.in | 7 -
src/gtk-helpers/Makefile.am | 6 +-
src/gtk-helpers/abrt-keyring.c | 130 ----
src/gtk-helpers/event_config_dialog.c | 71 +--
src/gtk-helpers/internal_libreport_gtk.h | 15 +-
src/gtk-helpers/secrets.c | 1050 ++++++++++++++++++++++++++++++
src/gui-wizard-gtk/main.c | 2 +-
8 files changed, 1072 insertions(+), 210 deletions(-)
delete mode 100644 src/gtk-helpers/abrt-keyring.c
create mode 100644 src/gtk-helpers/secrets.c
Let's jump over internals and take a look at users of this new API:
+void load_event_config_data_from_user_storage(GHashTable
*event_config_list)
+{
+ secrets_api_init();
+
+ if (is_secrets_api_available())
+ {
+ GError *error = NULL;
+ struct secrets_object *collection = get_default_collection(&error);
+
+ if (collection)
+ {
+ g_hash_table_foreach(event_config_list,&load_event_config, collection);
+ secrets_object_delete(collection);
+ }
+
+ if (error)
+ secrets_api_error(error, "D-Bus secrets API cannot provide default
collection");
+ }
+ else
+ {
+ VERB1 log("cannot load user's configuration due to unavailability of
D-Bus secrets API");
+ }
+}
I'd prefer to see something like:
void load_event_config_data_from_user_storage(GHashTable *event_config_list)
{
struct secrets_object *collection = get_default_secrets_collection();
if (collection)
{
g_hash_table_foreach(event_config_list, &load_event_config, collection);
secrets_object_delete(collection);
}
/* else: get_default_secrets_collection() already emitted error message */
}
That's it.
No secrets_api_init(),
no &error thingy,
no log("Vague error message of limited use").
I looked through the code and back-propagating of &error
looks counter-productive in most (all?) cases.
I suspect that logging error message on the spot and returning
NULL or bool error indicator is simpler, and gives better error messages.
+++ b/src/gtk-helpers/secrets.c
@@ -0,0 +1,1050 @@
+/*
+ Copyright (C) 2012 ABRT Team
+ Copyright (C) 2012 RedHat inc.
+
+ This program is free software; you can redistribute it and/or modify
+ it under the terms of the GNU General Public License as published by
+ the Free Software Foundation; either version 2 of the License, or
+ (at your option) any later version.
+
+ This program is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+ GNU General Public License for more details.
+
+ You should have received a copy of the GNU General Public License along
+ with this program; if not, write to the Free Software Foundation, Inc.,
+ 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA.
+*/
+#include "internal_libreport.h"
+#include "internal_libreport_gtk.h"
+
+#include <gio/gio.h>
+
+#define secrets_api "org.freedesktop.secrets"
+#define SECRETS_NAME_SPACE(interface) "org.freedesktop.Secret."interface
+
+/* label and alias have to be same because of ksecrets */
+#define SECRETS_COLLECTION_LABEL SECRETS_COLLECTION_ALIAS
+#define SECRETS_COLLECTION_ALIAS "default"
+
+#define SECRETS_SEARCH_ATTRIBUTE "libreportEventConfig"
+
+/* 5s timeout*/
+#define SECRETS_CALL_DEFAULT_TIMEOUT 5000
+
+/* Well known errors for which we have workarounds */
+#define NOT_IMPLEMENTED_READ_ALLIAS_ERROR_MESSAGE
"GDBus.Error:org.freedesktop.DBus.Error.UnknownMethod: No such method
'ReadAlias' in interface 'org.freedesktop.Secret.Service' at object path
'/org/freedesktop/secrets' (signature 's')"
+#define INVALID_PROPERTIES_ARGUMENTS
"GDBus.Error:org.freedesktop.DBus.Error.InvalidArgs: Invalid properties
argument"
I think a comment with short explanation of the Secret Service API
we are using in this file would be in order.
What dbus calls we use and for what?
How are passwords stored by Secret Service?
What is "collection"?
What is this locking/unlocking stuff we are doing?
etc...
I don't aks for screenfuls of text, but just give the general overview...
+enum secrets_api_state
+{
+ /* intial state */
+ SBS_INITIAL = 0,
+ /* after opening a secrets service session (different than D-Bus session) */
+ SBS_CONNECTED,
+ /* secrets service is not available do not bother user anymore */
+ SBS_UNAVAILABLE,
+};
+
+struct secrets_object
+{
+ GDBusProxy *proxy;
+ /*
http://dbus.freedesktop.org/doc/dbus-specification.html#standard-interfac... */
+ GDBusProxy *property_proxy;
+ const gchar *interface_name;
+};
+
+/* Glib DBus object org.freedesktop.secrets */
+GDBusConnection *g_connection;
+
+/* State of the service */
+enum secrets_api_state g_state;
The variable should be static.
+/*
http://standards.freedesktop.org/secret-service/re01.html */
+/* xorg.freedesktop.Secret.Service interface object */
+struct secrets_object *g_service;
This one too. Etc...
+/*
http://standards.freedesktop.org/secret-service/ch06.html */
+/* xorg.freedesktop.Secret.Session interface object */
+GDBusProxy *g_session;
+
+/*******************************************************************************/
+/* struct secrets_object */
+/*******************************************************************************/
+
+static struct secrets_object *secrets_object_new_from_proxy(GDBusProxy *proxy)
+{
+ struct secrets_object *obj = xmalloc(sizeof(*obj));
+ obj->proxy = proxy;
+ obj->property_proxy = NULL;
I use xzalloc in such cases: I don't need to initialize members to zero/NULL
with it.
+ obj->interface_name =
g_dbus_proxy_get_interface_name(proxy);
+
+ return obj;
+}
+
+static struct secrets_object *secrets_object_new(const gchar *path,
+ const gchar *interface,
+ GError **error)
+{
+ GDBusProxy *const proxy =
+ g_dbus_proxy_new_sync(g_connection, G_DBUS_PROXY_FLAGS_NONE, NULL,
+ secrets_api, path, interface, NULL, error);
+
+ if (*error)
+ return NULL;
+
+ return secrets_object_new_from_proxy(proxy);
+}
+
+static void secrets_object_change_path(struct secrets_object *obj,
+ const gchar *path,
+ GError **error)
+{
+ GDBusProxy *const proxy =
+ g_dbus_proxy_new_sync(g_connection, G_DBUS_PROXY_FLAGS_NONE, NULL,
+ secrets_api, path, obj->interface_name, NULL, error);
+
+ if (!*error)
+ {
+ if (obj->proxy)
+ g_object_unref(obj->proxy);
+
+ obj->proxy = proxy;
+ obj->interface_name = g_dbus_proxy_get_interface_name(proxy);
+ }
+}
+
+static void secrets_object_delete(struct secrets_object *obj)
+{
+ if (obj->proxy)
+ g_object_unref(obj->proxy);
+
+ if (obj->property_proxy)
+ g_object_unref(obj->property_proxy);
+
+ free(obj);
In C, free(NULL) is safe. In C++, "delete ptr" where ptr is NULL is safe.
It's useful to not confuse people and follow this convention
that freeing/deleting NULL pointers is safe.
Basically, maybe add "if (!obj) return;" at the beginning...
+/*******************************************************************************/
+/* struct secrets_api */
+/*******************************************************************************/
+
+static void secrets_api_set_unavailable();
+
+static void secrets_api_error(GError* error, const char *comment)
+{
+ if (comment)
+ {
+ log("%s : %s\n", comment, error->message);
+ }
+ else
+ {
+ log("%s\n", error->message);
log() function doesn't need trailing "\n".
(BTW, why do you have a space _before_ the colon in the messages?)
+ }
+
+ g_error_free(error);
+ secrets_api_set_unavailable();
+}
+
+/*
+ * Initialize all global variables
+ */
+static enum secrets_api_state secrets_api_connect()
+{
+ GError *error = NULL;
+
+ g_connection = g_bus_get_sync(G_BUS_TYPE_SESSION, NULL,&error);
+
+ if (g_connection == NULL)
+ {
+ secrets_api_error(error, "failed to open connection to D-Bus
session");
+ return SBS_UNAVAILABLE;
+ }
+
+ g_service = secrets_object_new("/org/freedesktop/secrets",
SECRETS_NAME_SPACE("Service"),&error);
+ if (error)
+ {
+ secrets_api_error(error, "failed to create service");
Imagine this error message.
<progname>: failed to create service : <text from g_dbus_proxy_new_sync()
error>
What "service" program failed to create? If I would be an admin and see
this error message from e.g. abrt-gui, I wouldn't understand anything.
<progname>: Can't connect over DBus to name '%s' path
'/org/freedesktop/secrets' interface
'SECRETS_NAME_SPACE("Service")': <text from g_dbus_proxy_new_sync()
error>
would at least tell user what happened. But you don't know the name here,
because it is encapsulated in secrets_object_new() function and while you
propagated error out of that function, the other bits (such as the name!)
are not, so you can't emit a good error message.
I think it's better to emit error message right after we detect
g_dbus_proxy_new_sync() failure, not in the several layers back
upo in the call stack.
+ return SBS_UNAVAILABLE;
+ }
+
+ GVariant *const alg_in = g_variant_new_string("");
+ GVariant *const args = g_variant_new("(sv)", "plain", alg_in);
+ GVariant *const ret =
+ g_dbus_proxy_call_sync(g_service->proxy, "OpenSession", args,
G_DBUS_PROXY_FLAGS_NONE,
+ SECRETS_CALL_DEFAULT_TIMEOUT, NULL,&error);
+
+ g_variant_unref(alg_in);
+
+ if (error)
+ {
+ secrets_api_error(error, "cannot open session Secrets session");
Maybe "Error in OpenSession() call: <...>"?
Maybe a wrapper around g_dbus_proxy_new_sync() will be of use here,
so that you don't need to repeat error handling in every call site...
+ return SBS_UNAVAILABLE;
+ }
...
...
...
+static int is_secrets_api_available()
+{
+ return g_state != SBS_UNAVAILABLE;
Maybe "return g_state == SBS_CONNECTED" instead?
+static void prompt_call_back(GDBusConnection *connection, const
gchar *sender_name,
+ const gchar *object_path, const gchar *interface_name,
+ const gchar *signal_name, GVariant *parameters,
+ gpointer user_data)
+{
+ struct prompt_call_back_args *const args = (struct prompt_call_back_args
*)user_data;
+ GVariant *const result = NULL;
+
+ g_variant_get(parameters, "(bv)",&args->dismissed,&result);
You are passing address of a constant ptr (&result thing).
g_variant_get expects a pointer to non-constant ptr,
it will (try to) modify it.
+ if (!args->dismissed)
+ args->result = result;
+ else
+ g_variant_unref(result);
gcc can assume that result is NULL, because in its definition
you lied to gcc that it is a constant.
+ prompt_quit_loop(args->loop);
+}
+static struct secrets_object *secrets_service_read_allias(const char
*alias, GError **error)
s/allias/alias/
+static void load_settings(struct secrets_object *collection, const
char *event_name, event_config_t *ec)
+{
+ /* Locked property is not supported by ksecrets */
+ /* thus we have to perform Unlock on each call of load settings */
+ /* because a collection can be locked by the service at anytime */
+ GError *error = NULL;
+ const bool dismissed = secrets_service_unlock_object(collection,&error);
+
+ if (!error&& !dismissed)
+ {
+ struct secrets_object *item = find_item_by_event_name(collection,
event_name,&error);
+
+ if (!error&& item)
+ {
+ VERB2 log("loading event config : '%s'", event_name);
+ load_event_options_from_item(ec->options, item);
+ secrets_object_delete(item);
+ }
+ }
+
+ if (dismissed)
+ secrets_api_set_unavailable();
+
+ if (error)
+ secrets_api_error(error, NULL);
+}
+
+static void load_event_config(gpointer key, gpointer value, gpointer user_data)
+{
+ char *const event_name = (char*)key;
+ event_config_t *const ec = (event_config_t *)value;
+ struct secrets_object *collection = (struct secrets_object *)user_data;
+
+ if (is_secrets_api_available())
+ load_settings(collection, event_name, ec);
+}
Looks like you leak collection?
+static void save_options(struct secrets_object *collection,
+ const char* event_name,
+ GList *options,
+ bool store_passwords,
+ GError **error)
+{
+ struct secrets_object *item;
+ item = find_item_by_event_name(collection, event_name, error);
+
+ if (*error)
+ return;
+
+ GVariant *const attributes = create_attributes_from_options(options,
+ event_name,
+ store_passwords);
+
+ if (item)
+ { /* item exists */
+ VERB2 log("updating event config : '%s'", event_name);
+ secrets_object_set_dbus_property(item, "Attributes", attributes,
error);
+ secrets_object_delete(item);
+ }
+ else /* create a new item */
+ {
+ VERB2 log("creating event config : '%s'", event_name);
+ secrets_collection_create_text_item(collection, event_name, attributes, error);
+ }
+}
+
+static void save_event_config(const char *event_name,
+ GList *options,
+ bool store_passwords)
+{
+ bool dismissed = false;
+ GError *error = NULL;
+ struct secrets_object *collection = get_default_collection(&error);
+
+ if (error)
+ goto save_event_config_end;
+
+ if (!collection)
+ {
+ VERB2 log("going to create a new default collection
'"SECRETS_COLLECTION_LABEL"'"
+ " with alias
'"SECRETS_COLLECTION_ALIAS"'");
+
+ collection =
+ secrets_service_create_collection(SECRETS_COLLECTION_LABEL,
SECRETS_COLLECTION_ALIAS,
+&dismissed,&error);
+
+ if (error || dismissed)
+ goto save_event_config_end;
+ }
+
+ save_options(collection, event_name, options, store_passwords,&error);
+
+save_event_config_end:
+ if (dismissed)
+ secrets_api_set_unavailable();
+
+ if (error)
+ secrets_api_error(error, NULL);
+}
Looks like you leak collection?
--
vda