[PATCH v2] Add decoding for evdev ioctls.

[Dmitry V. Levin]

On Mon, Feb 09, 2015 at 05:05:06PM +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.
[...]
> +void evdev_ioctl_decode_envelope(struct ff_envelope *envelope)

Please declare all new functions that are not going to be used outside of
evdev.c (that is, all but evdev_ioctl) as static.

> +{
> +	tprintf(", envelope={attack_length=%" PRIu16 ", attack_level=%" PRIu16
> +			", fade_length=%" PRIu16 ", fade_level=%" PRIx32 "}",
> +			envelope->attack_length,
> +			envelope->attack_level,
> +			envelope->fade_length,
> +			envelope->fade_level);
> +}
> +
> +int evdev_ff_effect_ioctl(struct tcb *tcp, long arg)
> +{
> +	struct ff_effect ffe;
> +
> +	if (!verbose(tcp) || umove(tcp, arg, &ffe) < 0)
> +		return 0;
> +
> +	tprints(", {type=");
> +	printxval(evdev_ff_types, ffe.type, "FF_???");
> +	tprintf(", id=%" PRIu16 ", direction=%" PRIu16,
> +			ffe.id, ffe.direction);

Most of tprintf calls in this patch are over-indented.
For example, the last one would look better if it was written this way:

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

WRT using of PRI[iux]* constants, the only case when they are really
useful is their 64-bit variants when they definitions differ between
architectures.  In other cases, it's a matter of taste.

[...]
> +int evdev_keycode_V2_ioctl(struct tcb *tcp, long arg)
> +{
> +	struct input_keymap_entry ike;
> +	unsigned i;
> +
> +	if (!arg) {
> +		tprints(", NULL");
> +		return 1;
> +	}
> +
> +	if (!verbose(tcp) || umove(tcp, arg, &ike) < 0)
> +		return 0;
> +
> +	tprintf(", {flags=%" PRIu8 ", len=%" PRIu8,
> +			ike.flags, ike.len);
> +	if (!abbrev(tcp)) {
> +		tprintf(", index=%" PRIu16 ", keycode=%" PRIu32
> +				,ike.index, ike.keycode);

Please don't start lines with a comma.

> +		tprints(", scancode={");
> +		for (i = 0; i < 32; i++)

Please replace 32 with ARRAY_SIZE(ike.scancode).

> +			tprintf(" %" PRIu8, ike.scancode[i]);

Have you considered printing scancodes in hex?

> +		tprints("}}");
> +	} else {
> +		tprints(", ...}");
> +	}
> +	return 1;
> +}
> +
> +int evdev_decode_per_byte_ioctls(struct tcb *tcp, long arg,

These function names are getting longer and longer.
Since the whole file is about decoding evdev ioctls, it's not necessary to
use all of these key words in the name of every local function.

For example, this one could be called decode_bitset or print_bitset or
something like this.

> +		const struct xlat decode_flag[], const unsigned int max_flag)
> +{
> +	char *decoded_arg = calloc(tcp->u_rval, sizeof(char));
> +	if (decoded_arg == NULL) {
> +		return 0;
> +	}

The max_flag argument here is a cap on tcp->u_rval, please apply it.
The largest cap currently defined in linux/input.h is KEY_MAX (0x2ff),
I suppose we can allow that much data to be allocated on the stack.

> +
> +	const char *sep;
> +	int all_to_zero;
> +	int i;
> +	int j = 0;
> +
> +	if (!verbose(tcp) || umoven(tcp, arg, tcp->u_rval, decoded_arg) < 0) {
> +		free(decoded_arg);
> +		return 0;
> +	}
> +
> +	all_to_zero = 1;
> +	tprints(", [");
> +	for (i = 0, sep = "";; i++) {
> +		i = next_set_bit(decoded_arg, i, tcp->u_rval);
> +		if (i < 0)
> +			break;
> +		tprintf("%s%s", sep, xlookup(decode_flag, i));

xlookup() can return NULL, please use printxval unless it isn't suitable.
Here you could safely write
		tprints(sep);
		printxval(decode_flag, i, default);

> +		sep = ", ";

This assignment would look better if placed along with i++.

> +		all_to_zero = 0;
> +		if (abbrev(tcp) && j >= 3) {
> +			tprints(", ...");
> +			break;
> +		}

What if it was the last bit in the bitset?
Wouldn't it be better to place this check before tprints/tprintf?

> +		j++;

j++ would look better if placed along with i++.

> +	}
> +
> +	if (all_to_zero)
> +		tprints(" 0 ]");
> +	else
> +		tprints("]");

all_to_zero is redundant, you can test j instead.

> +
> +	free(decoded_arg);
> +	return 1;
> +}
> +
> +int evdev_ioctl(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	struct input_id id;
> +	unsigned int val[2];
> +	int i;
> +	int values_size;
> +	int passed;
> +
> +	if (entering(tcp)) {
> +		//On entering, we should only decode _IOW ioctls

We use C style comments.

> +		if (_IOC_DIR(code) != _IOC_WRITE)
> +			return 0;

It's not easy to implement correctly this way.
sys_ioctl() uses return code of this function only on exit of syscall,
so extra case should be taken to return proper code on exit of _IOW ioctls.

I'd invert the checks for syscall state and ioctl direction:

	switch (_IOC_DIR(code)) {
		case _IOC_READ:
			return evdev_read_ioctl(tcp, code, arg);
		case _IOC_WRITE:
			return evdev_write_ioctl(tcp, code, arg);
		default:
			return 0;
	}

If evdev_write_ioctl() implements fallback printing of unparsable ioctls,
it could be made to always return 1 without any issues of passing return
code between syscall stages.

> +		switch (code) {
> +			case EVIOCSREP:
> +				if (!verbose(tcp) || umove(tcp, arg, (char *)val) < 0)
> +					return 0;
> +
> +				tprintf(", [delay=%" PRIu32 ", period=%" PRIu32 "]",
> +						val[0], val[1]);
> +				return 1;
> +			case EVIOCSKEYCODE:
> +				return evdev_keycode_ioctl(tcp, arg);
> +			case EVIOCSKEYCODE_V2:
> +				return evdev_keycode_V2_ioctl(tcp, arg);
> +			case EVIOCSFF:
> +				return evdev_ff_effect_ioctl(tcp, arg);
> +			case EVIOCRMFF:
> +			case EVIOCSCLOCKID:
> +			case EVIOCGRAB:
> +			case EVIOCREVOKE:
> +				tprints(", ");
> +				printnum_int(tcp, arg, "%u");
> +				return 1;
> +			default:
> +				return 0;
> +		}
> +
> +		if ((_IOC_NR(code) & ~ABS_MAX) == _IOC_NR(EVIOCSABS(0)))
> +			return evdev_abs_ioctl(tcp, arg);
> +

What is going to happen if control reached this point?

> +	} else {
> +		//On exiting, we should only decode _IOR ioctls
> +		if (syserror(tcp)) {
> +			if (_IOC_DIR(code) != _IOC_READ)
> +				return 2;
> +			else
> +				return 0;
> +		}
> +		if (_IOC_DIR(code) != _IOC_READ)
> +			return 2;

What do you mean by returning 2?
Do you really want to print ioctl return code in hex?

On entering syscall, it's return code is ignored.
On exiting, it's return code must be 0 if and only if nothing was printed,
neither on entering syscall nor on exiting.  Otherwise, it's return code
must be 1 + appropriate RVAL_* constant. 

> +		if ((_IOC_NR(code) & ~EV_MAX) == _IOC_NR(EVIOCGBIT(0, 0))) {
> +			switch (_IOC_NR(code) - 0x20) {
> +				case EV_SYN:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_sync, SYN_MAX);
> +				case EV_KEY:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_keycode, KEY_MAX);
> +				case EV_REL:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_relative_axes, REL_MAX);
> +				case EV_ABS:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_abs, ABS_MAX);
> +				case EV_MSC:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_misc, MSC_MAX);
> +				case EV_SW:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_switch, SW_MAX);
> +				case EV_LED:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_leds, LED_MAX);
> +				case EV_SND:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_snd, SND_MAX);
> +				case EV_REP:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_autorepeat, REP_MAX);
> +				case EV_FF:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_ff_types, FF_MAX);
> +				case EV_PWR:
> +					printnum(tcp, arg, "%" PRId32);
> +					return 1;
> +				case EV_FF_STATUS:
> +					return evdev_decode_per_byte_ioctls(tcp, arg,
> +							evdev_ff_status, FF_STATUS_MAX);
> +				default:
> +					return 0;
> +			}
> +		}
> +
> +		if ((_IOC_NR(code) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0)))
> +			return evdev_abs_ioctl(tcp, arg);
> +
> +		if (code == EVIOCGKEYCODE)
> +			return evdev_keycode_ioctl(tcp, arg);
> +		else if (code == EVIOCGKEYCODE_V2)
> +			return evdev_keycode_V2_ioctl(tcp, arg);
> +
> +		struct input_mt_request_layout {
> +			uint32_t code;
> +			int32_t values[tcp->u_rval/sizeof(int32_t) - 1];
> +		} mtslots;
> +
> +		switch (_IOC_NR(code)) {
> +			case _IOC_NR(EVIOCGVERSION):

Only parametrized macros like EVIOCGNAME need this _IOC_NR reduction,
all constants like EVIOCGVERSION have to be compared with all their bits
taken into account.


-- 
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/20150212/385d7caf/attachment.bin>