Created attachment 187687 [details] [review]
Reviewable implementation

Implemented as part of GcrGnupgCollection rather than a filter. This patch is a squashed version of the branch here:

http://cgit.collabora.co.uk/git/user/stefw/gnome-keyring.git/log/?h=gnupg-secret

Review of attachment 187687 [details] [review]:

Looks fine overall to me, though I haven't tested it. Some minor comments about documentation and code comments.

Would it give any performance improvement to spawn the public and secret processes in parallel and have them concurrently update the hash table (with the appropriate locking, of course)? If so, it might be an idea to refactor the code to do that instead.

::: gcr/gcr-gnupg-collection.c
@@ +41,3 @@
 	PROP_MAIN_CONTEXT
 };
 

Might be worthwhile adding a comment explaining the way that GcrGnupgCollection operates in two phases, why it's necessary, how/when it transitions between the two phases and how/when the final results are available.

@@ +45,3 @@
+	PHASE_PUBLIC_KEYS = 1,
+	PHASE_SECRET_KEYS = 2,
+};

I think it would make the code a little clearer to add a typedef here, so that you can use that type instead of a gint in the code and make things a little bit more type safe.

@@ +49,2 @@
 struct _GcrGnupgCollectionPrivate {
+	GHashTable *items;          /* char *keyid -> GcrGnupgKey* */

Why change this comment?

@@ +199,3 @@
 typedef struct {
 	GcrGnupgCollection *collection;       /* reffed pointer back to collection */
+	gint loading_phase;

See above comment about adding a typedef for the enum.

@@ +268,3 @@
+	/* Don't have this key */
+	if (key == NULL)
+		g_message ("secret key seen but no public key for: %s", keyid);

Should this not be a g_debug() message instead? If it remains as a g_message() it should probably start with a capital letter to make it pretty.

::: gcr/gcr-gnupg-key.c
@@ +182,1 @@
 	                             G_TYPE_PTR_ARRAY, G_PARAM_READWRITE));

I must've missed it in the initial review for this code, but these properties should probably have gtk-doc comments.

@@ +202,3 @@
  * _gcr_gnupg_key_new:
+ * @pubset: array of GcrColons* representing public part of key
+ * @secset: optional array of GcrColons* representing secret part of key.

I don't know what conventions you're using in the gnome-keyring code, but it might be an idea to add gobject-introspection annotations like (allow-none) to this parameter so that it's clear that it's nullable. Even if you're not currently using g-i, it'll make adding support for it later easier, and gtk-doc also picks up on the annotations and presents them in the documentation.

@@ +205,3 @@
  *
+ * Create a new GcrGnupgKey for the colons data passed. If the secret part
+ * of the key is set, then this represents a secret key.

How about “then this represents a secret key; otherwise it represents a public key.” to make it a little clearer?

@@ +247,3 @@
 	GObject *obj;
+	const gchar *old_keyid;
+	const gchar *new_keyid;

These could be declared inside the if block below.

@@ +260,3 @@
+			g_warning ("it is an error to change a gnupg key so that the "
+			           "fingerprint is no longer the same: %s != %s",
+			           old_keyid, new_keyid);

If it's an error, you might want to use g_error() instead.

@@ +275,3 @@
 	g_object_notify (obj, "label");
 	g_object_notify (obj, "markup");
+	g_object_thaw_notify (obj);

What happened to the g_object_notify(obj,"keyid")? Splinter's showing it as having been removed, but cgit says it hasn't been. Probably a bug in Splinter.

@@ +284,3 @@
+ * Get the colons secret data this key is based on. %NULL if a public key.
+ *
+ * Returns: An array of GcrColons*, owned by the key.

(allow-none) annotation?

@@ +296,3 @@
+ * _gcr_gnupg_key_set_secret_dataset:
+ * @self: The key
+ * @dataset: The new array of GcrColons*

(allow-none) annotation?

@@ +305,3 @@
+	GObject *obj;
+	const gchar *pub_keyid;
+	const gchar *sec_keyid;

Again, these declarations could be moved inside the if block.

@@ +317,3 @@
+			g_warning ("it is an error to create a gnupg key so that the "
+			           "fingerprint of thet pub and sec parts are not the same: %s != %s",
+			           pub_keyid, sec_keyid);

Again, g_error() might be more appropriate.

::: gcr/tests/test-gnupg-key.c
@@ +56,3 @@
 	test->dataset = dataset;
+
+	test->key = _gcr_gnupg_key_new (dataset, NULL);

Might be an idea to add extra sets which operate on a key which has both a public dataset and a secret dataset set.

Thanks!

(In reply to comment #2)
> Would it give any performance improvement to spawn the public and secret
> processes in parallel and have them concurrently update the hash table (with
> the appropriate locking, of course)? If so, it might be an idea to refactor the
> code to do that instead.

It's hard to know offhand whether that would be a big performance increase, but it does make things more complicated. We need to have a public key before we load the secret part. 

What would be ideal is to be able to provide the --list-keys and --list-secret-keys options to a single gpg process, but it doesn't allow that.

This is an interesting idea to keep in mind for later, but for now I think we should just keep in simple.

> ::: gcr/gcr-gnupg-collection.c
> @@ +41,3 @@
>      PROP_MAIN_CONTEXT
>  };

Removed.

> Might be worthwhile adding a comment explaining the way that GcrGnupgCollection
> operates in two phases, why it's necessary, how/when it transitions between the
> two phases and how/when the final results are available.

Good point. done.

> @@ +45,3 @@
> +    PHASE_PUBLIC_KEYS = 1,
> +    PHASE_SECRET_KEYS = 2,
> +};
> 
> I think it would make the code a little clearer to add a typedef here, so that
> you can use that type instead of a gint in the code and make things a little
> bit more type safe.

Done.

> @@ +268,3 @@
> +    /* Don't have this key */
> +    if (key == NULL)
> +        g_message ("secret key seen but no public key for: %s", keyid);
> 
> Should this not be a g_debug() message instead? If it remains as a g_message()
> it should probably start with a capital letter to make it pretty.

In gnome-keyring we generally have the policy to print a message on invalid input. But I've capitalized it like you said.

> ::: gcr/gcr-gnupg-key.c
> @@ +182,1 @@
>                                   G_TYPE_PTR_ARRAY, G_PARAM_READWRITE));
> 
> I must've missed it in the initial review for this code, but these properties
> should probably have gtk-doc comments.

Done.

> @@ +202,3 @@
>   * _gcr_gnupg_key_new:
> + * @pubset: array of GcrColons* representing public part of key
> + * @secset: optional array of GcrColons* representing secret part of key.
> 
> I don't know what conventions you're using in the gnome-keyring code, but it
> might be an idea to add gobject-introspection annotations like (allow-none) to
> this parameter so that it's clear that it's nullable. Even if you're not
> currently using g-i, it'll make adding support for it later easier, and gtk-doc
> also picks up on the annotations and presents them in the documentation.

Sounds good. I gave a shot at adding introspection, fixed some gobject-introspection bugs, but then gave up for now as it was getting too complicated. However added annotations to these new classes.

> @@ +260,3 @@
> +            g_warning ("it is an error to change a gnupg key so that the "
> +                       "fingerprint is no longer the same: %s != %s",
> +                       old_keyid, new_keyid);
> 
> If it's an error, you might want to use g_error() instead.

This is a precondition (although a complicated one), so like other preconditions, g_warning seems more appropriate here.

> What happened to the g_object_notify(obj,"keyid")? Splinter's showing it as
> having been removed, but cgit says it hasn't been. Probably a bug in Splinter.

Removed in a second commit. The keyid should never change. That's what the preconditions above are about.

> ::: gcr/tests/test-gnupg-key.c
> @@ +56,3 @@
>      test->dataset = dataset;
> +
> +    test->key = _gcr_gnupg_key_new (dataset, NULL);
> 
> Might be an idea to add extra sets which operate on a key which has both a
> public dataset and a secret dataset set.

Added several more tests, for both the keys and the collection. Added several more debug log lines.

Created attachment 187837 [details] [review]
Updated after pwithnall's review

Review of attachment 187837 [details] [review]:

Apart from these two comments, I can't see anything wrong with the updated branch.

::: gcr/gcr-gnupg-key.c
@@ +326,3 @@
+ * Get the colons secret data this key is based on. %NULL if a public key.
+ *
+ * Returns: (transfer none): An array of GcrColons*.

This can also be (allow-none).

::: gcr/tests/test-gnupg-collection.c
@@ +84,3 @@
+	gchar *path;
+
+	directory = g_get_current_dir ();

Is this going to be robust against running the tests from a different directory?

(In reply to comment #5)
> + * Returns: (transfer none): An array of GcrColons*.
> This can also be (allow-none).

Done

> +    directory = g_get_current_dir ();
> 
> Is this going to be robust against running the tests from a different
> directory?

Only if SRCDIR environment variable is set (see main()). I wish there was a cleaner way to run tests from a different directory, but haven't figured it out yet.

Merged into master.

(In reply to comment #6)
> Merged into master.

Should be bug be marked FIXED, or is there more still to do?

Whoops yes. Fixed.