On 01/05/2017 10:03 AM, Daniel P. Berrange wrote: > Currently when a task fails, the error is never explicitly > associated with the task object, it is just passed along > through the completion callback. This adds ability to
s/adds/adds the/
> explicitly associate an error with the task.
>
> Signed-off-by: Daniel P. Berrange <berra...@redhat.com>
> ---
> include/io/task.h | 29 +++++++++++++++++++++++++++++
> io/task.c | 23 +++++++++++++++++++++++
> 2 files changed, 52 insertions(+)
>
> diff --git a/include/io/task.h b/include/io/task.h
> index ece1372..244c1a1 100644
> --- a/include/io/task.h
> +++ b/include/io/task.h
> @@ -240,6 +240,35 @@ void qio_task_abort(QIOTask *task,
>
>
> /**
> + * qio_task_set_error:
> + * @task: the task struct
> + * @err: pointer to the error
> + *
> + * Associate an error with the task, which can later
> + * be retrieved with the qio_task_propagate_error()
> + * method. This method takes ownership of @err, so
> + * it is not valid to access it after this call
> + * completes.
> + */
> +void qio_task_set_error(QIOTask *task,
> + Error *err);
Is it valid for @err to be NULL (or put another way, may callers blindly
call this function whether or not an error has occurred)?...
> +
> +
> +/**
> + * qio_task_propagate_error:
> + * @task: the task struct
> + * @errp: pointer to a NULL-initialized error object
> + *
> + * Propagate the error associated with @task
> + * into @errp.
> + *
> + * Returns: true if an error was propagated, false otherwise
> + */
> +gboolean qio_task_propagate_error(QIOTask *task,
Must this return 'gboolean' (in which case the docs should mention
TRUE/FALSE rather than true/false)? Or can we make it return the nicer
'bool'?
> + Error **errp);
> +
> +
> +/**
> * qio_task_set_result_pointer:
> * @task: the task struct
> * @result: pointer to the result data
> diff --git a/io/task.c b/io/task.c
> index 675e196..1136c75 100644
> --- a/io/task.c
> +++ b/io/task.c
> @@ -29,6 +29,7 @@ struct QIOTask {
> QIOTaskFunc func;
> gpointer opaque;
> GDestroyNotify destroy;
> + Error *err;
> gpointer result;
> GDestroyNotify destroyResult;
> };
> @@ -62,6 +63,9 @@ static void qio_task_free(QIOTask *task)
> if (task->destroyResult) {
> task->destroyResult(task->result);
> }
> + if (task->err) {
> + error_free(task->err);
> + }
> object_unref(task->source);
>
> g_free(task);
> @@ -159,6 +163,25 @@ void qio_task_abort(QIOTask *task,
> }
>
>
> +void qio_task_set_error(QIOTask *task,
> + Error *err)
> +{
> + error_propagate(&task->err, err);
...As written, qio_task_set_error(task, NULL) is thus valid as a no-op;
and in turn, qio_task_set_error(task, err) is unconditionally valid a
cleanup path regardless of whether the cleanup was reached on success
(err is still NULL) or failure (err is set). But it's worth documenting.
In fact, error_propagate() can be called more than once (first call
wins, all later calls silently drop the subsequent error in favor of a
first one already present), and therefore qio_task_set_error() gains
that same property. Worth documenting?
> +}
> +
> +
> +gboolean qio_task_propagate_error(QIOTask *task,
> + Error **errp)
> +{
> + if (task->err) {
> + error_propagate(errp, task->err);
> + return TRUE;
> + }
> +
> + return FALSE;
Again, I think a 'bool' return (and true/false) is nicer.
> +}
> +
> +
> void qio_task_set_result_pointer(QIOTask *task,
> gpointer result,
> GDestroyNotify destroy)
>
The idea makes sense, though.
--
Eric Blake eblake redhat com +1-919-301-3266
Libvirt virtualization library http://libvirt.org
signature.asc
Description: OpenPGP digital signature
