Message ID | 1268239869-16058-1-git-send-email-aliguori@us.ibm.com |
---|---|
State | New |
Headers | show |
> +struct QEMUNotifier > +{ > + void (*notify)(QEMUNotifier *notifier); > +}; > I suggest combining this with QEMUBH. Paul
On 03/11/2010 01:57 PM, Paul Brook wrote: > +struct QEMUNotifier > > +{ > > + void (*notify)(QEMUNotifier *notifier); > > +}; > > I suggest combining this with QEMUBH. I didn't understand this suggestion exactly, but I think it's related that I didn't understand the advantage of making QEMUNotifier a struct. Instead of using container_of, reusing QEMUBHFunc (renamed to QEMUCallbackFunc maybe?) in QEMUNotifierNode like this: struct QEMUNotifierNode { QEMUCallbackFunc notify; void *opaque; QTAILQ_ENTRY(QEMUNotifierNode) node; }; void qemu_notifier_list_init(QEMUNotifierList *list); struct QEMUNotifierNode * qemu_notifier_list_add(QEMUNotifierList *list, QEMUCallbackFunc notify, void *opaque); void qemu_notifier_list_remove(QEMUNotifierList *list, QEMUNotifierNode *notifier); void qemu_notifier_list_notify(QEMUNotifierList *list); seems cleaner. You would place the QEMUNotifierNode in VncState in order to do the removal later. Besides this, the patchset is very nice indeed. Paolo
On 03/10/2010 06:51 PM, Anthony Liguori wrote: > + > +#ifndef QEMU_NOTIFY_H > +#define QEMU_NOTIFY_H > + > +#include "qemu-queue.h" > + > +typedef struct QEMUNotifier QEMUNotifier; > +typedef struct QEMUNotifierNode QEMUNotifierNode; > + > +struct QEMUNotifier > +{ > + void (*notify)(QEMUNotifier *notifier); > +}; > + > +struct QEMUNotifierNode > +{ > + QEMUNotifier *notifier; > + QTAILQ_ENTRY(QEMUNotifierNode) node; > +}; > + > +typedef struct QEMUNotifierList > +{ > + QTAILQ_HEAD(, QEMUNotifierNode) notifiers; > +} QEMUNotifierList; > + > +#define QEMU_NOTIFIER_LIST_INITIALIZER(head) \ > + { QTAILQ_HEAD_INITIALIZER((head).notifiers) } > + > +void qemu_notifier_list_init(QEMUNotifierList *list); > + > +void qemu_notifier_list_add(QEMUNotifierList *list, QEMUNotifier *notifier); > + > +void qemu_notifier_list_remove(QEMUNotifierList *list, QEMUNotifier *notifier); > + > +void qemu_notifier_list_notify(QEMUNotifierList *list); > + > Why the qemu_ prefixes everywhere? They make sense when wrapping library calls, but in native qemu code they're just noise.
On 03/11/2010 03:25 PM, Paolo Bonzini wrote: > On 03/11/2010 01:57 PM, Paul Brook wrote: >> +struct QEMUNotifier >> > +{ >> > + void (*notify)(QEMUNotifier *notifier); >> > +}; >> >> I suggest combining this with QEMUBH. > > I didn't understand this suggestion exactly, but I think it's related > that I didn't understand the advantage of making QEMUNotifier a > struct. Instead of using container_of, reusing QEMUBHFunc (renamed to > QEMUCallbackFunc maybe?) in QEMUNotifierNode like this: > > struct QEMUNotifierNode { > QEMUCallbackFunc notify; > void *opaque; > QTAILQ_ENTRY(QEMUNotifierNode) node; > }; > > void qemu_notifier_list_init(QEMUNotifierList *list); > > struct QEMUNotifierNode * > qemu_notifier_list_add(QEMUNotifierList *list, > QEMUCallbackFunc notify, void *opaque); > > void qemu_notifier_list_remove(QEMUNotifierList *list, > QEMUNotifierNode *notifier); > > void qemu_notifier_list_notify(QEMUNotifierList *list); > > seems cleaner. You would place the QEMUNotifierNode in VncState in > order to do the removal later. > > I disagree. container_of() is both a little more type safe, and removes the need for an extra pointer and memory object. The caller will almost always have an object in which to embed the notifier, best to make use of it.
> On 03/11/2010 01:57 PM, Paul Brook wrote: > > +struct QEMUNotifier > > > > > +{ > > > + void (*notify)(QEMUNotifier *notifier); > > > +}; > > > > I suggest combining this with QEMUBH. > > I didn't understand this suggestion exactly, but I think it's related > that I didn't understand the advantage of making QEMUNotifier a struct. My point is that we already have a mechanism for providing event notification callbacks, specifically QEMUBH. Why invent a new one? Paul
On 03/11/2010 06:57 AM, Paul Brook wrote: >> +struct QEMUNotifier >> +{ >> + void (*notify)(QEMUNotifier *notifier); >> +}; >> >> > I suggest combining this with QEMUBH. > I take it your not opposed to converting QEMUBH to be a QEMUNotifier? If so, I'm happy to do it. Regards, Anthony Liguori > Paul > > >
On 03/11/2010 07:25 AM, Paolo Bonzini wrote: > On 03/11/2010 01:57 PM, Paul Brook wrote: >> +struct QEMUNotifier >> > +{ >> > + void (*notify)(QEMUNotifier *notifier); >> > +}; >> >> I suggest combining this with QEMUBH. > > I didn't understand this suggestion exactly, but I think it's related > that I didn't understand the advantage of making QEMUNotifier a > struct. Instead of using container_of, reusing QEMUBHFunc (renamed to > QEMUCallbackFunc maybe?) in QEMUNotifierNode like this: I like treating a slot as a single object instead of as function pointer/opaque pair. It gives us better type safety and reduces the amount of parameters that need to be passed around. Regards, Anthony Liguori
On 03/11/2010 07:36 AM, Avi Kivity wrote: > On 03/10/2010 06:51 PM, Anthony Liguori wrote: >> + >> +#ifndef QEMU_NOTIFY_H >> +#define QEMU_NOTIFY_H >> + >> +#include "qemu-queue.h" >> + >> +typedef struct QEMUNotifier QEMUNotifier; >> +typedef struct QEMUNotifierNode QEMUNotifierNode; >> + >> +struct QEMUNotifier >> +{ >> + void (*notify)(QEMUNotifier *notifier); >> +}; >> + >> +struct QEMUNotifierNode >> +{ >> + QEMUNotifier *notifier; >> + QTAILQ_ENTRY(QEMUNotifierNode) node; >> +}; >> + >> +typedef struct QEMUNotifierList >> +{ >> + QTAILQ_HEAD(, QEMUNotifierNode) notifiers; >> +} QEMUNotifierList; >> + >> +#define QEMU_NOTIFIER_LIST_INITIALIZER(head) \ >> + { QTAILQ_HEAD_INITIALIZER((head).notifiers) } >> + >> +void qemu_notifier_list_init(QEMUNotifierList *list); >> + >> +void qemu_notifier_list_add(QEMUNotifierList *list, QEMUNotifier >> *notifier); >> + >> +void qemu_notifier_list_remove(QEMUNotifierList *list, QEMUNotifier >> *notifier); >> + >> +void qemu_notifier_list_notify(QEMUNotifierList *list); >> + > > > Why the qemu_ prefixes everywhere? They make sense when wrapping > library calls, but in native qemu code they're just noise. I don't disagree, but we do this a lot in code today. I think if folks generally agreed that qemu prefixes were just noise, we should make a concerted effort in the future to prevent people from introducing more of them and make a note in CODING_STYLE. Regards, Anthony Liguori
> On 03/11/2010 06:57 AM, Paul Brook wrote: > >> +struct QEMUNotifier > >> +{ > >> + void (*notify)(QEMUNotifier *notifier); > >> +}; > > > > I suggest combining this with QEMUBH. > > I take it your not opposed to converting QEMUBH to be a QEMUNotifier? > If so, I'm happy to do it. It's unclear to me why you've invented a new thing in the first place. Paul
On 03/11/2010 02:42 PM, Avi Kivity wrote: > On 03/11/2010 03:25 PM, Paolo Bonzini wrote: >> I didn't understand the advantage of making QEMUNotifier a >> struct. Instead of using container_of, reusing QEMUBHFunc (renamed to >> QEMUCallbackFunc maybe?) in QEMUNotifierNode [...] >> seems cleaner. You would place the QEMUNotifierNode in VncState in >> order to do the removal later. > > I disagree. container_of() is both a little more type safe, and removes > the need for an extra pointer and memory object. > The caller will almost always have an object in which to embed the > notifier, best to make use of it. It doesn't remove the need for an extra memory object. Anthony's design embeds the Notifier but not the NotifierNode. Indeed, my design does have an extra pointer (in the NotifierNode, which grows from 3 to 4 words). I still don't like container_of much, but maybe I'll grow my appreciation of it with time. :-) Paolo
On 03/11/2010 02:58 PM, Paul Brook wrote: >> On 03/11/2010 01:57 PM, Paul Brook wrote: >>> +struct QEMUNotifier >>> >>>> +{ >>>> + void (*notify)(QEMUNotifier *notifier); >>>> +}; >>> >>> I suggest combining this with QEMUBH. >> >> I didn't understand this suggestion exactly, but I think it's related >> that I didn't understand the advantage of making QEMUNotifier a struct. > > My point is that we already have a mechanism for providing event notification > callbacks, specifically QEMUBH. Why invent a new one? QEMUBH seems seriously overengineered for this simple task. Paolo
On 03/11/2010 04:36 PM, Paolo Bonzini wrote: > On 03/11/2010 02:42 PM, Avi Kivity wrote: >> On 03/11/2010 03:25 PM, Paolo Bonzini wrote: >>> I didn't understand the advantage of making QEMUNotifier a >>> struct. Instead of using container_of, reusing QEMUBHFunc (renamed to >>> QEMUCallbackFunc maybe?) in QEMUNotifierNode [...] >>> seems cleaner. You would place the QEMUNotifierNode in VncState in >>> order to do the removal later. >> >> I disagree. container_of() is both a little more type safe, and removes >> the need for an extra pointer and memory object. > > The caller will almost always have an object in which to embed the > > notifier, best to make use of it. > > It doesn't remove the need for an extra memory object. Anthony's > design embeds the Notifier but not the NotifierNode. Indeed, my > design does have an extra pointer (in the NotifierNode, which grows > from 3 to 4 words). Right. Well, it should. > I still don't like container_of much, but maybe I'll grow my > appreciation of it with time. :-) Or grow your dislike of void pointers.
On 03/11/2010 08:19 AM, Paul Brook wrote: >> On 03/11/2010 06:57 AM, Paul Brook wrote: >> >>>> +struct QEMUNotifier >>>> +{ >>>> + void (*notify)(QEMUNotifier *notifier); >>>> +}; >>>> >>> I suggest combining this with QEMUBH. >>> >> I take it your not opposed to converting QEMUBH to be a QEMUNotifier? >> If so, I'm happy to do it. >> > It's unclear to me why you've invented a new thing in the first place. > This style of callback has a number of advantages: - It provides better type safety since - It's a more compact representation - It maps to a function object (functor) which is a pretty common pattern in most high level languages Regards, Anthony Liguori > Paul >
On 03/11/2010 08:42 AM, Avi Kivity wrote: > On 03/11/2010 04:36 PM, Paolo Bonzini wrote: >> On 03/11/2010 02:42 PM, Avi Kivity wrote: >>> On 03/11/2010 03:25 PM, Paolo Bonzini wrote: >>>> I didn't understand the advantage of making QEMUNotifier a >>>> struct. Instead of using container_of, reusing QEMUBHFunc (renamed to >>>> QEMUCallbackFunc maybe?) in QEMUNotifierNode [...] >>>> seems cleaner. You would place the QEMUNotifierNode in VncState in >>>> order to do the removal later. >>> >>> I disagree. container_of() is both a little more type safe, and removes >>> the need for an extra pointer and memory object. >> > The caller will almost always have an object in which to embed the >> > notifier, best to make use of it. >> >> It doesn't remove the need for an extra memory object. Anthony's >> design embeds the Notifier but not the NotifierNode. Indeed, my >> design does have an extra pointer (in the NotifierNode, which grows >> from 3 to 4 words). > > Right. Well, it should. It's certainly possible (and reasonable) to stick the QTAIL_NODE() into QEMUNotifier. Regards, Anthony Liguori >> I still don't like container_of much, but maybe I'll grow my >> appreciation of it with time. :-) > > Or grow your dislike of void pointers. >
On 03/11/2010 04:54 PM, Anthony Liguori wrote: >>> I take it your not opposed to converting QEMUBH to be a QEMUNotifier? >>> If so, I'm happy to do it. >> It's unclear to me why you've invented a new thing in the first place. > > This style of callback has a number of advantages: > > - It provides better type safety since > - It's a more compact representation > - It maps to a function object (functor) which is a pretty common > pattern in most high level languages - The name isn't an eyesore
On 03/11/2010 08:19 AM, Paul Brook wrote: >> On 03/11/2010 06:57 AM, Paul Brook wrote: >> >>>> +struct QEMUNotifier >>>> +{ >>>> + void (*notify)(QEMUNotifier *notifier); >>>> +}; >>>> >>> I suggest combining this with QEMUBH. >>> >> I take it your not opposed to converting QEMUBH to be a QEMUNotifier? >> If so, I'm happy to do it. >> > It's unclear to me why you've invented a new thing in the first place. > It's a better approximation of the command pattern because the command is a single object as opposed to a tuple. Because the command is an object, you can also do things like binding. For instance: Which now gives you a notifier that has an fd bound to it's second argument (which is pretty useful for IO dispatch). You can do this with a tuple representation, but it gets awkward. One could argue for formalizing the tuple as a struct but extending by nesting becomes more complicated. Also, for the most part, you already have a state for the command and embedding the object means less dynamic memory allocation and less code to handle that. Regards, Anthony Liguori
On 03/15/2010 03:31 PM, Anthony Liguori wrote: > On 03/11/2010 08:19 AM, Paul Brook wrote: >>> On 03/11/2010 06:57 AM, Paul Brook wrote: >>>>> +struct QEMUNotifier >>>>> +{ >>>>> + void (*notify)(QEMUNotifier *notifier); >>>>> +}; >>>> I suggest combining this with QEMUBH. >>> I take it your not opposed to converting QEMUBH to be a QEMUNotifier? >>> If so, I'm happy to do it. >> It's unclear to me why you've invented a new thing in the first place. > > It's a better approximation of the command pattern because the command > is a single object as opposed to a tuple. Because the command is an > object, you can also do things like binding. For instance: typedef struct IONotifier { Notifier parent; void (*notify)(IONotifier *notifier, int fd); int fd; } IONotifier; It's been a long day... > Which now gives you a notifier that has an fd bound to it's second > argument (which is pretty useful for IO dispatch). > > You can do this with a tuple representation, but it gets awkward. One > could argue for formalizing the tuple as a struct but extending by > nesting becomes more complicated. Also, for the most part, you > already have a state for the command and embedding the object means > less dynamic memory allocation and less code to handle that. > > Regards, > > Anthony Liguori > Regards, Anthony Liguori
diff --git a/Makefile.objs b/Makefile.objs index e791dd5..dcb5a92 100644 --- a/Makefile.objs +++ b/Makefile.objs @@ -104,6 +104,7 @@ common-obj-$(CONFIG_VNC_TLS) += vnc-tls.o vnc-auth-vencrypt.o common-obj-$(CONFIG_VNC_SASL) += vnc-auth-sasl.o common-obj-$(CONFIG_COCOA) += cocoa.o common-obj-$(CONFIG_IOTHREAD) += qemu-thread.o +common-obj-y += notify.o slirp-obj-y = cksum.o if.o ip_icmp.o ip_input.o ip_output.o slirp-obj-y += slirp.o mbuf.o misc.o sbuf.o socket.o tcp_input.o tcp_output.o diff --git a/notify.c b/notify.c new file mode 100644 index 0000000..cbb4796 --- /dev/null +++ b/notify.c @@ -0,0 +1,53 @@ +/* + * Notifier lists + * + * Copyright IBM, Corp. 2010 + * + * Authors: + * Anthony Liguori <aliguori@us.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ + +#include "qemu-common.h" +#include "notify.h" + +void qemu_notifier_list_init(QEMUNotifierList *list) +{ + QTAILQ_INIT(&list->notifiers); +} + +void qemu_notifier_list_add(QEMUNotifierList *list, QEMUNotifier *notifier) +{ + QEMUNotifierNode *node = qemu_mallocz(sizeof(*node)); + + node->notifier = notifier; + QTAILQ_INSERT_HEAD(&list->notifiers, node, node); +} + +void qemu_notifier_list_remove(QEMUNotifierList *list, QEMUNotifier *notifier) +{ + QEMUNotifierNode *node; + + QTAILQ_FOREACH(node, &list->notifiers, node) { + if (node->notifier == notifier) { + break; + } + } + + if (node) { + QTAILQ_REMOVE(&list->notifiers, node, node); + qemu_free(node); + } +} + +void qemu_notifier_list_notify(QEMUNotifierList *list) +{ + QEMUNotifierNode *node, *node_next; + + QTAILQ_FOREACH_SAFE(node, &list->notifiers, node, node_next) { + node->notifier->notify(node->notifier); + } +} diff --git a/notify.h b/notify.h new file mode 100644 index 0000000..075d302 --- /dev/null +++ b/notify.h @@ -0,0 +1,49 @@ +/* + * Notifier lists + * + * Copyright IBM, Corp. 2010 + * + * Authors: + * Anthony Liguori <aliguori@us.ibm.com> + * + * This work is licensed under the terms of the GNU GPL, version 2. See + * the COPYING file in the top-level directory. + * + */ + +#ifndef QEMU_NOTIFY_H +#define QEMU_NOTIFY_H + +#include "qemu-queue.h" + +typedef struct QEMUNotifier QEMUNotifier; +typedef struct QEMUNotifierNode QEMUNotifierNode; + +struct QEMUNotifier +{ + void (*notify)(QEMUNotifier *notifier); +}; + +struct QEMUNotifierNode +{ + QEMUNotifier *notifier; + QTAILQ_ENTRY(QEMUNotifierNode) node; +}; + +typedef struct QEMUNotifierList +{ + QTAILQ_HEAD(, QEMUNotifierNode) notifiers; +} QEMUNotifierList; + +#define QEMU_NOTIFIER_LIST_INITIALIZER(head) \ + { QTAILQ_HEAD_INITIALIZER((head).notifiers) } + +void qemu_notifier_list_init(QEMUNotifierList *list); + +void qemu_notifier_list_add(QEMUNotifierList *list, QEMUNotifier *notifier); + +void qemu_notifier_list_remove(QEMUNotifierList *list, QEMUNotifier *notifier); + +void qemu_notifier_list_notify(QEMUNotifierList *list); + +#endif
Notifiers are data-less callbacks and a notifier list is a list of registered notifiers that all are interested in a particular event. We'll use this in a few patches to implement mouse change notification. Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> --- Makefile.objs | 1 + notify.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++++++ notify.h | 49 +++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 103 insertions(+), 0 deletions(-) create mode 100644 notify.c create mode 100644 notify.h