| Message ID | 1267087161-15204-2-git-send-email-kraxel@redhat.com |
|---|---|
| State | New |
| Headers | show |
Gerd Hoffmann <kraxel@redhat.com> wrote: > Adds infrastructure for keyboard led status tracking to qemu. > } QEMUPutMouseEntry; > > +typedef struct QEMUPutLEDEntry { > + QEMUPutLEDEvent *put_led; > + void *opaque; > + struct QEMUPutLEDEntry *next; > +} QEMUPutLEDEntry; Please, change this to a QLIST(), code becomes way simpler. > +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) > +{ > + QEMUPutLEDEntry *prev = NULL, *cursor; > + > + if (!qemu_put_led_event_head || entry == NULL) > + return; > + > + cursor = qemu_put_led_event_head; > + while (cursor != NULL && cursor != entry) { > + prev = cursor; > + cursor = cursor->next; > + } > + > + if (cursor == NULL) // does not exist or list empty > + return; > + > + if (prev == NULL) { // entry is head > + qemu_put_led_event_head = cursor->next; > + } else { > + prev->next = entry->next; > + } > + qemu_free(entry); > +} This functions simplifies to: void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) { QLIST_REMOVE(entry, leds); qemu_free(entry); } Rest of gains are smaller, but easier to read. Concept of the patch is ok with me. Later, Juan.
On 02/25/2010 02:39 AM, Gerd Hoffmann wrote: > Adds infrastructure for keyboard led status tracking to qemu. > > Signed-off-by: Gerd Hoffmann<kraxel@redhat.com> > This is an obvious extension to the current API so I'm not necessarily opposed to it. But I wonder if it really makes sense to treat all of these things differently since we end up duplicating a lot of code. Would it make more sense to just introduce: typedef struct QEMUInputHandler { void (*put_kbd_event)(QEMUInputHandler *obj, int keycode); void (*put_led_event)(QEMUInputHandler *obj, int ledstate); void (*put_mouse_event)(QEMUInputHandler *obj, int dx, int dy, int dz, int buttons_state); QLIST_ENTRY(QEMUInputHandler) node; } QEMUInputHandler; void qemu_add_input_handler(QEMUInputHandler *handler); void qemu_remove_input_handler(QEMUInputHandler *handler); Regards, Anthony Liguori > --- > console.h | 14 ++++++++++++++ > input.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 64 insertions(+), 0 deletions(-) > > diff --git a/console.h b/console.h > index 916859d..0e969d1 100644 > --- a/console.h > +++ b/console.h > @@ -10,10 +10,15 @@ > #define MOUSE_EVENT_RBUTTON 0x02 > #define MOUSE_EVENT_MBUTTON 0x04 > > +#define QEMU_SCROLL_LOCK_LED (1<< 0) > +#define QEMU_NUM_LOCK_LED (1<< 1) > +#define QEMU_CAPS_LOCK_LED (1<< 2) > + > /* in ms */ > #define GUI_REFRESH_INTERVAL 30 > > typedef void QEMUPutKBDEvent(void *opaque, int keycode); > +typedef void QEMUPutLEDEvent(void *opaque, int ledstate); > typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state); > > typedef struct QEMUPutMouseEntry { > @@ -26,13 +31,22 @@ typedef struct QEMUPutMouseEntry { > struct QEMUPutMouseEntry *next; > } QEMUPutMouseEntry; > > +typedef struct QEMUPutLEDEntry { > + QEMUPutLEDEvent *put_led; > + void *opaque; > + struct QEMUPutLEDEntry *next; > +} QEMUPutLEDEntry; > + > void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque); > QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func, > void *opaque, int absolute, > const char *name); > void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry); > +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque); > +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); > int kbd_mouse_is_absolute(void); > > diff --git a/input.c b/input.c > index 955b9ab..82bc85c 100644 > --- a/input.c > +++ b/input.c > @@ -31,6 +31,7 @@ > > static QEMUPutKBDEvent *qemu_put_kbd_event; > static void *qemu_put_kbd_event_opaque; > +static QEMUPutLEDEntry *qemu_put_led_event_head; > static QEMUPutMouseEntry *qemu_put_mouse_event_head; > static QEMUPutMouseEntry *qemu_put_mouse_event_current; > > @@ -102,6 +103,44 @@ void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry) > qemu_free(entry); > } > > +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, > + void *opaque) > +{ > + QEMUPutLEDEntry *s; > + > + s = qemu_mallocz(sizeof(QEMUPutLEDEntry)); > + > + s->put_led = func; > + s->opaque = opaque; > + s->next = qemu_put_led_event_head; > + qemu_put_led_event_head = s; > + return s; > +} > + > +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) > +{ > + QEMUPutLEDEntry *prev = NULL, *cursor; > + > + if (!qemu_put_led_event_head || entry == NULL) > + return; > + > + cursor = qemu_put_led_event_head; > + while (cursor != NULL&& cursor != entry) { > + prev = cursor; > + cursor = cursor->next; > + } > + > + if (cursor == NULL) // does not exist or list empty > + return; > + > + if (prev == NULL) { // entry is head > + qemu_put_led_event_head = cursor->next; > + } else { > + prev->next = entry->next; > + } > + qemu_free(entry); > +} > + > void kbd_put_keycode(int keycode) > { > if (qemu_put_kbd_event) { > @@ -109,6 +148,17 @@ void kbd_put_keycode(int keycode) > } > } > > +void kbd_put_ledstate(int ledstate) > +{ > + QEMUPutLEDEntry *cursor; > + > + cursor = qemu_put_led_event_head; > + while (cursor != NULL) { > + cursor->put_led(cursor->opaque, ledstate); > + cursor = cursor->next; > + } > +} > + > void kbd_mouse_event(int dx, int dy, int dz, int buttons_state) > { > QEMUPutMouseEvent *mouse_event; >
On 02/25/10 15:15, Anthony Liguori wrote: > But I wonder if it really makes sense to treat all of these things > differently since we end up duplicating a lot of code. Would it make > more sense to just introduce: > typedef struct QEMUInputHandler { > void (*put_kbd_event)(QEMUInputHandler *obj, int keycode); > void (*put_led_event)(QEMUInputHandler *obj, int ledstate); > void (*put_mouse_event)(QEMUInputHandler *obj, int dx, int dy, int dz, > int buttons_state); > QLIST_ENTRY(QEMUInputHandler) node; > } QEMUInputHandler; > void qemu_add_input_handler(QEMUInputHandler *handler); > void qemu_remove_input_handler(QEMUInputHandler *handler); I don't think so. Devil is in the details. Note that kbd_event and mouse_event go to the kbd/mouse drivers, whereas led_event comes from the kbd driver, so they are different albeit related beasts. Also the mouse register/unregister does some extra care to update the pointer to the current mouse device, so we can't easily unify the list management. Juan's suggestion to use QLISTs makes alot of sense though, will do that. cheers, Gerd
On 02/25/2010 08:36 AM, Gerd Hoffmann wrote: > On 02/25/10 15:15, Anthony Liguori wrote: >> But I wonder if it really makes sense to treat all of these things >> differently since we end up duplicating a lot of code. Would it make >> more sense to just introduce: > >> typedef struct QEMUInputHandler { >> void (*put_kbd_event)(QEMUInputHandler *obj, int keycode); >> void (*put_led_event)(QEMUInputHandler *obj, int ledstate); >> void (*put_mouse_event)(QEMUInputHandler *obj, int dx, int dy, int dz, >> int buttons_state); >> QLIST_ENTRY(QEMUInputHandler) node; >> } QEMUInputHandler; > >> void qemu_add_input_handler(QEMUInputHandler *handler); >> void qemu_remove_input_handler(QEMUInputHandler *handler); > > I don't think so. Devil is in the details. Note that kbd_event and > mouse_event go to the kbd/mouse drivers, whereas led_event comes from > the kbd driver, so they are different albeit related beasts. Right, I hadn't thought it through enough. Should led events be part of DisplayChangeListener? I think you could make the argument that it's part of the display state. Regards, Anthony Liguori > Also the mouse register/unregister does some extra care to update the > pointer to the current mouse device, so we can't easily unify the list > management. > > Juan's suggestion to use QLISTs makes alot of sense though, will do that. > > cheers, > Gerd >
On 02/25/10 16:07, Anthony Liguori wrote: > On 02/25/2010 08:36 AM, Gerd Hoffmann wrote: >> I don't think so. Devil is in the details. Note that kbd_event and >> mouse_event go to the kbd/mouse drivers, whereas led_event comes from >> the kbd driver, so they are different albeit related beasts. > > Right, I hadn't thought it through enough. > > Should led events be part of DisplayChangeListener? I think you could > make the argument that it's part of the display state. From a hardware point of view display and keyboard are not related at all. The keyboard drivers don't have a DisplayState pointer at hand for that reason. So I don't think this makes sense, even though remote desktop protocols typically transport both display and keyboard data. cheers, Gerd
On 02/25/2010 10:51 AM, Gerd Hoffmann wrote: > On 02/25/10 16:07, Anthony Liguori wrote: >> On 02/25/2010 08:36 AM, Gerd Hoffmann wrote: >>> I don't think so. Devil is in the details. Note that kbd_event and >>> mouse_event go to the kbd/mouse drivers, whereas led_event comes from >>> the kbd driver, so they are different albeit related beasts. >> >> Right, I hadn't thought it through enough. >> >> Should led events be part of DisplayChangeListener? I think you could >> make the argument that it's part of the display state. > > From a hardware point of view display and keyboard are not related at > all. The keyboard drivers don't have a DisplayState pointer at hand > for that reason. So I don't think this makes sense, even though > remote desktop protocols typically transport both display and keyboard > data. Fair enough. Regards, Anthony Liguori > cheers, > Gerd >
diff --git a/console.h b/console.h index 916859d..0e969d1 100644 --- a/console.h +++ b/console.h @@ -10,10 +10,15 @@ #define MOUSE_EVENT_RBUTTON 0x02 #define MOUSE_EVENT_MBUTTON 0x04 +#define QEMU_SCROLL_LOCK_LED (1 << 0) +#define QEMU_NUM_LOCK_LED (1 << 1) +#define QEMU_CAPS_LOCK_LED (1 << 2) + /* in ms */ #define GUI_REFRESH_INTERVAL 30 typedef void QEMUPutKBDEvent(void *opaque, int keycode); +typedef void QEMUPutLEDEvent(void *opaque, int ledstate); typedef void QEMUPutMouseEvent(void *opaque, int dx, int dy, int dz, int buttons_state); typedef struct QEMUPutMouseEntry { @@ -26,13 +31,22 @@ typedef struct QEMUPutMouseEntry { struct QEMUPutMouseEntry *next; } QEMUPutMouseEntry; +typedef struct QEMUPutLEDEntry { + QEMUPutLEDEvent *put_led; + void *opaque; + struct QEMUPutLEDEntry *next; +} QEMUPutLEDEntry; + void qemu_add_kbd_event_handler(QEMUPutKBDEvent *func, void *opaque); QEMUPutMouseEntry *qemu_add_mouse_event_handler(QEMUPutMouseEvent *func, void *opaque, int absolute, const char *name); void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry); +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, void *opaque); +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); int kbd_mouse_is_absolute(void); diff --git a/input.c b/input.c index 955b9ab..82bc85c 100644 --- a/input.c +++ b/input.c @@ -31,6 +31,7 @@ static QEMUPutKBDEvent *qemu_put_kbd_event; static void *qemu_put_kbd_event_opaque; +static QEMUPutLEDEntry *qemu_put_led_event_head; static QEMUPutMouseEntry *qemu_put_mouse_event_head; static QEMUPutMouseEntry *qemu_put_mouse_event_current; @@ -102,6 +103,44 @@ void qemu_remove_mouse_event_handler(QEMUPutMouseEntry *entry) qemu_free(entry); } +QEMUPutLEDEntry *qemu_add_led_event_handler(QEMUPutLEDEvent *func, + void *opaque) +{ + QEMUPutLEDEntry *s; + + s = qemu_mallocz(sizeof(QEMUPutLEDEntry)); + + s->put_led = func; + s->opaque = opaque; + s->next = qemu_put_led_event_head; + qemu_put_led_event_head = s; + return s; +} + +void qemu_remove_led_event_handler(QEMUPutLEDEntry *entry) +{ + QEMUPutLEDEntry *prev = NULL, *cursor; + + if (!qemu_put_led_event_head || entry == NULL) + return; + + cursor = qemu_put_led_event_head; + while (cursor != NULL && cursor != entry) { + prev = cursor; + cursor = cursor->next; + } + + if (cursor == NULL) // does not exist or list empty + return; + + if (prev == NULL) { // entry is head + qemu_put_led_event_head = cursor->next; + } else { + prev->next = entry->next; + } + qemu_free(entry); +} + void kbd_put_keycode(int keycode) { if (qemu_put_kbd_event) { @@ -109,6 +148,17 @@ void kbd_put_keycode(int keycode) } } +void kbd_put_ledstate(int ledstate) +{ + QEMUPutLEDEntry *cursor; + + cursor = qemu_put_led_event_head; + while (cursor != NULL) { + cursor->put_led(cursor->opaque, ledstate); + cursor = cursor->next; + } +} + void kbd_mouse_event(int dx, int dy, int dz, int buttons_state) { QEMUPutMouseEvent *mouse_event;
Adds infrastructure for keyboard led status tracking to qemu. Signed-off-by: Gerd Hoffmann <kraxel@redhat.com> --- console.h | 14 ++++++++++++++ input.c | 50 ++++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 0 deletions(-)