[PATCH v3] Add decoding for evdev ioctls.
Dmitry V. Levin
ldv at altlinux.org
Tue Feb 17 01:08:21 UTC 2015
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>
More information about the Strace-devel
mailing list