From mboxrd@z Thu Jan  1 00:00:00 1970
Received: from mailman by lists.gnu.org with tmda-scanned (Exim 4.43)
	id 1NvESq-0001CI-DB
	for qemu-devel@nongnu.org; Fri, 26 Mar 2010 14:41:00 -0400
Received: from [140.186.70.92] (port=38017 helo=eggs.gnu.org)
	by lists.gnu.org with esmtp (Exim 4.43) id 1NvESn-00019f-QZ
	for qemu-devel@nongnu.org; Fri, 26 Mar 2010 14:40:59 -0400
Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.69)
	(envelope-from <shaharh@redhat.com>) id 1NvESi-0004EA-KV
	for qemu-devel@nongnu.org; Fri, 26 Mar 2010 14:40:54 -0400
Received: from mx1.redhat.com ([209.132.183.28]:56470)
	by eggs.gnu.org with esmtp (Exim 4.69)
	(envelope-from <shaharh@redhat.com>) id 1NvESi-0004DJ-9S
	for qemu-devel@nongnu.org; Fri, 26 Mar 2010 14:40:52 -0400
Received: from int-mx04.intmail.prod.int.phx2.redhat.com
	(int-mx04.intmail.prod.int.phx2.redhat.com [10.5.11.17])
	by mx1.redhat.com (8.13.8/8.13.8) with ESMTP id o2QIemqZ018658
	(version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK)
	for <qemu-devel@nongnu.org>; Fri, 26 Mar 2010 14:40:48 -0400
Date: Fri, 26 Mar 2010 21:40:31 +0300
From: Shahar Havivi <shaharh@redhat.com>
Subject: Re: [Qemu-devel] [PATCH 2/2] Added monitor commands: 'keyboard_set'
	and 'info keybaord'
Message-ID: <20100326184030.GA25061@redhat.com>
References: <cover.1269373411.git.shaharh@redhat.com>
	<24a0619c9d6c07c5b10f5aff19478ff3040c4af8.1269373411.git.shaharh@redhat.com>
	<m3mxxv2ymx.fsf@blackfin.pond.sub.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <m3mxxv2ymx.fsf@blackfin.pond.sub.org>
List-Id: qemu-devel.nongnu.org
List-Unsubscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=unsubscribe>
List-Archive: <http://lists.gnu.org/pipermail/qemu-devel>
List-Post: <mailto:qemu-devel@nongnu.org>
List-Help: <mailto:qemu-devel-request@nongnu.org?subject=help>
List-Subscribe: <http://lists.nongnu.org/mailman/listinfo/qemu-devel>,
	<mailto:qemu-devel-request@nongnu.org?subject=subscribe>
To: Markus Armbruster <armbru@redhat.com>
Cc: qemu-devel@nongnu.org

On Fri, Mar 26, 2010 at 10:57:58AM +0100, Markus Armbruster wrote:
> Date: Fri, 26 Mar 2010 10:57:58 +0100
> From: Markus Armbruster <armbru@redhat.com>
> To: Shahar Havivi <shaharh@redhat.com>
> Cc: qemu-devel@nongnu.org
> Subject: Re: [Qemu-devel] [PATCH 2/2] Added monitor commands:
>  'keyboard_set' and 'info keybaord'
> 
> Shahar Havivi <shaharh@redhat.com> writes:
> 
> > Two new monitor commands: adding ability to handle which keyboard qemu will
> > use and to see which keyboard are currently available.
> >
> > $ info keyboard
> > $ keyboard_set <index>
> >
> > Signed-off-by: Shahar Havivi <shaharh@redhat.com>
> > ---
> >  console.h       |    4 ++
> >  input.c         |  104 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  monitor.c       |    8 ++++
> >  qemu-monitor.hx |   17 +++++++++
> >  qerror.c        |    8 ++++
> >  5 files changed, 141 insertions(+), 0 deletions(-)
> >
> > diff --git a/console.h b/console.h
> > index 16c9c3d..7efa88e 100644
> > --- a/console.h
> > +++ b/console.h
> > @@ -85,6 +85,10 @@ void do_info_mice_print(Monitor *mon, const QObject *data);
> >  void do_info_mice(Monitor *mon, QObject **ret_data);
> >  void do_mouse_set(Monitor *mon, const QDict *qdict);
> >  
> > +void do_info_keyboard_print(Monitor *mon, const QObject *data);
> > +void do_info_keyboard(Monitor *mon, QObject **ret_data);
> > +int do_keyboard_set(Monitor *mon, const QDict *qdict, QObject **ret_data);
> > +
> >  /* keysym is a unicode code except for special keys (see QEMU_KEY_xxx
> >     constants) */
> >  #define QEMU_KEY_ESC1(c) ((c) | 0xe100)
> > diff --git a/input.c b/input.c
> > index 563ecad..4daaeb0 100644
> > --- a/input.c
> > +++ b/input.c
> > @@ -368,3 +368,107 @@ void qemu_remove_mouse_mode_change_notifier(Notifier *notify)
> >  {
> >      notifier_list_remove(&mouse_mode_notifiers, notify);
> >  }
> > +
> > +static void info_keyboard_iter(QObject *data, void *opaque)
> > +{
> > +    QDict *kbd;
> > +    Monitor *mon = opaque;
> > +
> > +    kbd = qobject_to_qdict(data);
> > +    monitor_printf(mon, "%c Keyboard #%" PRId64 ": %s\n",
> > +                  (qdict_get_bool(kbd, "current") ? '*' : ' '),
> > +                  qdict_get_int(kbd, "index"), qdict_get_str(kbd, "name"));
> > +}
> > +
> > +void do_info_keyboard_print(Monitor *mon, const QObject *data)
> > +{
> > +    QList *kbd_list;
> > +
> > +    kbd_list = qobject_to_qlist(data);
> > +    if (qlist_empty(kbd_list)) {
> > +        monitor_printf(mon, "No keyboard devices connected\n");
> > +        return;
> > +    }
> > +
> > +    qlist_iter(kbd_list, info_keyboard_iter, mon);
> > +}
> > +
> > +/*
> > + * do_info_keyboard(): Show VM keyboard information
> > + *
> > + * Each keyboard is represented by a QDict, the returned QObject is
> > + * a QList of all keyboards.
> > + *
> > + * The keyboard QDict contains the following:
> > + *
> > + * - "name": keyboard's name
> > + * - "index": keyboard's index
> > + * - "current": true if this keyboard is receiving events, false otherwise
> > + *
> > + * Example:
> > + *
> > + * [ { "name": "QEMU USB Keyboard", "index": 0, "current": false },
> > + *   { "name": "QEMU PS/2 Keyboard", "index": 1, "current": true } ]
> > + */
> > +void do_info_keyboard(Monitor *mon, QObject **ret_data)
> > +{
> > +    QEMUPutKbdEntry *cursor;
> > +    QList *kbd_list;
> > +    int index = 0;
> > +
> > +    kbd_list = qlist_new();
> > +
> > +    if (!qemu_put_kbd_event_head) {
> > +        goto out;
> > +    }
> > +
> > +    cursor = qemu_put_kbd_event_head;
> > +    while (cursor != NULL) {
> > +        QObject *obj;
> > +        obj = qobject_from_jsonf("{ 'name': %s, 'index': %d, 'current': %i }",
> > +                                 cursor->qemu_put_kbd_name,
> > +                                 index, cursor == qemu_put_kbd_event_current);
> > +        qlist_append_obj(kbd_list, obj);
> > +        index++;
> > +        cursor = cursor->next;
> > +    }
> > +out:
> > +    *ret_data = QOBJECT(kbd_list);
> > +}
> 
> Keyboard indexes change when keyboards other than the last one get
> removed.  Hmm.
> 
> > +
> > +/*
> > + * do_keyboard_set(): Set active keyboard
> > + *
> > + * Argument qdict contains
> > + * - "index": the keyboard index to set
> > + *
> > + * Example:
> > + *
> > + * { "index": "0" }
> > + */
> > +int do_keyboard_set(Monitor *mon, const QDict *qdict, QObject **ret_data)
> > +{
> > +    QEMUPutKbdEntry *cursor;
> > +    int i = 0;
> > +    int index = qdict_get_int(qdict, "index");
> > +
> > +    if (!qemu_put_kbd_event_head) {
> > +        qerror_report(QERR_DEVICE_NOT_FOUND, "keyboard");
> > +        return -1;
> > +    }
> > +
> > +    cursor = qemu_put_kbd_event_head;
> > +    while (cursor != NULL && index != i) {
> > +        i++;
> > +        cursor = cursor->next;
> > +    }
> > +
> > +    if (cursor != NULL) {
> > +        qemu_put_kbd_event_current = cursor;
> > +    }
> > +    else {
> > +        qerror_report(QERR_INVALID_PARAMETER, "index");
> > +        return -1;
> > +    }
> > +    return 0;
> > +}
> 
> Since the index is not a stable identifier of keyboards, and we support
> multiple monitors, you're prone to remove the wrong keyboard.
> 
>     Monitor#1                           Monitor#2
>     "info keyboard" to find the index
>                                         unplug a keyboard, invalidating
>                                         index
>     keyboard_set <index>
> 
> I think it's best to use a stable ID, like we do in other places.  We
> commonly let the user specify it, e.g. as id=ID in -netdev, -device and
> elswhere.
What do we do when user not specify id for device?
> 
> > diff --git a/monitor.c b/monitor.c
> > index 0448a70..cc95b3d 100644
> > --- a/monitor.c
> > +++ b/monitor.c
> > @@ -2783,6 +2783,14 @@ static const mon_cmd_t info_cmds[] = {
> >          .mhandler.info_new = do_info_mice,
> >      },
> >      {
> > +        .name       = "keyboard",
> > +        .args_type  = "",
> > +        .params     = "",
> > +        .help       = "show which guest keyboard is receiving events",
> > +        .user_print = do_info_keyboard_print,
> > +        .mhandler.info_new = do_info_keyboard,
> > +    },
> > +    {
> >          .name       = "vnc",
> >          .args_type  = "",
> >          .params     = "",
> > diff --git a/qemu-monitor.hx b/qemu-monitor.hx
> > index 5308f36..e9beb12 100644
> > --- a/qemu-monitor.hx
> > +++ b/qemu-monitor.hx
> > @@ -659,6 +659,23 @@ info mice
> >  @end example
> >  ETEXI
> >  
> > +    {
> > +        .name       = "keyboard_set",
> > +        .args_type  = "index:i",
> > +        .params     = "index",
> > +        .help       = "set which keyboard device receives events",
> > +        .mhandler.cmd_new = do_keyboard_set,
> > +    },
> > +
> > +STEXI
> > +@item keyboard_set @var{index}
> > +@findex keyboard_set
> > +Set which keyboard device receives events at given @var{index}, index
> > +can be obtained with
> > +@example
> > +info keyboard
> > +@end example
> > +ETEXI
> >  #ifdef HAS_AUDIO
> >      {
> >          .name       = "wavcapture",
> > diff --git a/qerror.c b/qerror.c
> > index d0aba61..42a5cf7 100644
> > --- a/qerror.c
> > +++ b/qerror.c
> > @@ -172,6 +172,14 @@ static const QErrorStringTable qerror_table[] = {
> >          .error_fmt = QERR_VNC_SERVER_FAILED,
> >          .desc      = "Could not start VNC server on %(target)",
> >      },
> > +    {
> > +        .error_fmt = QERR_DEVICE_NOT_FOUND,
> > +        .desc      = "No keyboard device found",
> > +    },
> > +    {
> > +        .error_fmt = QERR_INVALID_PARAMETER,
> > +        .desc      = "Invalid index '%(name)' for keyboard device",
> > +    },
> >      {}
> >  };
> 
> I'm afraid you're breaking all other uses of these errors here.
you right, my bad - this errors where not meant to be in this patch.