X server testing

[Peter Hutterer]

Sorry about the slow reply, it got a bit busy in the last weeks.
I've verified the XI_Enter issue just now, will fix it ASAP.

On Mon, Jun 15, 2009 at 10:49:28PM -0400, Thomas Jaeger wrote:
> Here's another list of issues I've noticed over the past few weeks and
> questions that have come up.
> 
> * The server crashes (Xext/xtest.c, line 386) when a virtual XTest is
> detached due to a device grab and a client tries to send an event
> through that device.  Similarly, removing a master device whose XTest
> device is floated due to a grab will result in a crash.

The XTest device should never be floated. The only reason why these devices
exist that the provide a sane method for core XTest devices to send through
MDs.  I should proabbly put hooks in to disallow this.
Having said that, they shouldn't crash the server either :)

> * A related question is what the best way to detect virtual XTest
> devices is.  I'm currently checking whether the device name ends in
> "Xtst pointer", which seems slightly hackish, but should be fine as long
> as the servers keeps using a predictable name for the device.

I'm not quite sure why you need to detect virtual XTest devices? If you're
using XI anyway, leave them be, and if you're sending core events, then
you don't know they exist anyway. What's your use-case here?

> * The super modifier (Mod4Mask) isn't reported any longer in state field
> of core events and passive grabs containing this modifier in their
> modifier mask don't work anymore.

not sure about that yet, I blame daniel ;)
This could actually be a long-standing bug, there's a bug about syndaemon
ignoring Mod4 which is potentially the same.
 
> * My tablet's valuators (which should be absolute) are reported as
> having mode 3 (as opposed to XIModeAbsolute == 1 or XIModeRelative ==
> 0), which causes xinput to list them as relative devices.

I think this should be fixed with bc2ff5365030ad8bc11efde430b1064080dd7098.
Xi: copy the valuator mode from SD to MD.
Can you confirm that?
 
> * set_valuators (in dix/getevents.c) seems fishy to me, should this be
> i instead of first_valuator + i?

yes, that's broken. fixed, thanks.

> * xinput will crash if remove-master is called with returnMode
> AttachToMaster but no further argument.  I've attached two patches that
> will return the pointer/keyboard back to the VCP/VCK in this case.  The
> first patch assumes that the first pointer/keyboard master device is the
> VCP/VCK, the second - slightly simpler - patch identifies VCP and VCK by
> name -- I couldn't decide which one is better.

see comments below.
 
> * Since people are likely to turn to xinput for example code, I've also
> attached a patch replacing the custom BitIsOn/SetBit macros by the
> standard XIMaskIsSet/XISetMask macros.
> * xinput's test-xi2 reports incorrect event coordinates.  Patch attached

both pushed, thanks.
 
> * Proximity events aren't reported as XI2 events at this points.  This
> is not a big deal to me, I'm basically just making sure that you're
> aware of this.

there are no proximity XI2 events. If a device can do proximity, it should
report a proximity axis, even if that axis only has a range of [0, 1].
And no, no driver supports that yet :)

> * Subpixel coordinates are not working for me.  Glancing through the
> code, I think this is to be expected for devices like my tablet whose
> device resolution is higher than the screen resolution, but it seems
> that this should work for accelerated devices like my trackpoint.  Once
> again, not really a big deal.

yes, those bits are missing. sorry. subpixels right now are only for accel
code. feel free to send me a patch for this though, rescaleValuatorAxis
should be it.


> (gdb) bt
> #0  0x0086b422 in __kernel_vsyscall ()
> #1  0x002d46d0 in raise () from /lib/tls/i686/cmov/libc.so.6
> #2  0x002d6098 in abort () from /lib/tls/i686/cmov/libc.so.6
> #3  0x0031224d in ?? () from /lib/tls/i686/cmov/libc.so.6
> #4  0x00318604 in ?? () from /lib/tls/i686/cmov/libc.so.6
> #5  0x0031a5b6 in free () from /lib/tls/i686/cmov/libc.so.6
> #6  0x080a7a61 in Xfree (ptr=0x0) at ../../os/utils.c:1159
> #7  0x08129420 in ProcessOtherEvent (ev=0x944e298, device=0xbec2d38) at ../../Xi/exevents.c:953
> #8  0x08151ec8 in ProcessPointerEvent (ev=0x944e298, mouse=0xbec2d38) at ../../xkb/xkbAccessX.c:732
> #9  0x0809fb05 in mieqProcessDeviceEvent (dev=0xbec2d38, event=0x944e298, screen=0x0) at ../../mi/mieq.c:403
> #10 0x081056b8 in ProcXTestFakeInput (client=<value optimized out>) at ../../Xext/xtest.c:418
> #11 0x0806cf87 in Dispatch () at ../../dix/dispatch.c:426
> #12 0x080674b5 in main (argc=10, argv=0xbff73ac4, envp=0xbff73af0) at ../../dix/main.c:283
> (gdb) print ev->any.type == ET_Raw
> $11 = 1
> (gdb)

Xfree shouldn't crash on NULL pointers, we might actually have some memory
corruption happening. Does valgrind complain about anything?


> From 0f46b8a25fba1001b47850763d5de92162bce367 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Sun, 14 Jun 2009 13:58:39 -0400
> Subject: [PATCH] reattach: Default to return to VCP/VCK when returnMode is AttachToMaster
> 
> Signed-off-by: Thomas Jaeger <ThJaeger at gmail.com>
> ---
>  src/hierarchy.c |   39 +++++++++++++++++++++++++++++++++++++--
>  src/xinput.c    |    2 +-
>  2 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/src/hierarchy.c b/src/hierarchy.c
> index cf0f783..18c6710 100644
> --- a/src/hierarchy.c
> +++ b/src/hierarchy.c
> @@ -96,8 +96,43 @@ remove_master(Display* dpy, int argc, char** argv, char *name, char *desc)
>  
>      if (r.return_mode == XIAttachToMaster)
>      {
> -        r.return_pointer = atoi(argv[2]);
> -        r.return_keyboard = atoi(argv[3]);
> +	if (argc >= 3) {
> +	    info = xi2_find_device_info(dpy, argv[2]);
> +	    if (!info) {
> +		fprintf(stderr, "unable to find device %s\n", argv[2]);
> +		return EXIT_FAILURE;
> +	    }
> +
> +	    r.return_pointer = info->deviceid;
> +	} else {
> +	    int i, ndevices;
> +	    info = XIQueryDevice(dpy, XIAllDevices, &ndevices);
> +	    for(i = 0; i < ndevices; i++)
> +		if (info[i].use == XIMasterPointer) {
> +		    r.return_pointer = info[i].deviceid;
> +		    break;
> +		}

urgh. { } for the for loop please. 

> +	    XIFreeDeviceInfo(info);
> +	}
> +
> +	if (argc >= 4) {
> +	    info = xi2_find_device_info(dpy, argv[3]);
> +	    if (!info) {
> +		fprintf(stderr, "unable to find device %s\n", argv[3]);
> +		return EXIT_FAILURE;
> +	    }
> +
> +	    r.return_keyboard = info->deviceid;
> +	} else {
> +	    int i, ndevices;
> +	    info = XIQueryDevice(dpy, XIAllDevices, &ndevices);
> +	    for(i = 0; i < ndevices; i++)
> +		if (info[i].use == XIMasterKeyboard) {
> +		    r.return_keyboard = info[i].deviceid;
> +		    break;
> +		}

same here, {} for the block of the for loop please
also, please indent with spaces only.

> +	    XIFreeDeviceInfo(info);
> +	}

>      }
>  

the approach seems a bit ineffective. Swap XIAllDevices for
XIAllMasterDevices and if you check the arguments before, you only need one
XIQueryDevice call.

thanks for the two test-xi2 patches, merged and pushed.

Cheers,
  Peter