| Message ID | 1415119766-9745-1-git-send-email-akong@redhat.com |
|---|---|
| State | New |
| Headers | show |
On Mi, 2014-11-05 at 00:49 +0800, Amos Kong wrote: > qemu_input_find_handler() prefers a handler associated with con. > But if none exists, it takes any. This patch added a parameter > to strictly check console, in case we want to input event to > special console. > > 'input-send-event' has a parameter to assign special console, > so we should enable strict checking in finding handler. I don't think we want do that by default. It only matters in case of a multiseat setup where you actually have multiple input devices of the same kind. Which isn't a very typical use case. Options I see are: (a) Turn console into an optional parameter, do strict checking in case it is present. (b) Add a optional 'strict' parameter. cheers, Gerd
On Wed, Nov 05, 2014 at 09:47:47AM +0100, Gerd Hoffmann wrote: > On Mi, 2014-11-05 at 00:49 +0800, Amos Kong wrote: > > qemu_input_find_handler() prefers a handler associated with con. > > But if none exists, it takes any. This patch added a parameter > > to strictly check console, in case we want to input event to > > special console. If console is assigned, it will try to find right handler by first loop in qemu_input_find_handler(). The second loop is used to find mask matched handler if console isn't assigned. If we assigned console and didn't find handler in first loop, it skip second loop body by 'continue', and return NULL. It seems my concern is wrong, we don't need this repeated parameter. NACK this patch. Thanks. > > 'input-send-event' has a parameter to assign special console, > > so we should enable strict checking in finding handler. > > I don't think we want do that by default. It only matters in case of a > multiseat setup where you actually have multiple input devices of the > same kind. Which isn't a very typical use case. > > Options I see are: > > (a) Turn console into an optional parameter, do strict checking in > case it is present. > (b) Add a optional 'strict' parameter.
Gerd Hoffmann <kraxel@redhat.com> writes: > On Mi, 2014-11-05 at 00:49 +0800, Amos Kong wrote: >> qemu_input_find_handler() prefers a handler associated with con. >> But if none exists, it takes any. This patch added a parameter >> to strictly check console, in case we want to input event to >> special console. >> >> 'input-send-event' has a parameter to assign special console, >> so we should enable strict checking in finding handler. > > I don't think we want do that by default. It only matters in case of a > multiseat setup where you actually have multiple input devices of the > same kind. Which isn't a very typical use case. > > Options I see are: > > (a) Turn console into an optional parameter, do strict checking in > case it is present. > (b) Add a optional 'strict' parameter. Current behavior (please correct misunderstandings): The guest must be running. input-send-event parameter 'console' is mandatory. The console identified by its value must exist. If this console can accept the event, send it there. Else, a console that can accept the event must exist. Send it to one of them. Which one exactly isn't specified. Behavior with (a): The guest must be running. input-send-event parameter 'console' is optional. If it's present, the console identified by its value must exist, and must be able to accept the event. Send it there. Else, a console that can accept the event must exist. Send it to one of them. Which one exactly isn't specified. "Must" means "or else command fails". I think that's a clear improvement. It's actually what I expected from the command documentation, until I read the code.
On Thu, Nov 06, 2014 at 02:37:54PM +0800, Amos Kong wrote: > On Wed, Nov 05, 2014 at 09:47:47AM +0100, Gerd Hoffmann wrote: > > On Mi, 2014-11-05 at 00:49 +0800, Amos Kong wrote: > > > qemu_input_find_handler() prefers a handler associated with con. > > > But if none exists, it takes any. This patch added a parameter > > > to strictly check console, in case we want to input event to > > > special console. > > If console is assigned, it will try to find right handler by first > loop in qemu_input_find_handler(). The second loop is used to find > mask matched handler if console isn't assigned. > > If we assigned console and didn't find handler in first loop, it > skip second loop body by 'continue', and return NULL. > It seems my concern is wrong, we don't need this repeated parameter. I was wrong, if we don't assign the console for qemu_input_find_handler(), it has chance to get an arbitrary console (mask matched). So the original issue this patch try to fix truely exists. > NACK this patch. > > Thanks. > > > > 'input-send-event' has a parameter to assign special console, > > > so we should enable strict checking in finding handler. > > > > I don't think we want do that by default. It only matters in case of a > > multiseat setup where you actually have multiple input devices of the > > same kind. Which isn't a very typical use case. > > > > Options I see are: > > > > (a) Turn console into an optional parameter, do strict checking in > > case it is present. > > (b) Add a optional 'strict' parameter. > > -- > Amos. From Markus: > > Current behavior (please correct misunderstandings): > > The guest must be running. > input-send-event parameter 'console' is mandatory. > The console identified by its value must exist. > If this console can accept the event, send it there. > Else, a console that can accept the event must exist. Send it > to > one of them. Which one exactly isn't specified. > > Behavior with (a): > > The guest must be running. > input-send-event parameter 'console' is optional. > If it's present, the console identified by its value must exist, > and > must be able to accept the event. Send it there. > Else, a console that can accept the event must exist. Send it > to > one of them. Which one exactly isn't specified. > > "Must" means "or else command fails". > > I think that's a clear improvement. It's actually what I expected > from > the command documentation, until I read the code. Thanks for your clear description, I now agree with Gerd's option (a), (a) is better than (b). So we need to change QMP to support optional console parameter and change qemu_input_find_handler() to support strict checking (as my patch). Gerd, I'd like to work on both of them, if you already work on it, please let me know, thanks.
On Thu, Nov 06, 2014 at 11:01:11PM +0800, Amos Kong wrote: > On Thu, Nov 06, 2014 at 02:37:54PM +0800, Amos Kong wrote: > > On Wed, Nov 05, 2014 at 09:47:47AM +0100, Gerd Hoffmann wrote: > > > On Mi, 2014-11-05 at 00:49 +0800, Amos Kong wrote: > > > > qemu_input_find_handler() prefers a handler associated with con. > > > > But if none exists, it takes any. This patch added a parameter > > > > to strictly check console, in case we want to input event to > > > > special console. > > > > If console is assigned, it will try to find right handler by first > > loop in qemu_input_find_handler(). The second loop is used to find > > mask matched handler if console isn't assigned. > > > > If we assigned console and didn't find handler in first loop, it > > skip second loop body by 'continue', and return NULL. > > It seems my concern is wrong, we don't need this repeated parameter. > > I was wrong, if we don't assign the console for qemu_input_find_handler(), > it has chance to get an arbitrary console (mask matched). So the > original issue this patch try to fix truely exists. The 'console' in my mind was the input destination, so I thgouth we need to distinguish the console with strictly checking. The 'QemuConsole' is the input source for handler, we share some input handlers to process the input events from different QemuConsole. Normally we only have one set of keyboard, mouse, usbtablet, etc. The devices have different mask, it's fine to just checking mask to insure that the handler has the ability to process the event. I saw we try to bind console to handler in usb/dev-hid.c, but display always isn't available at that time. If we have multiseat setup (as Gerd said), we only have 'problem' in this case. Actually event from different devices have the same effect for system, it's fine to always use the first available handler without caring about the console. For send-key command, we just pass a NULL for console parameter in calling qemu_input_event_send_key(NULL, ..) Conclusion: * Generally assigning the special console is meanless, and we can't directly remove the QMP parameter for compatibility. So we can make the parameter optional. The parameter might be useful for some special condition: we have multiple devices without binding console and they all have the ability(mask) to process events, and we don't want to use the first one. Thanks. Amos
diff --git a/ui/input.c b/ui/input.c index 002831e..61b26a0 100644 --- a/ui/input.c +++ b/ui/input.c @@ -98,7 +98,7 @@ void qemu_input_handler_bind(QemuInputHandlerState *s, } static QemuInputHandlerState* -qemu_input_find_handler(uint32_t mask, QemuConsole *con) +qemu_input_find_handler(uint32_t mask, QemuConsole *con, bool strict_con) { QemuInputHandlerState *s; @@ -111,6 +111,10 @@ qemu_input_find_handler(uint32_t mask, QemuConsole *con) } } + if (strict_con) { + return NULL; + } + QTAILQ_FOREACH(s, &handlers, node) { if (s->con != NULL) { continue; @@ -142,7 +146,7 @@ void qmp_input_send_event(int64_t console, InputEventList *events, for (e = events; e != NULL; e = e->next) { InputEvent *event = e->value; - if (!qemu_input_find_handler(1 << event->kind, con)) { + if (!qemu_input_find_handler(1 << event->kind, con, true)) { error_setg(errp, "Input handler not found for " "event type %s", InputEventKind_lookup[event->kind]); @@ -311,7 +315,7 @@ void qemu_input_event_send(QemuConsole *src, InputEvent *evt) } /* send event */ - s = qemu_input_find_handler(1 << evt->kind, src); + s = qemu_input_find_handler(1 << evt->kind, src, NULL); if (!s) { return; } @@ -428,7 +432,7 @@ bool qemu_input_is_absolute(void) QemuInputHandlerState *s; s = qemu_input_find_handler(INPUT_EVENT_MASK_REL | INPUT_EVENT_MASK_ABS, - NULL); + NULL, NULL); return (s != NULL) && (s->handler->mask & INPUT_EVENT_MASK_ABS); }
qemu_input_find_handler() prefers a handler associated with con. But if none exists, it takes any. This patch added a parameter to strictly check console, in case we want to input event to special console. 'input-send-event' has a parameter to assign special console, so we should enable strict checking in finding handler. Signed-off-by: Amos Kong <akong@redhat.com> --- ui/input.c | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-)