[PATCHES] axisVal issues
Peter Hutterer
peter.hutterer at who-t.net
Wed Sep 23 19:43:48 PDT 2009
On Wed, Sep 23, 2009 at 01:43:55PM -0400, Thomas Jaeger wrote:
> As noted in the gtk bug [1], XGetDeviceState returns incorrect valuator
> information. I tracked this down to axisVal containing garbage. Rather
> than figure out where the problem is, I think it's easier to get rid of
> axisVal altogether and use dev->last.valuators instead.
not really. axisVal is the axis state that is sent to the client through the
events. Anything in dev->last is the state inside the server as used during
the signal handler. There's no guarantee that last.valuators is still on the
correct value when the server processes the event and even worse - being
used in the signal handler you can't even guarantee it's correct while
you're copying it out.
So for this case we actually need to find the bug and fix it.
Cheers,
Peter
> [1] https://bugzilla.gnome.org/show_bug.cgi?id=588649
> From a1219f5cd34b8df4c6862caf3d783d0ef45beda1 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Wed, 23 Sep 2009 13:01:35 -0400
> Subject: [PATCH 1/2] Xi: Don't rely on axisVal for QueryState
>
> axisVal may contain garbage, so this lead to incorrect behavior
> ---
> Xi/queryst.c | 5 ++---
> 1 files changed, 2 insertions(+), 3 deletions(-)
>
> diff --git a/Xi/queryst.c b/Xi/queryst.c
> index 60ec32e..e0343f9 100644
> --- a/Xi/queryst.c
> +++ b/Xi/queryst.c
> @@ -85,7 +85,6 @@ ProcXQueryDeviceState(ClientPtr client)
> xValuatorState *tv;
> xQueryDeviceStateReply rep;
> DeviceIntPtr dev;
> - double *values;
>
> REQUEST(xQueryDeviceStateReq);
> REQUEST_SIZE_MATCH(xQueryDeviceStateReq);
> @@ -151,8 +150,8 @@ ProcXQueryDeviceState(ClientPtr client)
> tv->num_valuators = v->numAxes;
> tv->mode = v->mode;
> buf += sizeof(xValuatorState);
> - for (i = 0, values = v->axisVal; i < v->numAxes; i++) {
> - *((int *)buf) = *values++;
> + for (i = 0; i < v->numAxes; i++) {
> + *((int *)buf) = dev->last.valuators[i];
> if (client->swapped) {
> swapl((int *)buf, n); /* macro - braces needed */
> }
> --
> 1.6.3.3
>
> From 442efa5a67ab3db5279a017be6aea757448e8695 Mon Sep 17 00:00:00 2001
> From: Thomas Jaeger <ThJaeger at gmail.com>
> Date: Wed, 23 Sep 2009 13:10:53 -0400
> Subject: [PATCH 2/2] Kill off axisVal completely
>
> There's no point in having the same information stored in two different
> places.
> ---
> Xi/exevents.c | 23 ++++++-----------------
> Xi/xiquerydevice.c | 4 ++--
> dix/devices.c | 8 ++------
> hw/xfree86/common/xf86Xinput.c | 6 ++----
> include/inputstr.h | 1 -
> 5 files changed, 12 insertions(+), 30 deletions(-)
>
> diff --git a/Xi/exevents.c b/Xi/exevents.c
> index b0e0ede..558cfd0 100644
> --- a/Xi/exevents.c
> +++ b/Xi/exevents.c
> @@ -551,7 +551,6 @@ DeepCopyPointerClasses(DeviceIntPtr from, DeviceIntPtr to)
> v->axes = (AxisInfoPtr)&v[1];
> memcpy(v->axes, from->valuator->axes, v->numAxes * sizeof(AxisInfo));
>
> - v->axisVal = (double*)(v->axes + from->valuator->numAxes);
> v->sourceid = from->id;
> v->mode = from->valuator->mode;
> } else if (to->valuator && !from->valuator)
> @@ -799,16 +798,6 @@ UpdateDeviceState(DeviceIntPtr device, DeviceEvent* event)
> }
> }
>
> - for (i = 0; i <= last_valuator && i < v->numAxes; i++)
> - {
> - if (BitIsOn(&event->valuators.mask, i))
> - {
> - /* XXX: Relative/Absolute mode */
> - v->axisVal[i] = event->valuators.data[i];
> - v->axisVal[i] += event->valuators.data_frac[i];
> - }
> - }
> -
> if (event->type == ET_KeyPress) {
> if (!k)
> return DONT_PROCESS;
> @@ -1176,11 +1165,11 @@ FixDeviceStateNotify(DeviceIntPtr dev, deviceStateNotify * ev, KeyClassPtr k,
> ev->num_valuators = nval < 3 ? nval : 3;
> switch (ev->num_valuators) {
> case 3:
> - ev->valuator2 = v->axisVal[first + 2];
> + ev->valuator2 = dev->last.valuators[first + 2];
> case 2:
> - ev->valuator1 = v->axisVal[first + 1];
> + ev->valuator1 = dev->last.valuators[first + 1];
> case 1:
> - ev->valuator0 = v->axisVal[first];
> + ev->valuator0 = dev->last.valuators[first];
> break;
> }
> }
> @@ -1198,11 +1187,11 @@ FixDeviceValuator(DeviceIntPtr dev, deviceValuator * ev, ValuatorClassPtr v,
> ev->first_valuator = first;
> switch (ev->num_valuators) {
> case 3:
> - ev->valuator2 = v->axisVal[first + 2];
> + ev->valuator2 = dev->last.valuators[first + 2];
> case 2:
> - ev->valuator1 = v->axisVal[first + 1];
> + ev->valuator1 = dev->last.valuators[first + 1];
> case 1:
> - ev->valuator0 = v->axisVal[first];
> + ev->valuator0 = dev->last.valuators[first];
> break;
> }
> first += ev->num_valuators;
> diff --git a/Xi/xiquerydevice.c b/Xi/xiquerydevice.c
> index 68d91fa..5c727f5 100644
> --- a/Xi/xiquerydevice.c
> +++ b/Xi/xiquerydevice.c
> @@ -338,8 +338,8 @@ ListValuatorInfo(DeviceIntPtr dev, xXIValuatorInfo* info, int axisnumber)
> info->min.frac = 0;
> info->max.integral = v->axes[axisnumber].max_value;
> info->max.frac = 0;
> - info->value.integral = (int)v->axisVal[axisnumber];
> - info->value.frac = (int)(v->axisVal[axisnumber] * (1 << 16) * (1 << 16));
> + info->value.integral = dev->last.valuators[axisnumber];
> + info->value.frac = (int)(dev->last.remainder[axisnumber] * (1 << 16) * (1 << 16));
> info->resolution = v->axes[axisnumber].resolution;
> info->number = axisnumber;
> info->mode = v->mode; /* Server doesn't have per-axis mode yet */
> diff --git a/dix/devices.c b/dix/devices.c
> index e86e606..cbe646b 100644
> --- a/dix/devices.c
> +++ b/dix/devices.c
> @@ -571,10 +571,8 @@ CorePointerProc(DeviceIntPtr pDev, int what)
> pDev->name);
> return BadAlloc; /* IPDS only fails on allocs */
> }
> - pDev->valuator->axisVal[0] = screenInfo.screens[0]->width / 2;
> - pDev->last.valuators[0] = pDev->valuator->axisVal[0];
> - pDev->valuator->axisVal[1] = screenInfo.screens[0]->height / 2;
> - pDev->last.valuators[1] = pDev->valuator->axisVal[1];
> + pDev->last.valuators[0] = screenInfo.screens[0]->width / 2;
> + pDev->last.valuators[1] = screenInfo.screens[0]->height / 2;
> break;
>
> case DEVICE_CLOSE:
> @@ -1176,7 +1174,6 @@ InitValuatorClassDeviceStruct(DeviceIntPtr dev, int numAxes, Atom *labels,
> valc->numAxes = numAxes;
> valc->mode = mode;
> valc->axes = (AxisInfoPtr)(valc + 1);
> - valc->axisVal = (double *)(valc->axes + numAxes);
> dev->valuator = valc;
>
> AllocateMotionHistory(dev);
> @@ -1184,7 +1181,6 @@ InitValuatorClassDeviceStruct(DeviceIntPtr dev, int numAxes, Atom *labels,
> for (i=0; i<numAxes; i++) {
> InitValuatorAxisStruct(dev, i, labels[i], NO_AXIS_LIMITS, NO_AXIS_LIMITS,
> 0, 0, 0);
> - valc->axisVal[i]=0;
> }
>
> dev->last.numValuators = numAxes;
> diff --git a/hw/xfree86/common/xf86Xinput.c b/hw/xfree86/common/xf86Xinput.c
> index b369537..62bb1e6 100644
> --- a/hw/xfree86/common/xf86Xinput.c
> +++ b/hw/xfree86/common/xf86Xinput.c
> @@ -1040,12 +1040,10 @@ void
> xf86InitValuatorDefaults(DeviceIntPtr dev, int axnum)
> {
> if (axnum == 0) {
> - dev->valuator->axisVal[0] = screenInfo.screens[0]->width / 2;
> - dev->last.valuators[0] = dev->valuator->axisVal[0];
> + dev->last.valuators[0] = screenInfo.screens[0]->width / 2;
> }
> else if (axnum == 1) {
> - dev->valuator->axisVal[1] = screenInfo.screens[0]->height / 2;
> - dev->last.valuators[1] = dev->valuator->axisVal[1];
> + dev->last.valuators[1] = screenInfo.screens[0]->height / 2;
> }
>
> if(axnum == 0) /* to prevent double invocation */
> diff --git a/include/inputstr.h b/include/inputstr.h
> index 29ad5a8..61a107c 100644
> --- a/include/inputstr.h
> +++ b/include/inputstr.h
> @@ -236,7 +236,6 @@ typedef struct _ValuatorClassRec {
>
> AxisInfoPtr axes;
> unsigned short numAxes;
> - double *axisVal; /* always absolute, but device-coord system */
> CARD8 mode;
> ValuatorAccelerationRec accelScheme;
> } ValuatorClassRec, *ValuatorClassPtr;
> --
> 1.6.3.3
>
More information about the xorg-devel
mailing list