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

[Daniel P. Berrange]

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

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/ :|