[PATCH v5] Add decoding for evdev ioctls.

[Dmitry V. Levin]

On Thu, Feb 19, 2015 at 05:11:20PM +0100, Etienne Gemsa wrote:
> This patch introduces decoding for evdev ioctls. For EVIOCSFF the five
> first members are decoded, the union is not. The code was compiled and
> tested on x86_64 with Linux 3.13.

The patch is getting better.  I hope v6 will be good enough to merge.

> +	tprintf(", id=%" PRIu16 ", direction=%" PRIu16,
> +			ffe.id, ffe.direction);

Please fix indentation here.

> +static int decode_bitset(struct tcb *tcp, long arg,
> +		const struct xlat decode_flag[], const unsigned int max_flag, const char *dflt)
> +{
> +	if (!verbose(tcp))
> +		return 0;
> +
> +	unsigned int size =
> +		(unsigned long) tcp->u_rval > max_flag ? max_flag : tcp->u_rval;
> +	char decoded_arg[size];
> +
> +	if (umoven(tcp, arg, size, decoded_arg) < 0)
> +		return 0;
> +
> +	tprints(", [");
> +
> +	int bit_displayed = 0;
> +	int i = next_set_bit(decoded_arg, 0, tcp->u_rval);

Once tcp->u_rval is checked against max_flag, size should be used instead
of tcp->u_rval:

	int i = next_set_bit(decoded_arg, 0, size);

> +	if (i < 0) {
> +		tprints(" 0 ");
> +	} else {
> +		printxval(decode_flag, i, dflt);

A variable called "decode_flag" is passed to a function called
"printxval" - sounds confusing.  The objects that are being decoded are 
actually not flags but values, otherwise they would have to be decoded 
with printflags().  Consider renaming decode_flag to something else that 
doesn't mention "flag".

> +		i++;
> +
> +		for (; (i = next_set_bit(decoded_arg, i, tcp->u_rval)) >= 0; i++) {

You can replace these two lines with a single line:

		while ((i = next_set_bit(decoded_arg, i + 1, size)) > 0)

> +static int mtslots_ioctl(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	const size_t size = _IOC_SIZE(code) / sizeof(int32_t);
> +	if (!size)
> +		return 0;
> +
> +	int32_t buffer[size];
> +
> +	if (!verbose(tcp) || umove(tcp, arg, &buffer) < 0)
> +		return 0;
> +
> +	tprints(", {code=");
> +	printxval(evdev_mtslots, buffer[0], "error");

"error" is not very descriptive, something like "ABS_MT_???" would be
better.

> +
> +	unsigned int i;
> +	int passed;
> +	tprints(", values=[");
> +	passed = 0;
> +	for (i = 1; i < ARRAY_SIZE(buffer); i++) {
> +		tprintf("%s%d", passed ? ", " : "", buffer[i]);

The same could be written as
		tprintf("%s%d", i > 1 ? ", " : "", buffer[i]);

> +		passed = 1;
> +	}
> +	tprints("}");

There is an opening square bracket for values[] without a closing one.

> +	switch (code) {
> +		case EVIOCGVERSION:
> +			tprints(", ");
> +			printnum_long(tcp, arg, "%" PRIx32);
> +			return 1;
> +		case EVIOCGEFFECTS:
> +			tprints(", ");
> +			printnum_long(tcp, arg, "%" PRIu32);
> +			return 1;

kernel puts int, PRIx32 prints int, but printnum_long would attempt to
fetch a long int, and in the worst case it will fail.
It has to be printnum_int.

> +	if(exiting(tcp))

Please put a space after "if".

> diff --git a/xlat/evdev_events.in b/xlat/evdev_events.in
> new file mode 100644
> index 0000000..0974b59
> --- /dev/null
> +++ b/xlat/evdev_events.in
> @@ -0,0 +1,12 @@
> +EV_SYN
> +EV_KEY
> +EV_REL
> +EV_ABS
> +EV_MSC
> +EV_SW
> +EV_LED
> +EV_SND
> +EV_REP
> +EV_FF
> +EV_PWR
> +EV_FF_STATUS

Do you really need this file?  If yes, there is another file with the same
contents (xlat/evdev_ev.in) that could be used instead.


-- 
ldv
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 181 bytes
Desc: not available
URL: <http://lists.strace.io/pipermail/strace-devel/attachments/20150220/d74c8003/attachment.bin>