Merge lp:~oif-team/evemu/fix-evbit-handling into lp:evemu

[Chase Douglas]

Fix evbit handling in device property files.

[Henrik Rydberg (rydberg)]

Rendering existing bug reports and accumulated data invalid because someone wants a "E:" instead of a "A:" in a report costs much more than it gains. I am disapproving this change.

review: Disapprove

[Henrik Rydberg (rydberg)]

Oh, I forgot - "E:" is also used in other related contexts (events files), creating a different kind of confusion.

[Chase Douglas (chasedouglas)]

Certainly it can be renamed. However, I fail to see how this invalidates existing bug reports and accumulated data.

This is a real bug that I spent a good 30 minutes trying to figure out, and I only figured it out because I had done extensive work with uinput in the past. This isn't just a cosmetic issue.

We can quite easily rename this to something other than "E:". How about "T:" for types?

[lp:~oif-team/evemu/fix-evbit-handling]

28. By Chase Douglas on 2011-02-28

Rename evbit line from "E:" to "T:" (name clash with event records)

[Henrik Rydberg (rydberg)]

How about adding the line "The first B: line lists the used event types" to the documentation and be done with it. It solves the real problem, without changing file formats.

And the less time we spend on this non-issue, the less time get wasted.

[Chase Douglas (chasedouglas)]

The documentation isn't enough. A developer should be able to look at a device prop file and figure this out themselves. If we couldn't do this in a backwards compatible way, then documenting the issue might be a reasonable approach. However, the proposed merge keeps all backwards compatibility and fixes things going forward. No more time needs to be spent on the issue if the merge is approved, and I'm unaware of any technical reason not to.

[Henrik Rydberg (rydberg)]

Documentation should be enough. Right now, there are at least two ways to interpret the letter "B:" one is that it means something with bitmask, and the numbers correspond to something. There is nothing documented saying all B:s have to means the same thing, and indeed, they don't. Thus, a change of letters only switches confusion to some other place.

The main reason to not change this is not technical, but a matter of stability. The file format is already in use. This alone makes a request like this branch raise all red flags. When combined with the fact that indeed, the whole issue is just a matter of interpretation, it makes it even more important to say no.

I am disapproving this again, to underline that fact that I really, really, think this is both a silly and harmful change.

review: Disapprove

Unmerged revisions

28. By Chase Douglas on 2011-02-28

Rename evbit line from "E:" to "T:" (name clash with event records)

27. By Chase Douglas on 2011-02-28

Output EVBIT bits on a separate line

The current code outputs the EVBIT bits on the EV_SYN mask line (the line
that begins with "B: 00"). This is incorrect as the EVBIT bits have
nothing to do with EV_SYN.

This change creates a new "E:" line that outputs the EVBIT bits. The
"B: 00" will not be printed any longer, since there's no EVDEV protocol
handling for EV_SYN. The change is backwards compatible with the old
format. Creating a device with an older file will still work properly.

Fixes LP: #726773

=== modified file 'src/evemu-impl.h'
--- src/evemu-impl.h	2011-02-03 02:29:13 +0000
+++ src/evemu-impl.h	2011-02-28 21:08:14 +0000
@@ -21,6 +21,8 @@
 #include <evemu.h>
 #include <linux/uinput.h>
 
+#define EVBIT_NBITS	EV_CNT
+#define EVBIT_NBYTES	((EV_CNT + 7) / 8)
 #define EVPLAY_NBITS	KEY_CNT
 #define EVPLAY_NBYTES	((EVPLAY_NBITS + 7) / 8)
 
@@ -29,6 +31,7 @@
 	char name[UINPUT_MAX_NAME_SIZE];
 	struct input_id id;
 	unsigned char prop[EVPLAY_NBYTES];
+	unsigned char evmask[EVBIT_NBYTES];
 	unsigned char mask[EV_CNT][EVPLAY_NBYTES];
 	int pbytes, mbytes[EV_CNT];
 	struct input_absinfo abs[ABS_CNT];
 
=== modified file 'src/evemu.c'
--- src/evemu.c	2011-02-03 02:29:13 +0000
+++ src/evemu.c	2011-02-28 21:08:14 +0000
@@ -164,9 +164,15 @@
 	return (dev->prop[code >> 3] >> (code & 7)) & 1;
 }
 
+static int evemu_has_type(const struct evemu_device *dev, int type)
+{
+	return (dev->evmask[type >> 3] >> (type & 7)) & 1;
+}
+
 int evemu_has_event(const struct evemu_device *dev, int type, int code)
 {
-	return (dev->mask[type][code >> 3] >> (code & 7)) & 1;
+	return (evemu_has_type(dev, type) &&
+		(dev->mask[type][code >> 3] >> (code & 7)) & 1);
 }
 
 int evemu_extract(struct evemu_device *dev, int fd)
@@ -193,7 +199,12 @@
 		dev->pbytes = rc;
 	}
 
-	for (i = 0; i < EV_CNT; i++) {
+	SYSCALL(rc = ioctl(fd, EVIOCGBIT(0, EVBIT_NBYTES), bits));
+	if (rc < 0)
+		return rc;
+	copy_bits(dev->evmask, bits, rc);
+
+	for (i = EV_KEY; i < EV_CNT; i++) {
 		SYSCALL(rc = ioctl(fd, EVIOCGBIT(i, sizeof(bits)), bits));
 		if (rc < 0)
 			continue;
@@ -221,6 +232,15 @@
 			mask[i + 4], mask[i + 5], mask[i + 6], mask[i + 7]);
 }
 
+static void write_evmask(FILE * fp, const unsigned char *evmask)
+{
+	int i;
+	fprintf(fp, "T:");
+	for (i = 0; i < EVBIT_NBYTES; i++)
+		fprintf(fp, " %02x", evmask[i]);
+	fprintf(fp, "\n");
+}
+
 static void write_mask(FILE * fp, int index,
 		       const unsigned char *mask, int bytes)
 {
@@ -249,7 +269,9 @@
 
 	write_prop(fp, dev->prop, dev->pbytes);
 
-	for (i = 0; i < EV_CNT; i++)
+	write_evmask(fp, dev->evmask);
+
+	for (i = EV_KEY; i < EV_CNT; i++)
 		write_mask(fp, i, dev->mask[i], dev->mbytes[i]);
 
 	for (i = 0; i < ABS_CNT; i++)
@@ -271,6 +293,21 @@
 	}
 }
 
+static void read_evmask(struct evemu_device *dev, FILE *fp)
+{
+	unsigned int mask[8];
+	int i;
+	if (fscanf(fp, "B: 00 %02x %02x %02x %02x %02x %02x %02x %02x\n",
+		   mask + 0, mask + 1, mask + 2, mask + 3, mask + 4, mask + 5,
+		   mask + 6, mask + 7) > 0)
+		for (i = 0; i < EVBIT_NBYTES; i++)
+			dev->evmask[i] = mask[i];
+	else if (fscanf(fp, "T: %02x %02x %02x %02x\n", mask + 0, mask + 1,
+			mask + 2, mask + 3) > 0)
+		for (i = 0; i < EVBIT_NBYTES; i++)
+			dev->evmask[i] = mask[i];
+}
+
 static void read_mask(struct evemu_device *dev, FILE *fp)
 {
 	unsigned int mask[8];
@@ -315,6 +352,8 @@
 
 	read_prop(dev, fp);
 
+	read_evmask(dev, fp);
+
 	read_mask(dev, fp);
 
 	read_abs(dev, fp);
@@ -395,9 +434,6 @@
 	int ret = 0;
 
 	switch(type) {
-	case EV_SYN:
-		SYSCALL(ret = ioctl(fd, UI_SET_EVBIT, code));
-		break;
 	case EV_KEY:
 		SYSCALL(ret = ioctl(fd, UI_SET_KEYBIT, code));
 		break;
@@ -441,6 +477,19 @@
 	return 0;
 }
 
+static int set_evmask(const struct evemu_device *dev, int fd)
+{
+	int ret, i;
+	for (i = 0; i < EVBIT_NBITS; i++) {
+		if (!evemu_has_type(dev, i))
+			continue;
+		SYSCALL(ret = ioctl(fd, UI_SET_EVBIT, i));
+		if (ret < 0)
+			return ret;
+	}
+	return 0;
+}
+
 static int set_mask(const struct evemu_device *dev, int type, int fd)
 {
 	int bits = 8 * dev->mbytes[type];
@@ -476,6 +525,10 @@
 	if (ret < 0)
 		return ret;
 
+	ret = set_evmask(dev, fd);
+	if (ret < 0)
+		return ret;
+
 	for (i = 0; i < EV_CNT; i++) {
 		ret = set_mask(dev, i, fd);
 		if (ret < 0)