[Bug 30430] Make wocky_implement_finish_* macros safer

[bugzilla-daemon at freedesktop.org]

https://bugs.freedesktop.org/show_bug.cgi?id=30430

--- Comment #4 from Simon McVittie <simon.mcvittie at collabora.co.uk> 2010-09-28 10:07:17 PDT ---
I've skim-read but not reviewed in detail yet.

(In reply to comment #0)
> 1. _is_valid() is called after _propagate_error()

I was worried about this when Guillaume re-ordered these calls, so I
investigated. As far as I can tell, these are the possible situations: only the
last one is problematic.

(By "invalid path", I mean "can't be reached unless the programmer is using it
wrong".)

* Valid path: result is valid, error is set. Error is correctly propagated
either way.

* Valid path: result is valid, error is not set. propagate_error returns FALSE,
validity is checked, finish succeeds.

* Invalid path: result has been disposed already. propagate_error fails its
initial g_return_if_fail, criticals and returns FALSE. As a result, is_valid is
called; it *also* fails its check, and the finish function criticals and
returns too.

* Invalid path: result has the wrong source tag and no error. propagate_error
returns FALSE, the delayed check runs anyway, and all is good.

* Invalid path: result has the wrong source tag but has an error set. As a
result of the order we use, a NULL source tag isn't diagnosed (good for
g_simple_async_report_in_idle), but an incorrect non-NULL source tag isn't
diagnosed either (failure to diagnose certain programmer errors, but then, this
*is* C).

Avoiding g_simple_async_report_error_in_idle, just for this, seems to me to
make our code less clear for very little gain?

> 2. Cancellable are not always reffed during the async calls
> 
> When cancelling a call, it is correct to do "g_cancellable_cancel(c);
> g_object_unref (c);". Some of the async functions are not safe for that since
> they don't acquire a reference on the cancellable. Patch 002 fixes all the
> calls that where not doing proper ref/unref of cancellables.

This looks good in general, and I approve of making use of the return value of
g_object_ref.

> +  if (job->cancellable)
> +    g_object_unref (job->cancellable);
> +
> +  job->source_object = NULL;                 <------
> +  job->cancellable = NULL;

I'd prefer to keep all the stuff dealing with source_object together, *then* do
all the stuff dealing with cancellable (in other words, move the indicated line
before the "if").

> +  if (cancellable != NULL)
> +    priv->output_cancellable = g_object_ref (cancellable);
>  
> -  priv->output_cancellable = cancellable;

This does change the semantics for the case where cancellable is NULL: if
output_cancellable isn't already NULL, it's not overwritten. This seems to be
checked for already, but it might be worth asserting.

> 3. The macro is missing scope

This is secretly several commits...

> * Make the wocky_implement_*() macro safer by calling _is_valid() before
>   trying to propagate errors.

As explained above, I'm not sure how useful this actually is.

> * Fixed missing scope (G_STMT_START/G_STMT_END) and missing parenthesis

This bit certainly seems worthwhile to me, although I think you may mean
"parentheses" (parenthesis is the singular).

> +  GSimpleAsyncResult *__simple; \

You didn't mention in the commit message that you switched to
double-underscored variable names. All double-underscored names are reserved
for use by the compiler/C library, so this is wrong. Use a single underscore if
you want to avoid collisions with library-user-defined variable names (but to
be honest, these macros are the entire body of a function, so I don't really
see what they'd be colliding with).

-- 
Configure bugmail: https://bugs.freedesktop.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the QA contact for the bug.