[PATCH v3] Add decoding for evdev ioctls.

[Dmitry V. Levin]

On Mon, Feb 16, 2015 at 02:52:10PM +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.
[...]
> +static int decode_bitset(struct tcb *tcp, long arg,
> +		const struct xlat decode_flag[], const unsigned int max_flag)
> +{
> +	/* KEY_MAX is the largest possible flag */
> +	char decoded_arg[KEY_MAX];

No meed to hardcode KEY_MAX, the upper limit is passed via max_flag.
Just make sure that tcp->u_rval does not exceed max_flag, and allocate
an array of size tcp->u_rval:

	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;
	...
		i = next_set_bit(decoded_arg, i, size);

> +	const char *sep;
> +	int i;
> +	int j;
> +	int last = 0;
> +
> +	if (!verbose(tcp) || umoven(tcp, arg, tcp->u_rval, decoded_arg) < 0) {
> +		return 0;
> +	}
> +
> +	tprints(", [");
> +	for (i = 0, j = 0, sep = "";; i++, j++, sep = ", ") {
> +		i = next_set_bit(decoded_arg, i, tcp->u_rval);
> +		if (i < 0)
> +			break;
> +		if (abbrev(tcp) && j >= 3) {
> +			if (last) {
> +				tprints(", ...");
> +				break;
> +			}
> +			last = 1;
> +		}

You don't have to make the code so complicated.  A simple check like this
		if (abbrev(tcp) && j >= 3) {
			tprints(", ...");
			break;
		}
should be enough.

> +		tprints(sep);
> +		printxval(decode_flag, i, "ERR");

This "ERR" is not quite descriptive.
Could you pass a more specific default value to this function?

> +			case EV_PWR:
> +				printnum(tcp, arg, "%" PRId32);

In this and other places where printnum() is used:
- if you want to print int, use printnum_int() with an integer format;
- if you want to print long int, use printnum() with a long integer format;
- if you really want to fetch a long integer and print it as an integer,
  this is very unlikely and requires a lengthy comment.

> +				return 1;
> +			case EV_FF_STATUS:
> +				return decode_bitset(tcp, arg,
> +						evdev_ff_status, FF_STATUS_MAX);
> +			default:
> +				return 0;
> +		}
> +	}
> +
> +	if ((_IOC_NR(code) & ~ABS_MAX) == _IOC_NR(EVIOCGABS(0)))
> +		return abs_ioctl(tcp, arg);
> +
> +	if (code == EVIOCGKEYCODE)
> +		return keycode_ioctl(tcp, arg);
> +	else if (code == EVIOCGKEYCODE_V2)
> +		return keycode_V2_ioctl(tcp, arg);

Is there any reason for this EVIOCGKEYCODE/EVIOCGKEYCODE_V2 code to be
moved out of the main "switch (code)" below?
> +
> +	struct input_mt_request_layout {
> +		uint32_t code;
> +		int32_t values[tcp->u_rval/sizeof(int32_t) - 1];

Is it correct to use tcp->u_rval here?

> +	} mtslots;
> +
> +	switch (code) {
> +		case EVIOCGVERSION:
> +			tprints(", ");
> +			printnum(tcp, arg, "%" PRIx32);
> +			return 1;
> +		case EVIOCGEFFECTS:
> +			tprints(", ");
> +			printnum(tcp, arg, "%" PRIu32);
> +			return 1;
> +		case EVIOCGID:
> +			if (!verbose(tcp) || umove(tcp, arg, &id) < 0)
> +				return 0;
> +
> +			tprintf(", {ID_BUS=%" PRIu16 ", ID_VENDOR=%" PRIu16,
> +				id.bustype, id.vendor);
> +			if (!abbrev(tcp)) {
> +				tprintf(", ID_PRODUCT=%" PRIu16 ", ID_VERSION=%" PRIu16
> +					"}", id.product, id.version);
> +			} else {
> +				tprints(", ...}");
> +			}
> +			return 1;

This EVIOCGID parser is long enough to be moved to a separate local function.

> +		case EVIOCGREP:
> +			if (!verbose(tcp) || umove(tcp, arg, &val) < 0)
> +				return 0;
> +
> +			tprintf(", [delay=%" PRIu32 ", period=%" PRIu32 "]",
> +				val[0], val[1]);
> +				return 1;
> +		case _IOC_NR(EVIOCSFF):
> +			return ff_effect_ioctl(tcp, arg);
> +	}
> +
> +	switch (_IOC_NR(code)) {
> +		case _IOC_NR(EVIOCGMTSLOTS(0)):
> +			if (!verbose(tcp) || umove(tcp, arg, &mtslots) < 0)
> +				return 0;
> +
> +			if (!mtslots.values) {
> +				tprints(", [ 0 ]");
> +				return 1;
> +			}
> +
> +			tprints(", {code=");
> +			printxval(evdev_mtslots, mtslots.code, "error");
> +			tprints(", values={");
> +			values_size = _IOC_SIZE(code) - sizeof(uint32_t);
> +			passed = 0;
> +			for(i = 0; i < (int)(values_size / sizeof(int32_t)); i++) {

A simpler method to iterate over mtslots.values[] is to use
ARRAY_SIZE(mtslots.values) instead of values_size/sizeof(int32_t).

> +				tprintf("%s%d", passed ? ", " : "", mtslots.values[i]);
> +				passed = 1;
> +			}
> +			tprints("}");
> +			return 1;

This EVIOCGMTSLOTS parser deserves to be moved to a separate local
function.

> +static int evdev_write_ioctl(struct tcb *tcp, const unsigned int code, long arg)
> +{
> +	unsigned int val[2];
> +
> +	if (entering(tcp)) {

An opposite check like
	if (exiting(tcp))
		return 1;
	...
would result to better indented and therefore more readable code.

> +		if ((_IOC_NR(code) & ~ABS_MAX) == _IOC_NR(EVIOCSABS(0)))
> +			return abs_ioctl(tcp, arg);
> +
> +		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 keycode_ioctl(tcp, arg);
> +			case EVIOCSKEYCODE_V2:
> +				return keycode_V2_ioctl(tcp, arg);
> +			case EVIOCSFF:
> +				return ff_effect_ioctl(tcp, arg);
> +			case EVIOCRMFF:
> +			case EVIOCSCLOCKID:
> +			case EVIOCGRAB:
> +			case EVIOCREVOKE:
> +				tprints(", ");
> +				printnum_int(tcp, arg, "%u");
> +				return 1;
> +			default:
> +				return 0;
> +		}
> +	} else {
> +		return 1;
> +	}

If you decode on entering and always return 1 on exiting, which is the
right thing to do for _IOC_WRITE ioctls, you cannot return 0, that is,
you have to implement fallback decoding yourself.


-- 
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/20150217/10764174/attachment.bin>