[Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Anthony Liguori]

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

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
-- 
1.6.5.2

[Qemu-devel] [PATCH 2/7] Rewrite mouse handlers to use QTAILQ and to have an activation function

[Anthony Liguori]

And convert usb-hid to use it (to avoid regression with bisection)

Right now, when we do info mice and we've added a usb tablet, we don't see it
until the guest starts using the tablet.  We implement this behavior in order
to provide a means to delay registration of a mouse handler since we treat
the last registered handler as the current handler.

This is a usability problem though as we would like to give the user feedback
that they've either 1) not added an absolute device 2) there is an absolute
device but the guest isn't using it 3) we have an absolute device and it's
active.

By using QTAILQ and having an explicit activation function that moves the
handler to the front of the queue, we can implement the same semantics as
before with respect to automatically switching to usb tablet while providing
the user with a whole lot more information.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 console.h    |    6 +++-
 hw/usb-hid.c |   15 ++++++--
 input.c      |  109 +++++++++++++++++++++++-----------------------------------
 3 files changed, 59 insertions(+), 71 deletions(-)

diff --git a/console.h b/console.h
index 71e8ff2..94d9cae 100644
--- a/console.h
+++ b/console.h
@@ -28,8 +28,10 @@ typedef struct QEMUPutMouseEntry {
     int qemu_put_mouse_event_absolute;
     char *qemu_put_mouse_event_name;
 
+    int index;
+
     /* used internally by qemu for handling mice */
-    struct QEMUPutMouseEntry *next;
+    QTAILQ_ENTRY(QEMUPutMouseEntry) node;
 } QEMUPutMouseEntry;
 
 typedef struct QEMUPutLEDEntry {
@@ -43,6 +45,8 @@ QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name);
 void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry);
+void qemu_activate_mouse_event_handler(QEMUPutMouseEntry *entry);
+
 QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque);
 void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
 
diff --git a/hw/usb-hid.c b/hw/usb-hid.c
index 2e4e647..8e6c6e0 100644
--- a/hw/usb-hid.c
+++ b/hw/usb-hid.c
@@ -511,8 +511,7 @@ static int usb_mouse_poll(USBHIDState *hs, uint8_t *buf, int len)
     USBMouseState *s = &hs->ptr;
 
     if (!s->mouse_grabbed) {
-	s->eh_entry = qemu_add_mouse_event_handler(usb_mouse_event, hs,
-                                                  0, "QEMU USB Mouse");
+        qemu_activate_mouse_event_handler(s->eh_entry);
 	s->mouse_grabbed = 1;
     }
 
@@ -553,8 +552,7 @@ static int usb_tablet_poll(USBHIDState *hs, uint8_t *buf, int len)
     USBMouseState *s = &hs->ptr;
 
     if (!s->mouse_grabbed) {
-	s->eh_entry = qemu_add_mouse_event_handler(usb_tablet_event, hs,
-                                                  1, "QEMU USB Tablet");
+        qemu_activate_mouse_event_handler(s->eh_entry);
 	s->mouse_grabbed = 1;
     }
 
@@ -866,6 +864,15 @@ static int usb_hid_initfn(USBDevice *dev, int kind)
     USBHIDState *s = DO_UPCAST(USBHIDState, dev, dev);
     s->dev.speed = USB_SPEED_FULL;
     s->kind = kind;
+
+    if (s->kind == USB_MOUSE) {
+        s->ptr.eh_entry = qemu_add_mouse_event_handler(usb_mouse_event, s,
+                                                       0, "QEMU USB Mouse");
+    } else if (s->kind == USB_TABLET) {
+        s->ptr.eh_entry = qemu_add_mouse_event_handler(usb_tablet_event, s,
+                                                       1, "QEMU USB Tablet");
+    }
+        
     /* Force poll routine to be run and grab input the first time.  */
     s->changed = 1;
     return 0;
diff --git a/input.c b/input.c
index baaa4c6..397bfc5 100644
--- a/input.c
+++ b/input.c
@@ -31,9 +31,9 @@
 
 static QEMUPutKBDEvent *qemu_put_kbd_event;
 static void *qemu_put_kbd_event_opaque;
-static QEMUPutMouseEntry *qemu_put_mouse_event_head;
-static QEMUPutMouseEntry *qemu_put_mouse_event_current;
 static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
+static QTAILQ_HEAD(, QEMUPutMouseEntry) mouse_handlers =
+    QTAILQ_HEAD_INITIALIZER(mouse_handlers);
 
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
 {
@@ -45,7 +45,8 @@ QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name)
 {
-    QEMUPutMouseEntry *s, *cursor;
+    QEMUPutMouseEntry *s;
+    static int mouse_index = 0;
 
     s = qemu_mallocz(sizeof(QEMUPutMouseEntry));
 
@@ -53,51 +54,24 @@ QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
     s->qemu_put_mouse_event_opaque = opaque;
     s->qemu_put_mouse_event_absolute = absolute;
     s->qemu_put_mouse_event_name = qemu_strdup(name);
-    s->next = NULL;
+    s->index = mouse_index++;
 
-    if (!qemu_put_mouse_event_head) {
-        qemu_put_mouse_event_head = qemu_put_mouse_event_current = s;
-        return s;
-    }
-
-    cursor = qemu_put_mouse_event_head;
-    while (cursor->next != NULL)
-        cursor = cursor->next;
-
-    cursor->next = s;
-    qemu_put_mouse_event_current = s;
+    QTAILQ_INSERT_TAIL(&mouse_handlers, s, node);
 
     return s;
 }
 
-void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
+void qemu_activate_mouse_event_handler(QEMUPutMouseEntry *entry)
 {
-    QEMUPutMouseEntry *prev = NULL, *cursor;
-
-    if (!qemu_put_mouse_event_head || entry == NULL)
-        return;
-
-    cursor = qemu_put_mouse_event_head;
-    while (cursor != NULL && cursor != entry) {
-        prev = cursor;
-        cursor = cursor->next;
-    }
+    QTAILQ_REMOVE(&mouse_handlers, entry, node);
+    QTAILQ_INSERT_HEAD(&mouse_handlers, entry, node);
 
-    if (cursor == NULL) // does not exist or list empty
-        return;
-    else if (prev == NULL) { // entry is head
-        qemu_put_mouse_event_head = cursor->next;
-        if (qemu_put_mouse_event_current == entry)
-            qemu_put_mouse_event_current = cursor->next;
-        qemu_free(entry->qemu_put_mouse_event_name);
-        qemu_free(entry);
-        return;
-    }
-
-    prev->next = entry->next;
+    check_mode_change();
+}
 
-    if (qemu_put_mouse_event_current == entry)
-        qemu_put_mouse_event_current = prev;
+void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
+{
+    QTAILQ_REMOVE(&mouse_handlers, entry, node);
 
     qemu_free(entry->qemu_put_mouse_event_name);
     qemu_free(entry);
@@ -142,39 +116,41 @@ void kbd_put_ledstate(int ledstate)
 
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state)
 {
+    QEMUPutMouseEntry *entry;
     QEMUPutMouseEvent *mouse_event;
     void *mouse_event_opaque;
     int width;
 
-    if (!qemu_put_mouse_event_current) {
+    if (QTAILQ_EMPTY(&mouse_handlers)) {
         return;
     }
 
-    mouse_event =
-        qemu_put_mouse_event_current->qemu_put_mouse_event;
-    mouse_event_opaque =
-        qemu_put_mouse_event_current->qemu_put_mouse_event_opaque;
+    entry = QTAILQ_FIRST(&mouse_handlers);
+
+    mouse_event = entry->qemu_put_mouse_event;
+    mouse_event_opaque = entry->qemu_put_mouse_event_opaque;
 
     if (mouse_event) {
         if (graphic_rotate) {
-            if (qemu_put_mouse_event_current->qemu_put_mouse_event_absolute)
+            if (entry->qemu_put_mouse_event_absolute)
                 width = 0x7fff;
             else
                 width = graphic_width - 1;
             mouse_event(mouse_event_opaque,
-                                 width - dy, dx, dz, buttons_state);
+                        width - dy, dx, dz, buttons_state);
         } else
             mouse_event(mouse_event_opaque,
-                                 dx, dy, dz, buttons_state);
+                        dx, dy, dz, buttons_state);
     }
 }
 
 int kbd_mouse_is_absolute(void)
 {
-    if (!qemu_put_mouse_event_current)
+    if (QTAILQ_EMPTY(&mouse_handlers)) {
         return 0;
+    }
 
-    return qemu_put_mouse_event_current->qemu_put_mouse_event_absolute;
+    return QTAILQ_FIRST(&mouse_handlers)->qemu_put_mouse_event_absolute;
 }
 
 static void info_mice_iter(QObject *data, void *opaque)
@@ -222,23 +198,23 @@ void do_info_mice(Monitor *mon, QObject **ret_data)
 {
     QEMUPutMouseEntry *cursor;
     QList *mice_list;
-    int index = 0;
+    int current;
 
     mice_list = qlist_new();
 
-    if (!qemu_put_mouse_event_head) {
+    if (QTAILQ_EMPTY(&mouse_handlers)) {
         goto out;
     }
 
-    cursor = qemu_put_mouse_event_head;
-    while (cursor != NULL) {
+    current = QTAILQ_FIRST(&mouse_handlers)->index;
+
+    QTAILQ_FOREACH(cursor, &mouse_handlers, node) {
         QObject *obj;
         obj = qobject_from_jsonf("{ 'name': %s, 'index': %d, 'current': %i }",
                                  cursor->qemu_put_mouse_event_name,
-                                 index, cursor == qemu_put_mouse_event_current);
+                                 cursor->index,
+                                 cursor->index == current);
         qlist_append_obj(mice_list, obj);
-        index++;
-        cursor = cursor->next;
     }
 
 out:
@@ -248,22 +224,23 @@ out:
 void do_mouse_set(Monitor *mon, const QDict *qdict)
 {
     QEMUPutMouseEntry *cursor;
-    int i = 0;
     int index = qdict_get_int(qdict, "index");
+    int found = 0;
 
-    if (!qemu_put_mouse_event_head) {
+    if (QTAILQ_EMPTY(&mouse_handlers)) {
         monitor_printf(mon, "No mouse devices connected\n");
         return;
     }
 
-    cursor = qemu_put_mouse_event_head;
-    while (cursor != NULL && index != i) {
-        i++;
-        cursor = cursor->next;
+    QTAILQ_FOREACH(cursor, &mouse_handlers, node) {
+        if (cursor->index == index) {
+            found = 1;
+            qemu_activate_mouse_event_handler(cursor);
+            break;
+        }
     }
 
-    if (cursor != NULL)
-        qemu_put_mouse_event_current = cursor;
-    else
+    if (!found) {
         monitor_printf(mon, "Mouse at given index not found\n");
+    }
 }
-- 
1.6.5.2

[Qemu-devel] [PATCH 3/7] Add kbd_mouse_has_absolute()

[Anthony Liguori]

kbd_mouse_is_absolute tells us whether the current mouse handler is an absolute
device.  kbd_mouse_has_absolute tells us whether we have any device that is
capable of absolute input.

This lets us tell a user that they have configured an absolute device but that
the guest is not currently using it.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 console.h |    5 +++++
 input.c   |   13 +++++++++++++
 2 files changed, 18 insertions(+), 0 deletions(-)

diff --git a/console.h b/console.h
index 94d9cae..f3c619f 100644
--- a/console.h
+++ b/console.h
@@ -53,8 +53,13 @@ void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry);
 void kbd_put_keycode(int keycode);
 void kbd_put_ledstate(int ledstate);
 void kbd_mouse_event(int dx, int dy, int dz, int buttons_state);
+
+/* Does the current mouse generate absolute events */
 int kbd_mouse_is_absolute(void);
 
+/* Of all the mice, is there one that generates absolute events */
+int kbd_mouse_has_absolute(void);
+
 struct MouseTransformInfo {
     /* Touchscreen resolution */
     int x;
diff --git a/input.c b/input.c
index 397bfc5..8d5a14d 100644
--- a/input.c
+++ b/input.c
@@ -153,6 +153,19 @@ int kbd_mouse_is_absolute(void)
     return QTAILQ_FIRST(&mouse_handlers)->qemu_put_mouse_event_absolute;
 }
 
+int kbd_mouse_has_absolute(void)
+{
+    QEMUPutMouseEntry *entry;
+
+    QTAILQ_FOREACH(entry, &mouse_handlers, node) {
+        if (entry->qemu_put_mouse_event_absolute) {
+            return 1;
+        }
+    }
+
+    return 0;
+}
+
 static void info_mice_iter(QObject *data, void *opaque)
 {
     QDict *mouse;
-- 
1.6.5.2

[Qemu-devel] [PATCH 4/7] Add notifier for mouse mode changes

[Anthony Liguori]

Right now, DisplayState clients rely on polling the mouse mode to determine
when the device is changed to an absolute device.  Use a notification list to
add an explicit notification.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 console.h |    3 +++
 input.c   |   37 ++++++++++++++++++++++++++++++++++++-
 2 files changed, 39 insertions(+), 1 deletions(-)

diff --git a/console.h b/console.h
index f3c619f..cb928cf 100644
--- a/console.h
+++ b/console.h
@@ -3,6 +3,7 @@
 
 #include "qemu-char.h"
 #include "qdict.h"
+#include "notify.h"
 
 /* keyboard/mouse support */
 
@@ -56,6 +57,8 @@ void kbd_mouse_event(int dx, int dy, int dz, int buttons_state);
 
 /* Does the current mouse generate absolute events */
 int kbd_mouse_is_absolute(void);
+void qemu_add_mouse_mode_change_notifier(QEMUNotifier *notify);
+void qemu_remove_mouse_mode_change_notifier(QEMUNotifier *notify);
 
 /* Of all the mice, is there one that generates absolute events */
 int kbd_mouse_has_absolute(void);
diff --git a/input.c b/input.c
index 8d5a14d..cbf55b7 100644
--- a/input.c
+++ b/input.c
@@ -28,12 +28,13 @@
 #include "console.h"
 #include "qjson.h"
 
-
 static QEMUPutKBDEvent *qemu_put_kbd_event;
 static void *qemu_put_kbd_event_opaque;
 static QTAILQ_HEAD(, QEMUPutLEDEntry) led_handlers = QTAILQ_HEAD_INITIALIZER(led_handlers);
 static QTAILQ_HEAD(, QEMUPutMouseEntry) mouse_handlers =
     QTAILQ_HEAD_INITIALIZER(mouse_handlers);
+static QEMUNotifierList mouse_mode_notifiers = 
+    QEMU_NOTIFIER_LIST_INITIALIZER(mouse_mode_notifiers);
 
 void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
 {
@@ -41,6 +42,24 @@ void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque)
     qemu_put_kbd_event = func;
 }
 
+static void check_mode_change(void)
+{
+    static int current_is_absolute, current_has_absolute;
+    int is_absolute;
+    int has_absolute;
+
+    is_absolute = kbd_mouse_is_absolute();
+    has_absolute = kbd_mouse_has_absolute();
+
+    if (is_absolute != current_is_absolute ||
+        has_absolute != current_has_absolute) {
+        qemu_notifier_list_notify(&mouse_mode_notifiers);
+    }
+
+    current_is_absolute = is_absolute;
+    current_has_absolute = has_absolute;
+}
+
 QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
                                                 void *opaque, int absolute,
                                                 const char *name)
@@ -58,6 +77,8 @@ QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func,
 
     QTAILQ_INSERT_TAIL(&mouse_handlers, s, node);
 
+    check_mode_change();
+
     return s;
 }
 
@@ -75,6 +96,8 @@ void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry)
 
     qemu_free(entry->qemu_put_mouse_event_name);
     qemu_free(entry);
+
+    check_mode_change();
 }
 
 QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func,
@@ -256,4 +279,16 @@ void do_mouse_set(Monitor *mon, const QDict *qdict)
     if (!found) {
         monitor_printf(mon, "Mouse at given index not found\n");
     }
+
+    check_mode_change();
+}
+
+void qemu_add_mouse_mode_change_notifier(QEMUNotifier *notify)
+{
+    qemu_notifier_list_add(&mouse_mode_notifiers, notify);
+}
+
+void qemu_remove_mouse_mode_change_notifier(QEMUNotifier *notify)
+{
+    qemu_notifier_list_remove(&mouse_mode_notifiers, notify);
 }
-- 
1.6.5.2

[Qemu-devel] [PATCH 5/7] Expose whether a mouse is an absolute device via QMP and the human monitor.

[Anthony Liguori]

For QMP, we just add an attribute which is backwards compatible.  For the human
monitor, we add (absolute) to the end of the line.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 input.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/input.c b/input.c
index cbf55b7..d2a6f26 100644
--- a/input.c
+++ b/input.c
@@ -195,9 +195,10 @@ static void info_mice_iter(QObject *data, void *opaque)
     Monitor *mon = opaque;
 
     mouse = qobject_to_qdict(data);
-    monitor_printf(mon, "%c Mouse #%" PRId64 ": %s\n",
+    monitor_printf(mon, "%c Mouse #%" PRId64 ": %s%s\n",
                   (qdict_get_bool(mouse, "current") ? '*' : ' '),
-                  qdict_get_int(mouse, "index"), qdict_get_str(mouse, "name"));
+                   qdict_get_int(mouse, "index"), qdict_get_str(mouse, "name"),
+                   qdict_get_bool(mouse, "absolute") ? " (absolute)" : "");
 }
 
 void do_info_mice_print(Monitor *mon, const QObject *data)
@@ -224,11 +225,12 @@ void do_info_mice_print(Monitor *mon, const QObject *data)
  * - "name": mouse's name
  * - "index": mouse's index
  * - "current": true if this mouse is receiving events, false otherwise
+ * - "absolute": true if the mouse generates absolute input events
  *
  * Example:
  *
- * [ { "name": "QEMU Microsoft Mouse", "index": 0, "current": false },
- *   { "name": "QEMU PS/2 Mouse", "index": 1, "current": true } ]
+ * [ { "name": "QEMU Microsoft Mouse", "index": 0, "current": false, "absolute": false },
+ *   { "name": "QEMU PS/2 Mouse", "index": 1, "current": true, "absolute": true } ]
  */
 void do_info_mice(Monitor *mon, QObject **ret_data)
 {
@@ -246,10 +248,14 @@ void do_info_mice(Monitor *mon, QObject **ret_data)
 
     QTAILQ_FOREACH(cursor, &mouse_handlers, node) {
         QObject *obj;
-        obj = qobject_from_jsonf("{ 'name': %s, 'index': %d, 'current': %i }",
+        obj = qobject_from_jsonf("{ 'name': %s,"
+                                 "  'index': %d,"
+                                 "  'current': %i,"
+                                 "  'absolute': %i }",
                                  cursor->qemu_put_mouse_event_name,
                                  cursor->index,
-                                 cursor->index == current);
+                                 cursor->index == current,
+                                 !!cursor->qemu_put_mouse_event_absolute);
         qlist_append_obj(mice_list, obj);
     }
 
-- 
1.6.5.2

[Qemu-devel] [PATCH 6/7] input: make vnc use mouse mode notifiers

[Anthony Liguori]

When we switch to absolute mode, we send out a notification (if the client
supports it).  Today, we only send this notification when the client sends us
a mouse event and we're in the wrong mode.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 vnc.c |   13 ++++++++-----
 vnc.h |    2 ++
 2 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/vnc.c b/vnc.c
index 7ce73dc..f4d7f12 100644
--- a/vnc.c
+++ b/vnc.c
@@ -1110,6 +1110,7 @@ static void vnc_disconnect_finish(VncState *vs)
         dcl->idle = 1;
     }
 
+    qemu_remove_mouse_mode_change_notifier(&vs->mouse_mode_notifier);
     vnc_remove_timer(vs->vd);
     qemu_remove_led_event_handler(vs->led);
     qemu_free(vs);
@@ -1427,8 +1428,11 @@ static void client_cut_text(VncState *vs, size_t len, uint8_t *text)
 {
 }
 
-static void check_pointer_type_change(VncState *vs, int absolute)
+static void check_pointer_type_change(QEMUNotifier *notifier)
 {
+    VncState *vs = container_of(notifier, VncState, mouse_mode_notifier);
+    int absolute = kbd_mouse_is_absolute();
+
     if (vnc_has_feature(vs, VNC_FEATURE_POINTER_TYPE_CHANGE) && vs->absolute != absolute) {
         vnc_write_u8(vs, 0);
         vnc_write_u8(vs, 0);
@@ -1474,8 +1478,6 @@ static void pointer_event(VncState *vs, int button_mask, int x, int y)
         vs->last_x = x;
         vs->last_y = y;
     }
-
-    check_pointer_type_change(vs, kbd_mouse_is_absolute());
 }
 
 static void reset_keys(VncState *vs)
@@ -1814,8 +1816,6 @@ static void set_encodings(VncState *vs, int32_t *encodings, size_t n_encodings)
             break;
         }
     }
-
-    check_pointer_type_change(vs, kbd_mouse_is_absolute());
 }
 
 static void set_pixel_conversion(VncState *vs)
@@ -2431,6 +2431,9 @@ static void vnc_connect(VncDisplay *vd, int csock)
     reset_keys(vs);
     vs->led = qemu_add_led_event_handler(kbd_leds, vs);
 
+    vs->mouse_mode_notifier.notify = check_pointer_type_change;
+    qemu_add_mouse_mode_change_notifier(&vs->mouse_mode_notifier);
+
     vnc_init_timer(vd);
 
     /* vs might be free()ed here */
diff --git a/vnc.h b/vnc.h
index 0fc89bd..e1d764a 100644
--- a/vnc.h
+++ b/vnc.h
@@ -167,6 +167,8 @@ struct VncState
     Buffer zlib_tmp;
     z_stream zlib_stream[4];
 
+    QEMUNotifier mouse_mode_notifier;
+
     QTAILQ_ENTRY(VncState) next;
 };
 
-- 
1.6.5.2

[Qemu-devel] [PATCH 7/7] sdl: use mouse mode notifier

[Anthony Liguori]

Today we poll the mouse mode whenever there is a mouse movement.  There is a
subtle usability problem with this though.

If we're in relative mode and grab is enabled, when we change to absolute mode,
we break grab.  This gives a user a seamless transition when the new pointer
is enabled.

But because we poll for mouse change, this grab break won't occur until the user
attempts to move the mouse.  By using notifiers, the grab break happens as soon
as possible.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 sdl.c |   31 ++++++++++++++++++++-----------
 1 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/sdl.c b/sdl.c
index 1d1e100..c51eb90 100644
--- a/sdl.c
+++ b/sdl.c
@@ -57,6 +57,7 @@ static SDL_Cursor *guest_sprite = NULL;
 static uint8_t allocator;
 static SDL_PixelFormat host_format;
 static int scaling_active = 0;
+static QEMUNotifier mouse_mode_notifier;
 
 static void sdl_update(DisplayState *ds, int x, int y, int w, int h)
 {
@@ -485,6 +486,22 @@ static void sdl_grab_end(void)
     sdl_update_caption();
 }
 
+static void sdl_mouse_mode_change(QEMUNotifier *notify)
+{
+    if (kbd_mouse_is_absolute()) {
+        if (!absolute_enabled) {
+            sdl_hide_cursor();
+            if (gui_grab) {
+                sdl_grab_end();
+            }
+            absolute_enabled = 1;
+        }
+    } else if (absolute_enabled) {
+	sdl_show_cursor();
+	absolute_enabled = 0;
+    }
+}
+
 static void sdl_send_mouse_event(int dx, int dy, int dz, int x, int y, int state)
 {
     int buttons;
@@ -497,19 +514,8 @@ static void sdl_send_mouse_event(int dx, int dy, int dz, int x, int y, int state
         buttons |= MOUSE_EVENT_MBUTTON;
 
     if (kbd_mouse_is_absolute()) {
-	if (!absolute_enabled) {
-	    sdl_hide_cursor();
-	    if (gui_grab) {
-		sdl_grab_end();
-	    }
-	    absolute_enabled = 1;
-	}
-
        dx = x * 0x7FFF / (width - 1);
        dy = y * 0x7FFF / (height - 1);
-    } else if (absolute_enabled) {
-	sdl_show_cursor();
-	absolute_enabled = 0;
     } else if (guest_cursor) {
         x -= guest_x;
         y -= guest_y;
@@ -875,6 +881,9 @@ void sdl_display_init(DisplayState *ds, int full_screen, int no_frame)
         dpy_resize(ds);
     }
 
+    mouse_mode_notifier.notify = sdl_mouse_mode_change;
+    qemu_add_mouse_mode_change_notifier(&mouse_mode_notifier);
+
     sdl_update_caption();
     SDL_EnableKeyRepeat(250, 50);
     gui_grab = 0;
-- 
1.6.5.2

[Qemu-devel] Re: [PATCH 6/7] input: make vnc use mouse mode notifiers

[Gerd Hoffmann]

On 03/10/10 17:51, Anthony Liguori wrote:
> When we switch to absolute mode, we send out a notification (if the client
> supports it).  Today, we only send this notification when the client sends us
> a mouse event and we're in the wrong mode.

Very nice.

ACK for the whole patch series.

cheers,
   Gerd

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Paul Brook]

> +struct QEMUNotifier
> +{
> +    void (*notify)(QEMUNotifier *notifier);
> +};
> 

I suggest combining this with QEMUBH.

Paul

[Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Paolo Bonzini]

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

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Paul Brook]

> 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

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[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.

Regards,

Anthony Liguori

> Paul
>
>
>    

Re: [Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[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

[Qemu-devel] Re: [PATCH 5/7] Expose whether a mouse is an absolute device via QMP and the human monitor.

[Luiz Capitulino]

On Wed, 10 Mar 2010 10:51:07 -0600
Anthony Liguori <aliguori@us.ibm.com> wrote:

> For QMP, we just add an attribute which is backwards compatible.  For the human
> monitor, we add (absolute) to the end of the line.
> 
> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>

 Looks good to me.

> ---
>  input.c |   18 ++++++++++++------
>  1 files changed, 12 insertions(+), 6 deletions(-)
> 
> diff --git a/input.c b/input.c
> index cbf55b7..d2a6f26 100644
> --- a/input.c
> +++ b/input.c
> @@ -195,9 +195,10 @@ static void info_mice_iter(QObject *data, void *opaque)
>      Monitor *mon = opaque;
>  
>      mouse = qobject_to_qdict(data);
> -    monitor_printf(mon, "%c Mouse #%" PRId64 ": %s\n",
> +    monitor_printf(mon, "%c Mouse #%" PRId64 ": %s%s\n",
>                    (qdict_get_bool(mouse, "current") ? '*' : ' '),
> -                  qdict_get_int(mouse, "index"), qdict_get_str(mouse, "name"));
> +                   qdict_get_int(mouse, "index"), qdict_get_str(mouse, "name"),
> +                   qdict_get_bool(mouse, "absolute") ? " (absolute)" : "");
>  }
>  
>  void do_info_mice_print(Monitor *mon, const QObject *data)
> @@ -224,11 +225,12 @@ void do_info_mice_print(Monitor *mon, const QObject *data)
>   * - "name": mouse's name
>   * - "index": mouse's index
>   * - "current": true if this mouse is receiving events, false otherwise
> + * - "absolute": true if the mouse generates absolute input events
>   *
>   * Example:
>   *
> - * [ { "name": "QEMU Microsoft Mouse", "index": 0, "current": false },
> - *   { "name": "QEMU PS/2 Mouse", "index": 1, "current": true } ]
> + * [ { "name": "QEMU Microsoft Mouse", "index": 0, "current": false, "absolute": false },
> + *   { "name": "QEMU PS/2 Mouse", "index": 1, "current": true, "absolute": true } ]
>   */
>  void do_info_mice(Monitor *mon, QObject **ret_data)
>  {
> @@ -246,10 +248,14 @@ void do_info_mice(Monitor *mon, QObject **ret_data)
>  
>      QTAILQ_FOREACH(cursor, &mouse_handlers, node) {
>          QObject *obj;
> -        obj = qobject_from_jsonf("{ 'name': %s, 'index': %d, 'current': %i }",
> +        obj = qobject_from_jsonf("{ 'name': %s,"
> +                                 "  'index': %d,"
> +                                 "  'current': %i,"
> +                                 "  'absolute': %i }",
>                                   cursor->qemu_put_mouse_event_name,
>                                   cursor->index,
> -                                 cursor->index == current);
> +                                 cursor->index == current,
> +                                 !!cursor->qemu_put_mouse_event_absolute);
>          qlist_append_obj(mice_list, obj);
>      }
>  

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Paul Brook]

> 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

Re: [Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Paolo Bonzini]

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

[Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Paolo Bonzini]

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

Re: [Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Avi Kivity]

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.

-- 
error compiling committee.c: too many arguments to function

[Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Avi Kivity]

Signed-off-by: Avi Kivity <avi@redhat.com>
---
 CODING_STYLE |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/CODING_STYLE b/CODING_STYLE
index a579cb1..92036f3 100644
--- a/CODING_STYLE
+++ b/CODING_STYLE
@@ -49,6 +49,9 @@ and is therefore likely to be changed.
 Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
 QEMU coding style.
 
+When wrapping standard library functions, use the prefix qemu_ to alert
+readers that they are seeing a wrapped version; otherwise avoid this prefix.
+
 4. Block structure
 
 Every indented statement is braced; even if the block contains just one
-- 
1.7.0.2

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Anthony Liguori]

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
>    

[Qemu-devel] Re: [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Anthony Liguori]

On 03/11/2010 08:48 AM, Avi Kivity wrote:
> Signed-off-by: Avi Kivity<avi@redhat.com>
> ---
>   CODING_STYLE |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..92036f3 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>   Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>   QEMU coding style.
>
> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> +
>    

Acked-by: Anthony Liguori <aliguori@us.ibm.com>

But I'd like to also see others agree with this.

Regards,

Anthony Liguori

>   4. Block structure
>
>   Every indented statement is braced; even if the block contains just one
>    

Re: [Qemu-devel] Re: [PATCH 1/7] Add support for generic notifier lists

[Anthony Liguori]

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.
>

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Avi Kivity]

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

-- 
error compiling committee.c: too many arguments to function

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Markus Armbruster]

Avi Kivity <avi@redhat.com> writes:

> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  CODING_STYLE |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..92036f3 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>  Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>  QEMU coding style.
>  
> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> +
>  4. Block structure
>  
>  Every indented statement is braced; even if the block contains just one

ACK

Re: [Qemu-devel] Re: [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Jamie Lokier]

Anthony Liguori wrote:
> On 03/11/2010 08:48 AM, Avi Kivity wrote:
> >Signed-off-by: Avi Kivity<avi@redhat.com>
> >---
> >  CODING_STYLE |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> >diff --git a/CODING_STYLE b/CODING_STYLE
> >index a579cb1..92036f3 100644
> >--- a/CODING_STYLE
> >+++ b/CODING_STYLE
> >@@ -49,6 +49,9 @@ and is therefore likely to be changed.
> >  Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
> >  QEMU coding style.
> >
> >+When wrapping standard library functions, use the prefix qemu_ to alert
> >+readers that they are seeing a wrapped version; otherwise avoid this 
> >prefix.
> >+
> >   
> 
> Acked-by: Anthony Liguori <aliguori@us.ibm.com>
> 
> But I'd like to also see others agree with this.

I agree - not that I've contributed any code, mind you, but I do read
bits of it from time to time.

However if qemu is ever made into a library used by other
applications, i.e. libqemu.a or libqemu.so, then you might decide
application-visible functions should have the prefix

-- Jamie

[Qemu-devel] Re: [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Juan Quintela]

Avi Kivity <avi@redhat.com> wrote:
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  CODING_STYLE |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..92036f3 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>  Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>  QEMU coding style.
>  
> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> +
>  4. Block structure
>  
>  Every indented statement is braced; even if the block contains just one

Acked-by: Juan Quintela

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Aurelien Jarno]

On Thu, Mar 11, 2010 at 04:48:43PM +0200, Avi Kivity wrote:
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  CODING_STYLE |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..92036f3 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>  Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>  QEMU coding style.
>  
> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> +
>  4. Block structure
>  
>  Every indented statement is braced; even if the block contains just one

Acked-by: Aurelien Jarno <aurelien@aurel32.net>

-- 
Aurelien Jarno	                        GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Paul Brook]

> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this
>  prefix. +

I'm tempted to say "When wrapping or providing functionality that could easily 
be confused with standard library functions [...]".

I'm not going to argue either way though.

Paul

Re: [Qemu-devel] Re: [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Blue Swirl]

On 3/11/10, Anthony Liguori <anthony@codemonkey.ws> wrote:
> On 03/11/2010 08:48 AM, Avi Kivity wrote:
>
> > Signed-off-by: Avi Kivity<avi@redhat.com>
> > ---
> >  CODING_STYLE |    3 +++
> >  1 files changed, 3 insertions(+), 0 deletions(-)
> >
> > diff --git a/CODING_STYLE b/CODING_STYLE
> > index a579cb1..92036f3 100644
> > --- a/CODING_STYLE
> > +++ b/CODING_STYLE
> > @@ -49,6 +49,9 @@ and is therefore likely to be changed.
> >  Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
> >  QEMU coding style.
> >
> > +When wrapping standard library functions, use the prefix qemu_ to alert
> > +readers that they are seeing a wrapped version; otherwise avoid this
> prefix.
> > +
> >
> >
>
>  Acked-by: Anthony Liguori <aliguori@us.ibm.com>
>
>  But I'd like to also see others agree with this.

OK for me too.

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Edgar E. Iglesias]

On Thu, Mar 11, 2010 at 04:48:43PM +0200, Avi Kivity wrote:
> Signed-off-by: Avi Kivity <avi@redhat.com>
> ---
>  CODING_STYLE |    3 +++
>  1 files changed, 3 insertions(+), 0 deletions(-)
> 
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..92036f3 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>  Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>  QEMU coding style.
>  
> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> +
>  4. Block structure
>  
>  Every indented statement is braced; even if the block contains just one


Not sure what "standard library functions" includes but I think the
qemu prefix should be allowed whenever one needs to wrap external
code blocks into qemu.

I don't feel very strongly about this though. I can call my stuff
wrap_xxx if I need to :)

Cheers,
Edde

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Avi Kivity]

On 03/13/2010 04:55 AM, Edgar E. Iglesias wrote:
> On Thu, Mar 11, 2010 at 04:48:43PM +0200, Avi Kivity wrote:
>    
>> Signed-off-by: Avi Kivity<avi@redhat.com>
>> ---
>>   CODING_STYLE |    3 +++
>>   1 files changed, 3 insertions(+), 0 deletions(-)
>>
>> diff --git a/CODING_STYLE b/CODING_STYLE
>> index a579cb1..92036f3 100644
>> --- a/CODING_STYLE
>> +++ b/CODING_STYLE
>> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>>   Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>>   QEMU coding style.
>>
>> +When wrapping standard library functions, use the prefix qemu_ to alert
>> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
>> +
>>   4. Block structure
>>
>>   Every indented statement is braced; even if the block contains just one
>>      
>
> Not sure what "standard library functions" includes but I think the
> qemu prefix should be allowed whenever one needs to wrap external
> code blocks into qemu.
>
>    

That was the intent.

> I don't feel very strongly about this though. I can call my stuff
> wrap_xxx if I need to :)
>
>    

A downside of a formal coding style document is that people start 
lawyering about it.  Common sense can still be applied, if there is a 
use case that is similar to what is described, surely qemu_ can still hold.

-- 
Do not meddle in the internals of kernels, for they are subtle and quick to panic.

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Edgar E. Iglesias]

On Sat, Mar 13, 2010 at 10:17:37AM +0200, Avi Kivity wrote:
> On 03/13/2010 04:55 AM, Edgar E. Iglesias wrote:
> > On Thu, Mar 11, 2010 at 04:48:43PM +0200, Avi Kivity wrote:
> >    
> >> Signed-off-by: Avi Kivity<avi@redhat.com>
> >> ---
> >>   CODING_STYLE |    3 +++
> >>   1 files changed, 3 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/CODING_STYLE b/CODING_STYLE
> >> index a579cb1..92036f3 100644
> >> --- a/CODING_STYLE
> >> +++ b/CODING_STYLE
> >> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
> >>   Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
> >>   QEMU coding style.
> >>
> >> +When wrapping standard library functions, use the prefix qemu_ to alert
> >> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> >> +
> >>   4. Block structure
> >>
> >>   Every indented statement is braced; even if the block contains just one
> >>      
> >
> > Not sure what "standard library functions" includes but I think the
> > qemu prefix should be allowed whenever one needs to wrap external
> > code blocks into qemu.
> >    
> 
> That was the intent.

Great

ACK

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[Anthony Liguori]

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

Re: [Qemu-devel] [PATCH 1/7] Add support for generic notifier lists

[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

Re: [Qemu-devel] [PATCH] CODING_STYLE: Reserve qemu_ prefix for library wrappers

[Anthony Liguori]

On 03/11/2010 08:48 AM, Avi Kivity wrote:
> Signed-off-by: Avi Kivity<avi@redhat.com>
> ---
>   CODING_STYLE |    3 +++
>   1 files changed, 3 insertions(+), 0 deletions(-)
>
> diff --git a/CODING_STYLE b/CODING_STYLE
> index a579cb1..92036f3 100644
> --- a/CODING_STYLE
> +++ b/CODING_STYLE
> @@ -49,6 +49,9 @@ and is therefore likely to be changed.
>   Typedefs are used to eliminate the redundant 'struct' keyword.  It is the
>   QEMU coding style.
>
> +When wrapping standard library functions, use the prefix qemu_ to alert
> +readers that they are seeing a wrapped version; otherwise avoid this prefix.
> +
>   4. Block structure
>
>   Every indented statement is braced; even if the block contains just one
>    

Applied.  Thanks.

Regards,

Anthony Liguori

[Qemu-devel] [PATCH 5/7] Expose whether a mouse is an absolute device via QMP and the human monitor.

[Anthony Liguori]

For QMP, we just add an attribute which is backwards compatible.  For the human
monitor, we add (absolute) to the end of the line.

Signed-off-by: Anthony Liguori <aliguori@us.ibm.com>
---
 input.c |   18 ++++++++++++------
 1 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/input.c b/input.c
index c956e06..8f0941e 100644
--- a/input.c
+++ b/input.c
@@ -195,9 +195,10 @@ static void info_mice_iter(QObject *data, void *opaque)
     Monitor *mon = opaque;
 
     mouse = qobject_to_qdict(data);
-    monitor_printf(mon, "%c Mouse #%" PRId64 ": %s\n",
+    monitor_printf(mon, "%c Mouse #%" PRId64 ": %s%s\n",
                   (qdict_get_bool(mouse, "current") ? '*' : ' '),
-                  qdict_get_int(mouse, "index"), qdict_get_str(mouse, "name"));
+                   qdict_get_int(mouse, "index"), qdict_get_str(mouse, "name"),
+                   qdict_get_bool(mouse, "absolute") ? " (absolute)" : "");
 }
 
 void do_info_mice_print(Monitor *mon, const QObject *data)
@@ -224,11 +225,12 @@ void do_info_mice_print(Monitor *mon, const QObject *data)
  * - "name": mouse's name
  * - "index": mouse's index
  * - "current": true if this mouse is receiving events, false otherwise
+ * - "absolute": true if the mouse generates absolute input events
  *
  * Example:
  *
- * [ { "name": "QEMU Microsoft Mouse", "index": 0, "current": false },
- *   { "name": "QEMU PS/2 Mouse", "index": 1, "current": true } ]
+ * [ { "name": "QEMU Microsoft Mouse", "index": 0, "current": false, "absolute": false },
+ *   { "name": "QEMU PS/2 Mouse", "index": 1, "current": true, "absolute": true } ]
  */
 void do_info_mice(Monitor *mon, QObject **ret_data)
 {
@@ -246,10 +248,14 @@ void do_info_mice(Monitor *mon, QObject **ret_data)
 
     QTAILQ_FOREACH(cursor, &mouse_handlers, node) {
         QObject *obj;
-        obj = qobject_from_jsonf("{ 'name': %s, 'index': %d, 'current': %i }",
+        obj = qobject_from_jsonf("{ 'name': %s,"
+                                 "  'index': %d,"
+                                 "  'current': %i,"
+                                 "  'absolute': %i }",
                                  cursor->qemu_put_mouse_event_name,
                                  cursor->index,
-                                 cursor->index == current);
+                                 cursor->index == current,
+                                 !!cursor->qemu_put_mouse_event_absolute);
         qlist_append_obj(mice_list, obj);
     }
 
-- 
1.6.5.2