Re: [Qemu-devel] [PATCH v5 01/11] notify: add NotiferWithReturn so notifier list can abort

[Stefan Hajnoczi <stefanha@redhat.com>]

On Thu, May 30, 2013 at 04:27:48PM -0600, Eric Blake wrote:
> On 05/30/2013 06:34 AM, Stefan Hajnoczi wrote:
> > notifier_list_notify() has no return value.  This is fine when we just
> > want to invoke side-effects.
> > 
> > Sometimes it's useful for notifiers to produce a return value.  This
> > allows notifiers to "veto" an operation and will be used by the block
> > layer before-write notifier.
> > 
> > Signed-off-by: Stefan Hajnoczi <stefanha@redhat.com>
> > ---
> >  include/qemu/notify.h | 25 +++++++++++++++++++++++++
> >  util/notify.c         | 30 ++++++++++++++++++++++++++++++
> >  2 files changed, 55 insertions(+)
> > 
> > diff --git a/include/qemu/notify.h b/include/qemu/notify.h
> > index 4e2e7f0..d3103e7 100644
> > --- a/include/qemu/notify.h
> > +++ b/include/qemu/notify.h
> > @@ -40,4 +40,29 @@ void notifier_remove(Notifier *notifier);
> >  
> >  void notifier_list_notify(NotifierList *list, void *data);
> >  
> > +/* Same as Notifier but allows .notify() to return errors */
> 
> It's probably worth documenting that the callback must return 0 for
> success, and that the first non-zero return is passed back without
> further interpretation (thus allowing negative errno values if desired,
> vs. a simpler -1).

You are right, let's improve the doc comments.

> Isn't this really just syntax sugar since existing notifiers could
> already use a member within the opaque *data argument as a way to
> short-circuit later notifiers on earlier errors?  If so, how hard would
> it be to scrub the existing code base to always return 0 in existing
> notifiers, rather than adding a parallel naming scheme?

I considered both approaches and decided on NotifierWithReturn because
it neither touches existing users nor adds a side-effect for aborting.
BTW adding a side-effect in the BdrvTrackedRequest case is ugly, the
struct is used for more than just the pre-write notifier and the field
would be useless in the other cases.

Stefan