[5/8] io: add ability to associate an error with a task

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
explicitly associate an error with the task.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
---
 include/io/task.h | 29 +++++++++++++++++++++++++++++
 io/task.c         | 23 +++++++++++++++++++++++
 2 files changed, 52 insertions(+)

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 <berrange@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.

On Thu, Jan 05, 2017 at 03:03:08PM -0600, Eric Blake wrote:
> 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 <berrange@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'?

bool is fine - gboolean is just from my habit of using glib

> > @@ -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?

Ok, will expand docs.


Regards,
Daniel