[PATCH v3] Add decoding for parametered ioctls

[Dmitry V. Levin]

On Wed, Jan 21, 2015 at 09:46:46PM +0100, Gabriel Laskar wrote:
> Some ioctls from evdev, hiddev, mixer, uinput, spi and joystick are
> parametered by a size or a number that is variable. These changes add
> the display of these ones, and their parameters.
>
> * defs.h: Declare new function ioctl_decode_number.

This line should be
* defs.h (ioctl_decode_number): New prototype.

> * io.c (sys_ioctl): Add call to ioctl_decode_number.

This line should be
* io.c (sys_ioctl): Use ioctl_decode_number.
or even
* io.c (sys_ioctl): Use it.
if the next line is placed before it.

> * ioctl.c (ioctl_decode_number): Implement this new function.

This line should be
* ioctl.c (ioctl_decode_number): New function.

> * xlat/evdev_abs.in: new file.
> * xlat/evdev_ev.in: new file.

These sentences should start with a capital letter.

For more information about ChangeLog style, please refer to
https://www.gnu.org/prep/standards/html_node/Change-Logs.html

I suppose we will stop this practice of writing ChangeLog-style entries
in commit messages some day, but this won't happen today. :)

[...]
> --- a/ioctl.c
> +++ b/ioctl.c
> @@ -32,6 +32,11 @@
>  #include <asm/ioctl.h>
>  #include "xlat/ioctl_dirs.h"
>  
> +#include <linux/input.h>

Probably OK to include this header file unconditionally.
If any issues arises, they could be dealt with.

> +
> +#include "xlat/evdev_abs.h"
> +#include "xlat/evdev_ev.h"
> +
>  static int
>  compare(const void *a, const void *b)
>  {
> @@ -77,6 +82,142 @@ ioctl_print_code(const unsigned int code)
>  		_IOC_TYPE(code), _IOC_NR(code), _IOC_SIZE(code));
>  }
>  
> +static int
> +evdev_decode_number(unsigned int arg)
> +{
> +	unsigned int nr = _IOC_NR(arg);
> +
> +	if (_IOC_DIR(arg) == _IOC_WRITE) {
> +		if (nr >= 0xc0 && nr <= 0xc0 + 0x3f) {
> +			tprints("EVIOCSABS(");
> +			printxval(evdev_abs, nr - 0xc0, "invalid absolute axe");

There might be other reasons for this value not to be found in evdev_abs,
for example, the version of linux/input.h used to built strace had no
definition for this value.  That's why we are not so resolute in cases
like this.  Let's use a more neutral string like "ABS_???".

> +			tprints(")");
> +			return 1;
> +		}
> +	}
> +
> +	if (_IOC_DIR(arg) != _IOC_READ)
> +		return 0;
> +
> +	if (nr >= 0x20 && nr <= 0x20 + 0x1f) {
> +		tprints("EVIOCGBIT(");
> +		printxval(evdev_ev, nr - 0x20, "invalid event type");

Same here, let's use "EV_???" instead.

[...]
> +int
> +ioctl_decode_command_number(unsigned int arg)
> +{
> +	switch (_IOC_TYPE(arg)) {
> +		case 'E':
> +			return evdev_decode_number(arg);
> +		case 'H':
> +			return hiddev_decode_number(arg);
> +		case 'M':
> +			if (_IOC_DIR(arg) == _IOC_WRITE) {
> +				tprintf("MIXER_WRITE(%u)", _IOC_NR(arg));
> +			} else if (_IOC_DIR(arg) == _IOC_READ) {
> +				tprintf("MIXER_READ(%u)", _IOC_NR(arg));
> +			}
> +			return 1;

What if _IOC_DIR(arg) is neither _IOC_WRITE nor _IOC_READ?
Please do "return 1;" consistently.


-- 
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/20150122/cbdbe455/attachment.bin>