GNOME Bugzilla – Bug 704890
Fix up the smartcard plugin
Last modified: 2013-07-27 15:59:39 UTC
This patchset brings back a rewritten smartcard plugin. It now user the ObjectManager interface and uses modern datatypes like GTask.
Created attachment 250138 [details] [review] common: add utilities to help work with GDBus This commit adds two functions to help work with GDBus: 1) gsd_dbus_register_error_domain: It looks up an error enum from a passed in GType, and registers all the associated error codes with GDBus, so they can easily be sent over the bus. 2) gsd_dbus_escape_object_path: It takes a NUL terminated string and escapes characters that are illegal characters for an object path. Slashes don't get touched. Those two functions are prerequisite work before landing a new smartcard plugin.
Created attachment 250139 [details] [review] configure: turn back on smartcard support
Created attachment 250140 [details] [review] smartcard: drop old implementation It's going to be substantially rewritten, so drop the old code to avoid a confusing diff when the new stuff lands.
Created attachment 250141 [details] [review] smartcard: add back smartcard support This commit lands a rewrite of the smartcard plugin. It makes use of the DBus ObjectManager interface to export tokens over the bus, as they're inserted and removed.
Review of attachment 250138 [details] [review]: That looks fine, but as we're not using an error domain for gsd errors (we usually reuse the ones from the failing call), this isn't too useful in common/ Could you please move it to smartcard/ ? Looks fine after that.
Review of attachment 250139 [details] [review]: Could you do that as a revert of the original commit instead? e2998b46a9d5eaa9ec2812867efac1eff8b3937f for reference
Review of attachment 250140 [details] [review]: Fine, though that means that this commit will break compilation.
Review of attachment 250141 [details] [review]: ::: plugins/smartcard/gsd-smartcard-manager.c @@ +151,3 @@ + + if (self->priv->nss_is_loaded) { + NSS_Shutdown (); and reset ->nss_is_loaded. @@ +254,3 @@ + * removal + */ + if (old_slot_series == slot_series) { remove braces for one line conditionals. @@ +317,3 @@ + operation = g_new0 (WatchSmartcardsOperation, 1); + operation->driver = SECMOD_ReferenceModule (driver); + operation->smartcards = g_hash_table_new_full (NULL, Please add the function names here, even if g_hash_table_new_full() falls back. @@ +320,3 @@ + NULL, + NULL, + (GDestroyNotify) one line. @@ +327,3 @@ + g_task_set_task_data (task, + operation, + (GDestroyNotify) one line. @@ +334,3 @@ + task); + g_object_weak_ref (G_OBJECT (task), + (GWeakNotify) one line. @@ +452,3 @@ + g_task_attach_source (task, + source, + (GSourceFunc) one line. @@ +474,3 @@ + g_task_set_task_data (task, + operation, + (GDestroyNotify) one line. @@ +499,3 @@ + driver, + cancellable, + (GAsyncReadyCallback) one line. @@ +505,3 @@ + driver, + cancellable, + (GAsyncReadyCallback) one line. @@ +585,3 @@ + driver_list = SECMOD_GetDefaultModuleList (); + for (node = driver_list; node != NULL; node = node->next) { + if (!SECMOD_HasRemovableSlots (node->module) || split up the conditional, it's easier to read. @@ +593,3 @@ + node->module, + cancellable, + (GAsyncReadyCallback) one line. @@ +642,3 @@ + activate_all_drivers_async (self, + cancellable, + (GAsyncReadyCallback) one line. @@ +712,3 @@ + watch_smartcards_async (self, + priv->cancellable, + (GAsyncReadyCallback) one line. @@ +732,3 @@ + gsd_smartcard_service_new_async (self, + priv->cancellable, + (GAsyncReadyCallback) one line. @@ +786,3 @@ + + if (g_strcmp0 (g_getenv ("PKCS11_LOGIN_TOKEN_NAME"), token_name) == 0) + return card_slot; indentation. @@ +832,3 @@ + card_slot = (PK11SlotInfo *) value; + + if (PK11_IsPresent (card_slot)) braces for 2 line conditional. @@ +849,3 @@ + G_LOCK (gsd_smartcards_watch_tasks); + node = priv->smartcards_watch_tasks; + while (node != NULL) { Please use a for loop. @@ +882,3 @@ + g_return_if_fail (self->priv != NULL); + + if (priv->start_idle_id != 0) Call stop() here. ::: plugins/smartcard/gsd-smartcard-service.c @@ +127,3 @@ + } + + g_debug ("taking name %s on session bus", same line. @@ +131,3 @@ + + self = g_task_get_source_object (task); + priv = self->priv; you don't use priv afterwards. @@ +177,3 @@ + g_bus_get (G_BUS_TYPE_SESSION, + g_task_get_cancellable (task), + (GAsyncReadyCallback) We have wide screens, put this on the same line. @@ +202,3 @@ + slot_id = PK11_GetSlotID (card_slot); + + escaped_library_path = gsd_dbus_escape_object_path (driver->dllName); leaking escaped_library_path? @@ +255,3 @@ + object_paths = g_ptr_array_new (); + node = inserted_tokens; + while (node != NULL) { Please use a for loop instead. @@ +269,3 @@ + gsd_smartcard_service_manager_complete_get_inserted_tokens (manager, + invocation, + (const char * const *) same line. @@ +308,3 @@ + g_clear_object (&self->priv->cancellable); + g_clear_pointer (&self->priv->tokens, + (GDestroyNotify) No need to cast. @@ +317,3 @@ +gsd_smartcard_service_finalize (GObject *object) +{ + G_OBJECT_CLASS (gsd_smartcard_service_parent_class)->finalize (object); Remove finalize, it's unused. @@ +355,3 @@ + g_value_set_object (value, priv->bus_connection); + break; + remove the extra linefeed. @@ +369,3 @@ + + object_class->dispose = gsd_smartcard_service_dispose; + object_class->finalize = gsd_smartcard_service_finalize; Finalize just chains up. @@ +406,3 @@ + } + + g_assert (object != NULL); No need to assert here, you just checked for it. @@ +428,3 @@ + G_PRIORITY_DEFAULT, + cancellable, + (GAsyncReadyCallback) single line. @@ +447,3 @@ + + if (self == NULL) + goto out; just return self; @@ +554,3 @@ +{ + g_clear_pointer (&operation->main_thread_source, + (GDestroyNotify) one line. @@ +642,3 @@ + g_task_set_task_data (task, + operation, + (GDestroyNotify) one line. @@ +645,3 @@ + destroy_register_new_token_operation); + + create_main_thread_source ((GSourceFunc) one line. @@ +756,3 @@ + g_task_set_task_data (task, + operation, + (GDestroyNotify) one line. @@ +759,3 @@ + destroy_synchronize_token_operation); + + create_main_thread_source ((GSourceFunc) one line,. @@ +778,3 @@ + g_debug ("Couldn't synchronize token: %s", + error->message); + goto out; This isn't needed. ::: plugins/smartcard/org.gnome.Smartcard.xml @@ +24,3 @@ +--> + +<node name="/" xmlns:doc="http://www.freedesktop.org/dbus/1.0/doc.dtd"> Can you please indent this file? it's not very readable... @@ +30,3 @@ + An interface used for managing smartcard functionality. + --> + <interface name="org.gnome.Smartcard.Manager"> Why is the interface on org.gnome.* not on org.gnome.SettingsDaemon.*?
*** Bug 690167 has been marked as a duplicate of this bug. ***
(In reply to comment #6) > Review of attachment 250139 [details] [review]: > > Could you do that as a revert of the original commit instead? > e2998b46a9d5eaa9ec2812867efac1eff8b3937f for reference sure. (In reply to comment #7) > Review of attachment 250140 [details] [review]: > > Fine, though that means that this commit will break compilation. I'll push the configure.ac revert last to keep things building at every stage. (In reply to comment #8) > Review of attachment 250141 [details] [review]: [will do all requested style changes and fixes] > ::: plugins/smartcard/org.gnome.Smartcard.xml > @@ +24,3 @@ > +--> > + > +<node name="/" xmlns:doc="http://www.freedesktop.org/dbus/1.0/doc.dtd"> > Can you please indent this file? it's not very readable... Sure. > Why is the interface on org.gnome.* not on org.gnome.SettingsDaemon.*? No good reason. I'll change it.
Created attachment 250269 [details] [review] smartcard: drop old implementation It's going to be substantially rewritten, so drop the old code to avoid a confusing diff when the new stuff lands.
Created attachment 250270 [details] [review] smartcard: add back smartcard support This commit lands a rewrite of the smartcard plugin. It makes use of the DBus ObjectManager interface to export tokens over the bus, as they're inserted and removed.
Created attachment 250271 [details] [review] Revert "build: Disable smartcard plugin for now" This reverts commit e2998b46a9d5eaa9ec2812867efac1eff8b3937f. We're going to be revamping the smartcard plugin, so as a first step, revert the commit that disables it.
Review of attachment 250269 [details] [review]: Looks good.
Review of attachment 250270 [details] [review]: Yep.
Review of attachment 250271 [details] [review]: You'll need to change the commit message slightly, as it's the last thing you're doing now ;)
Attachment 250269 [details] pushed as 28a23b8 - smartcard: drop old implementation Attachment 250270 [details] pushed as 5832062 - smartcard: add back smartcard support Attachment 250271 [details] pushed as 12b3784 - Revert "build: Disable smartcard plugin for now" with updated commit message