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 <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'?
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
--
|: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :|
|: http://libvirt.org -o- http://virt-manager.org :|
|: http://entangle-photo.org -o- http://search.cpan.org/~danberr/ :|
