Created attachment 187948 [details] [review]
Implementation of GcrGnupgProcess

Branch is here: http://cgit.collabora.co.uk/git/user/stefw/gnome-keyring.git/log/?h=gnupg-process

Review of attachment 187948 [details] [review]:

I couldn't find any major problems, and the code looks fairly solid to me, though I didn't run it.

::: gcr/gcr-gnupg-collection.c
@@ +243,3 @@
+		g_object_unref (load->process);
+	if (load->output_sig)
+		g_source_remove (load->output_sig);

Should this be g_signal_handler_disconnect(), since output_sig is a signal handler ID?

@@ +456,3 @@
 spawn_gnupg_list_process (GcrGnupgCollectionLoad *load, GSimpleAsyncResult *res)
 {
+	GcrGnupgProcessFlags flags = 0;

Might be clearer to use a GCR_GNUPG_PROCESS_NONE enum value here.

::: gcr/gcr-gnupg-process.c
@@ +185,3 @@
+	g_object_class_install_property (gobject_class, PROP_EXECUTABLE,
+	           g_param_spec_string ("executable", "Executable", "Gnupg Executable",
+	                                GPG_EXECUTABLE, G_PARAM_READWRITE | G_PARAM_CONSTRUCT_ONLY));

These properties could do with gtk-doc comments.

@@ +205,3 @@
+	           G_SIGNAL_RUN_LAST, G_STRUCT_OFFSET (GcrGnupgProcessClass, attribute_data),
+	           NULL, NULL, _gcr_marshal_VOID__BOXED,
+	           G_TYPE_NONE, 1, G_TYPE_BYTE_ARRAY);

…as could these signals.

@@ +244,3 @@
+ * argument is used.
+ *
+ * Returns: (transfer full) A newly allocated process.

Missing a colon after the g-i annotation (e.g. “Returns: (transfer full): A newly…”).

@@ +395,3 @@
+
+	if (gnupg_source->cancel_sig) {
+		g_source_remove (gnupg_source->cancel_sig);

Shouldn't this be g_cancellable_disconnect()?

@@ +416,3 @@
+	gssize result;
+
+	g_return_val_if_fail (fd >= 0, -1);

FALSE instead of -1?

@@ +469,3 @@
+	GPollFD *poll;
+
+	/* Standard input, no suport yet */

s/suport/support/

@@ +520,3 @@
+				g_warning ("couldn't read output data from gnupg process");
+			} else if (buffer->len > 0) {
+				_gcr_debug ("received %d bytes of attribute data", (gint)buffer->len);

s/attribute data/output data/?

@@ +587,3 @@
+		if (!g_error_matches (self->pv->error, G_IO_ERROR, G_IO_ERROR_CANCELLED))
+			error = g_error_new (G_SPAWN_ERROR, G_SPAWN_ERROR_FAILED,
+			                     "Gnupg process was terminated with signal: %d", code);

These two error messages might want to be translatable.

@@ +597,3 @@
+	/* Already have an error, just print out message */
+	} else if (error) {
+		g_message ("%s", error->message);

Perhaps this should be a g_warning()?

@@ +643,3 @@
+	if (gnupg_source->process->pv->error == NULL)
+		gnupg_source->process->pv->error = g_error_new_literal (G_IO_ERROR, G_IO_ERROR_CANCELLED,
+		                                                        "The operation was cancelled");

This error message should be translatable, since it could theoretically reach the user.

@@ +652,3 @@
+ * @self: The process
+ * @argv: The arguments for the process, not including executable
+ * @envp: (allow-none): The environment for new process.

Might want to document that these are both NULL-terminated. There's also the “(array zero-terminated=1)” g-i annotation.

@@ +656,3 @@
+ * @cancellable: (allow-none): Cancellation object
+ * @callback: Will be called when operation completes.
+ *

The documentation for user_data's missing. It should be annotated with “(closure)”.

@@ +666,3 @@
+ * will be status and attribute output respectively. The
+ * GcrGnupgProcess:status_record and GcrGnupgProcess:attribute_data signals
+ * will provide this data.

It might be helpful to document which thread the signals are expected to be emitted in (e.g. the main thread or the one which is running the gpg instance?).

@@ +688,3 @@
+	g_return_if_fail (GCR_IS_GNUPG_PROCESS (self));
+	g_return_if_fail (argv);
+	g_return_if_fail (callback);

I think you could also add g_return_if_fail (cancellable == NULL || G_IS_CANCELLABLE (cancellable)) here.

@@ +838,3 @@
+ * @error: Location to raise an error on failure.
+ *
+ * Get the result of running a gnupg process.

The “Return value” documentation comment is missing.

::: gcr/gcr-gnupg-process.h
@@ +59,3 @@
+
+	gboolean (*attribute_data) (GcrGnupgProcess *self, GByteArray *output);
+};

I think gtk-doc will probably demand a documentation comment for _GcrGnupgProcess and _GcrGnupgProcessClass, though adding them isn't too important.

@@ +65,3 @@
+	GCR_GNUPG_PROCESS_WITH_STATUS       = 1 << 1,
+	GCR_GNUPG_PROCESS_WITH_ATTRIBUTES   = 1 << 2
+} GcrGnupgProcessFlags;

This should have a gtk-doc comment.

@@ +67,3 @@
+} GcrGnupgProcessFlags;
+
+GType               _gcr_gnupg_process_get_type                (void);

G_GNUC_CONST.

::: gcr/tests/files/gnupg-mock/mock-status-and-attribute
@@ +3,3 @@
+# This script is used with test-gnupg-process
+# Needs to be run with /bin/bash in order to handle two digit
+# file descripter redirects

s/descripter/descriptor/

::: gcr/tests/test-gnupg-process.c
@@ +56,3 @@
+		g_object_unref (test->result);
+	if (test->process)
+		g_object_unref (test->process);

I think it would be a little cleaner to move the unrefs for ->result and ->process into the test_*() functions so that the ref counting is a little easier to follow.

@@ +172,3 @@
+	g_assert (test->result);
+	_gcr_gnupg_process_run_finish (test->process, test->result, &error);
+	g_assert_no_error (error);

It might be cleaner to move the above three lines into on_async_ready(), eliminating them from all the test_*() functions which are not supposed to error, and also mirroring what actual client code would do (call _gcr_gnupg_process_run_finish() in the callback from _gcr_gnupg_process_run_async()).

For the test_*() functions which are supposed to error, you could either expose the GError in the Test struct, or have a variant of on_async_ready() for each of them which tests for the required error. This would make the code just as icky though. :-\

@@ +410,3 @@
+	_gcr_gnupg_process_run_async (test->process, argv, NULL, 0, cancellable, on_async_ready, test);
+	g_cancellable_cancel (cancellable);
+	egg_test_wait_until (500);

It might be an idea to add another cancellation test which calls g_cancellable_cancel() a little while after starting off the async process, so that you're not just testing the cancellation code on startup. Perhaps call g_cancellable_cancel() in one of the signal handlers for the process? Might be more reliable than waiting a few milliseconds.

Thanks! I'm back from the !wonderful world of procrastination. Made most of those changes and merged. Some comments:

(In reply to comment #2)
> Should this be g_signal_handler_disconnect(), since output_sig is a signal
> handler ID?

Good catch, changed througout.

> @@ +395,3 @@
> +
> +    if (gnupg_source->cancel_sig) {
> +        g_source_remove (gnupg_source->cancel_sig);
> 
> Shouldn't this be g_cancellable_disconnect()?

g_cancellable_disconnect is scary and can deadlock. Can't use it here because this function could be called from within the cancelled signal hanlder.

> @@ +666,3 @@
> + * will be status and attribute output respectively. The
> + * GcrGnupgProcess:status_record and GcrGnupgProcess:attribute_data signals
> + * will provide this data.
> 
> It might be helpful to document which thread the signals are expected to be
> emitted in (e.g. the main thread or the one which is running the gpg
> instance?).

No threads involved. This is nice async code, run from the mainloop.

> @@ +172,3 @@
> +    g_assert (test->result);
> +    _gcr_gnupg_process_run_finish (test->process, test->result, &error);
> +    g_assert_no_error (error);
> 
> It might be cleaner to move the above three lines into on_async_ready(),
> eliminating them from all the test_*() functions which are not supposed to
> error, and also mirroring what actual client code would do (call
> _gcr_gnupg_process_run_finish() in the callback from
> _gcr_gnupg_process_run_async()).
> 
> For the test_*() functions which are supposed to error, you could either expose
> the GError in the Test struct, or have a variant of on_async_ready() for each
> of them which tests for the required error. This would make the code just as
> icky though. :-\

Yeah, so that's why I didn't end up doing this part.

> @@ +410,3 @@
> +    _gcr_gnupg_process_run_async (test->process, argv, NULL, 0, cancellable,
> on_async_ready, test);
> +    g_cancellable_cancel (cancellable);
> +    egg_test_wait_until (500);
> 
> It might be an idea to add another cancellation test which calls
> g_cancellable_cancel() a little while after starting off the async process, so
> that you're not just testing the cancellation code on startup. Perhaps call
> g_cancellable_cancel() in one of the signal handlers for the process? Might be
> more reliable than waiting a few milliseconds.

Good plan, done.