Re: [Qemu-devel] [PATCH 5/8] io: add ability to associate an error with a task

[Eric Blake]

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 <address@hidden>
> ---
>  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