Re: [Qemu-devel] [PATCH 1/1] genius: add genius serial mouse emulation

[Paolo Bonzini <pbonzini@redhat.com>]

Il 14/01/2014 00:47, Romain Naour ha scritto:
> Hi,
> 
> Thanks for review.
> 
> Le 13/01/2014 23:45, Eric Blake a écrit :
>> On 01/13/2014 03:33 PM, Romain Naour wrote:
>>> This patch adds the emulation for a serial Genius mouse using
>>> Mouse Systems protocol (5bytes).
>>> This protocol is compatible with most 3-button serial mouse.
>>>
>>> Signed-off-by: Romain Naour <romain.naour@openwide.fr>
>>> ---
>>>   backends/Makefile.objs |   2 +-
>>>   backends/gnmouse.c     | 339
>>> +++++++++++++++++++++++++++++++++++++++++++++++++
>>>   include/sysemu/char.h  |   3 +
>>>   qapi-schema.json       |   1 +
>>>   qemu-char.c            |   4 +
>>>   qemu-options.hx        |  10 ++
>>>   6 files changed, 358 insertions(+), 1 deletion(-)
>>>   create mode 100644 backends/gnmouse.c
>>> +++ b/backends/gnmouse.c
>>> @@ -0,0 +1,339 @@
>>> +/*
>>> + * QEMU Genius GM-6 serial mouse emulation
>>> + *
>>> + * Adapted from msmouse
>>> + *
>>> + * Copyright (c) 2012 Romain Naour
>> It is now 2014.
> Fixed
>>
>>
>>> +++ b/qapi-schema.json
>>> @@ -3617,6 +3617,7 @@
>>>                                          'null'   : 'ChardevDummy',
>>>                                          'mux'    : 'ChardevMux',
>>>                                          'msmouse': 'ChardevDummy',
>>> +                                       'gnmouse': 'ChardevDummy',
>>>                                          'braille': 'ChardevDummy',
>>>                                          'stdio'  : 'ChardevStdio',
>>>                                          'console': 'ChardevDummy',
>> Ideally, you should also document that this branch was not always
>> available in the union (I was expecting to see a doc line with '(since
>> 2.0)' somewhere in the patch); but looking at the existing schema,
>> you're just copying bad practice of adding to an already
>> under-documented union.
>>
> 
> You mean something like that ?
> ##
> # @ChardevBackendKind:
> #
> # A union referencing different chardev backend configuration' info.
> #
> # Since: 1.4
> #
> # gnmouse backend since: 2.0
> ##
> { 'union': 'ChardevBackend', 'data': { 'file'   : 'ChardevFile',
> [snip]
> 

The standard is more like

# A union referencing different chardev backend configuration' info.
#
# @gnmouse: Send mouse data using the Genius protocol (since 2.0).
#
# Since: 1.4
##

but that format would require doucmenting all items. :(

You can leave this out of v2.

However, I'd be happy if you changed the documentation like this

@item -chardev msmouse,id=@var{id}
Forward events from QEMU's emulated mouse to the guest using the
Microsoft protocol. @option{msmouse} does not take any options.

@item -chardev gnmouse ,id=@var{id}

Forward events from QEMU's emulated mouse to the guest using the Genius
(Mouse Systems) protocol. @option{gnmouse} does not take any options.