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

[Romain Naour]

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 the frame 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 ,address@hidden

-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.
+
address@hidden -chardev gnmouse ,address@hidden
+
+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