[Qemu-devel] [RESEND] [PATCH] Support for serial mouse emulation

[Lubomir Rintel]

Hi,

On Wed Dec 24 19:41:53 CET 2008, Aurelien Jarno wrote:
> Please find my comments inline.

Thanks a lot for your review. I am attaching the revised version,
addressing most of your concerns.

> This should be replaced by a Signed-off-by: line.

Done

>> Index: Makefile.target
>> ===================================================================
...
>> -OBJS+= ide.o pckbd.o ps2.o vga.o $(SOUND_HW) dma.o
>> +OBJS+= ide.o pckbd.o ps2.o msmouse.o vga.o $(SOUND_HW) dma.o
>
... (Lots of msmouse.o additions) ...
>
> Is it really necessary to enable that in all targets, especially when
> this is inited only in hw/pc.c?

Well, given it's a character device, it's generally possible to use
everywhere a serial port can be used. I limited this to a single occurence
of adding to OBJS in the new version of patch though.

>> +CharDriverState *msmouse_chr = NULL;
>
> A global variable is not a good idea. You should instead create
> structure containing this variable (and maybe others like
> CharDriverState) called for example VMMouseState.

Originally I created a global variable because the serial mouse did not
work for me when attached at the serial port initialization time. Now I
figured out multiple mouse can be attached at once, though only one can be
selected at the time from the console. I am wondering if it would make
sense to somehow make the serial mouse have preference over the PS/2 mouse
by default, since it's most likely the way one would expect it to work?

I did not create the structure with the state yet, since the only data I
keep for the instance of serial mouse is its serial port character device.
It's also consistent with original two-button mouse hardware using MS
protocol, since it was basically stateless.

On the other hand, three button mouses keep state about the past state of
the third button, and use the two-button compatible protocol if the third
button is depressed and its state did not change, to retain compatibility
with two-button drivers, which is what this emulation doesn't support.
(See the comment around the relevant part of the code.)

I'll create a separate structure for serial mice and support this
backward-compatible behavior if you want me to.

>> +    /* Only one serial mouse per instance is allowed */

> I don't think it is a good idea. I guess this is due to the use of a
> global variable.

Right. Fixed.

... (the character device methods)...

> I really think all this part that you add in vl.c should be moved into
> hw/vmmouse.c. The qemu_chr_open_msmouse should be merged into
> msmouse_init and called from vl.c. Then the structure should be
> allocated, and passed instead of the NULL argument of
> qemu_add_mouse_event_handler().

I think you meant "qemu-char.c", it hat split from "vl.c". And I guess it
shouldn't be moved to "vmmouse.c", since that's a VMWare mouse, but it
could have been a typo as well. If you meant "msmouse.c", then that was
done in the new revision of the patch.

I'm not completely sure it is right though -- see, qemu-char.c contains a
lot of similar examples of complete implementation of character devices. 
I find separation to msmouse.c a lot cleaner tough.

>> +    /* Buttons */
>> +    bytes[0] |= (buttons_state & 0x01 ? 0x20 : 0x00);
>> +    bytes[0] |= (buttons_state & 0x02 ? 0x10 : 0x00);
>> +    bytes[3] |= (buttons_state & 0x04 ? 0x20 : 0x00);
>
> There is probably a copy&paste error in the indices here.

It's actually correct. It is just that the serial mouse protocol evolved
into being a little bit weird.

-- 
Lubomir Rintel <address@hidden>

qemu-20081224-msmouse.patch
Description: Text Data