| Message ID | 1369917299-5725-2-git-send-email-stefanha@redhat.com |
|---|---|
| State | New |
| Headers | show |
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). 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? At any rate, adding a new naming scheme minimizes churn, and the code itself looks okay; Reviewed-by: Eric Blake <eblake@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
Am 30.05.2013 um 14:34 hat Stefan Hajnoczi geschrieben: > 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> Reviewed-by: Kevin Wolf <kwolf@redhat.com>
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 */ +typedef struct NotifierWithReturn NotifierWithReturn; + +struct NotifierWithReturn { + int (*notify)(NotifierWithReturn *notifier, void *data); + QLIST_ENTRY(NotifierWithReturn) node; +}; + +typedef struct NotifierWithReturnList { + QLIST_HEAD(, NotifierWithReturn) notifiers; +} NotifierWithReturnList; + +#define NOTIFIER_WITH_RETURN_LIST_INITIALIZER(head) \ + { QLIST_HEAD_INITIALIZER((head).notifiers) } + +void notifier_with_return_list_init(NotifierWithReturnList *list); + +void notifier_with_return_list_add(NotifierWithReturnList *list, + NotifierWithReturn *notifier); + +void notifier_with_return_remove(NotifierWithReturn *notifier); + +int notifier_with_return_list_notify(NotifierWithReturnList *list, + void *data); + #endif diff --git a/util/notify.c b/util/notify.c index 7b7692a..f215dfc 100644 --- a/util/notify.c +++ b/util/notify.c @@ -39,3 +39,33 @@ void notifier_list_notify(NotifierList *list, void *data) notifier->notify(notifier, data); } } + +void notifier_with_return_list_init(NotifierWithReturnList *list) +{ + QLIST_INIT(&list->notifiers); +} + +void notifier_with_return_list_add(NotifierWithReturnList *list, + NotifierWithReturn *notifier) +{ + QLIST_INSERT_HEAD(&list->notifiers, notifier, node); +} + +void notifier_with_return_remove(NotifierWithReturn *notifier) +{ + QLIST_REMOVE(notifier, node); +} + +int notifier_with_return_list_notify(NotifierWithReturnList *list, void *data) +{ + NotifierWithReturn *notifier, *next; + int ret = 0; + + QLIST_FOREACH_SAFE(notifier, &list->notifiers, node, next) { + ret = notifier->notify(notifier, data); + if (ret != 0) { + break; + } + } + return ret; +}
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(+)