diff mbox

[1/7] Add support for generic notifier lists

Message ID 1268239869-16058-1-git-send-email-aliguori@us.ibm.com
State New
Headers show

Commit Message

Anthony Liguori March 10, 2010, 4:51 p.m. UTC 
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

Comments

Paul Brook March 11, 2010, 12:57 p.m. UTC | #1 
> +struct QEMUNotifier
> +{
> +    void (*notify)(QEMUNotifier *notifier);
> +};
> 

I suggest combining this with QEMUBH.

Paul
Paolo Bonzini March 11, 2010, 1:25 p.m. UTC | #2 
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
Avi Kivity March 11, 2010, 1:36 p.m. UTC | #3 
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.
Avi Kivity March 11, 2010, 1:42 p.m. UTC | #4 
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.
Paul Brook March 11, 2010, 1:58 p.m. UTC | #5 
> 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
Anthony Liguori March 11, 2010, 2:09 p.m. UTC | #6 
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
>
>
>
Anthony Liguori March 11, 2010, 2:11 p.m. UTC | #7 
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
Anthony Liguori March 11, 2010, 2:12 p.m. UTC | #8 
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
Paul Brook March 11, 2010, 2:19 p.m. UTC | #9 
> 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
Paolo Bonzini March 11, 2010, 2:36 p.m. UTC | #10 
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
Paolo Bonzini March 11, 2010, 2:39 p.m. UTC | #11 
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
Avi Kivity March 11, 2010, 2:42 p.m. UTC | #12 
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.
Anthony Liguori March 11, 2010, 2:54 p.m. UTC | #13 
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
>
Anthony Liguori March 11, 2010, 3:08 p.m. UTC | #14 
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.
>
Avi Kivity March 11, 2010, 3:19 p.m. UTC | #15 
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
Anthony Liguori March 15, 2010, 8:31 p.m. UTC | #16 
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
Anthony Liguori March 15, 2010, 11:37 p.m. UTC | #17 
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 mbox

Patch

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