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

[Romain Naour <romain.naour@openwide.fr>]

[-- Attachment #1: Type: text/plain, Size: 2882 bytes --]

Hi,

[snip]
>> +    int dx;
>> +    int dy;
>> +    int button;
>> +    struct QEMUTimer *transmit_timer; /* QEMU timer */
>> +    uint64_t transmit_time;           /* time to transmit a char in ticks */
>> +    unsigned char data[5];
>> +    int index;
>> +} gnmouse_save;
> How does all this state get migrated at VM migration? I know
> how this works for device models, but does anybody know
> the answer for char backends?
I never used the VM migration, I don't know.

[snip]
>> +    switch (save->index) {
>> +    case CHAR_4:
>> +        if (save->dx && save->dy) {
> This looks wrong. If there's a saved delta-x then we
> still want to send it to the guest, even if the save->dy is
> correct, right?
Yes indeed, thanks.

[snip]
> Also, it could use a comment. If I understand the logic correctly,
> something like:
>   /* Send as much of our accumulated delta as we can fit into
>    * the packet format. Anything remaining will get sent in a
>    * subsequent packet.
>    */
These additional bytes are intended to send the movement made since the 
beginning of theframe transfer.

[snip]
> + /* reload timer */
> This comment is stating the obvious. It would be more interesting
> to know why we have a timer at all...
If I do like msmouse, the receive buffer is flooded by each GUI event 
and the last frame is not entirely received.
I hav'nt found a better solution than using a timer...
I added a comment for that in v3.

[snip]
>> +
>> +    /* Buttons */
>> +    BP |= (buttons_state & 0x01 ? 0x00 : 0x04); /* BP1 = L */
>> +    BP |= (buttons_state & 0x02 ? 0x00 : 0x01); /* BP2 = R */
>> +    BP |= (buttons_state & 0x04 ? 0x00 : 0x02); /* BP4 = M */
> Are these really 'bit set if button is up' ? If so that could use a comment,
> because that's a bit of a weird protocol design.
Yes it's really "bit set if button is up".

>>   @item -chardev msmouse ,id=@var{id}
>>
>> -Forward QEMU's emulated msmouse events to the guest. @option{msmouse} does not
>> -take any options.
>> +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.
> If we have more than one supported protocol, we should probably say in the
> documentation which one we recommend for which purposes, rather than
> leaving people to guess which one might be better supported/more efficient/etc.
> My guess is that the answer is "use msmouse unless your guest OS only
> supports the Mouse Systems protocol", but your patch doesn't include any
> rationale, so that is just a guess...
I'm agree with you, I added a comment in the documentation to says that.

Thank you for your time and review !

Best regards,
Romain Naour

[-- Attachment #2: Type: text/html, Size: 6266 bytes --]