On Wed, Mar 24, 2010 at 5:15 PM, Peter Hutterer
<[email protected]> wrote:
> On Wed, Mar 24, 2010 at 03:13:10PM -0700, Ping Cheng wrote:
>> We might not get all the valid types for ISDV4 devices in
>> wcmPreInit(). Assuming users know what they are doing so we make their
>> types valid.
>>
>> Signed-off-by: Ping Cheng <[email protected]>
>
>> From fcf8974b5f9ee3d515a12296467efc0a14b6ce82 Mon Sep 17 00:00:00 2001
>> From: Ping Cheng <[email protected]>
>> Date: Wed, 24 Mar 2010 13:54:32 -0700
>> Subject: [PATCH] User defined ISDV4 device types in xorg.conf are always
>> valid
>>
>> We might not get all the valid types for ISDV4 devices in
>> wcmPreInit(). Assuming users know what they are doing so we
>> make their types valid.
>>
>> Signed-off-by: Ping Cheng <[email protected]>
>> ---
>> src/wcmValidateDevice.c | 28 +++++++++++++++++++++++++++-
>> 1 files changed, 27 insertions(+), 1 deletions(-)
>>
>> diff --git a/src/wcmValidateDevice.c b/src/wcmValidateDevice.c
>> index f153cd2..f121fb1 100644
>> --- a/src/wcmValidateDevice.c
>> +++ b/src/wcmValidateDevice.c
>> @@ -25,6 +25,7 @@
>> #include <sys/stat.h>
>> #include <fcntl.h>
>> #include <unistd.h>
>> +#include <linux/serial.h>
>>
>> #define ARRAY_SIZE(a) (sizeof(a)/sizeof((a)[0]))
>>
>> @@ -139,6 +140,10 @@ Bool wcmIsAValidType(LocalDevicePtr local, const char*
>> type)
>> int j, ret = FALSE;
>> WacomDevicePtr priv = (WacomDevicePtr)local->private;
>> WacomCommonPtr common = priv->common;
>> + char* dsource = xf86CheckStrOption(local->options, "_source", "");
>> + char* device = xf86SetStrOption(local->options, "Device", NULL);
>> + int fd = -1;
>> + struct serial_struct tmp;
>>
>> if (!type)
>> return FALSE;
>> @@ -147,11 +152,32 @@ Bool wcmIsAValidType(LocalDevicePtr local, const char*
>> type)
>> for (j = 0; j < ARRAY_SIZE(wcmType); j++)
>> {
>> if (!strcmp(wcmType[j].type, type))
>> - if (ISBITSET (common->wcmKeys, wcmType[j].tool))
>> + {
>> + SYSCALL(fd = open(device, O_RDONLY));
>
> file descriptor is leaked.
Nice catch! Will fix that.
>> + if (fd < 0)
>> {
>> + xf86Msg(X_WARNING, "%s: failed to open %s in "
>> + "wcmIsAValidType.\n", local->name, device);
>> + return FALSE;
>> + }
>> +
>> + /* tool defined in xorg.conf and is an ISDV4 device? */
>> + if (!strlen(dsource) && !ioctl(fd, TIOCGSERIAL, &tmp))
>> + {
>> + /* We might not get all the valid types for
>> + * ISDV4 devices at this stage. Assuming user
>> + * is right, let's make the type valid */
>> + common->wcmKeys[LONG(wcmType[j].tool)]
>> + |= BIT(wcmType[j].tool);
>> ret = TRUE;
>> break;
>> }
>> + else if (ISBITSET (common->wcmKeys, wcmType[j].tool))
>> + {
>> + ret = TRUE;
>> + break;
>> + }
>> + }
>> }
>> return ret;
>> }
>> --
>> 1.6.6.1
>
> thanks for the patch. A few comments:
> This approach seems expensive.
Yes, it is an expensive situation. Otherwise, we would have to open
the serial port and query the actual data at the first place. We
could go with that path if you feel that is cheaper than this one (I
have no personal preference :).
> for USB devices we can already assume that
> the keys are set correctly but with this patch we now open the FD on every
> one of them, do some checks, etc. before checking the bits.
> I think it'd be better to check the keys first and only if they are invalid,
> then check the actual file.
Yeah, I see your point. That can be done.
> Having said that, this code seems in the wrong place.
>
> wcmIsAValidType() is called from only two locations - wcmNeedAutoHotplug()
> and directly from wcmPreInit(). Only in the latter case are the checks above
> sensible. Why not move the check into isdv4ProbeKeys(), since we do all the
> opening of the device file, the check for serial, etc. there anyway. It
> should be trivial to add the bit flags for the device if it comes from the
> xorg.conf.
We can not move the check into isdv4ProbeKeys since it would only be
called once for the first tool on the port, which leaves all the other
valid tools as invalid. This patch is mainly for those ISDV4 devices
that we could not find their id's (such as Fujitsu ones) in
isdv4ProbeKeys for some reason. So, each type would have to be added
manually.
Another way to handle this special case might be to trust our users to
the most and make all xorg.conf defined types valid. It is cheap. But
I don't know if I can trust myself all the time :).
Ping
------------------------------------------------------------------------------
Download Intel® Parallel Studio Eval
Try the new software tools for yourself. Speed compiling, find bugs
proactively, and fine-tune applications for parallel performance.
See why Intel Parallel Studio got high marks during beta.
http://p.sf.net/sfu/intel-sw-dev
_______________________________________________
Linuxwacom-devel mailing list
[email protected]
https://lists.sourceforge.net/lists/listinfo/linuxwacom-devel
