Add lirc ioctl decoding #208
Conversation
src/lirc.c
Outdated
| static const struct features { | ||
| int flag; | ||
| const char *name; | ||
| } features[] = { | ||
| { LIRC_CAN_REC_SCANCODE, "LIRC_CAN_REC_SCANCODE" }, | ||
| { LIRC_CAN_REC_MODE2, "LIRC_CAN_REC_MODE2" }, | ||
| { LIRC_CAN_GET_REC_RESOLUTION, "LIRC_CAN_GET_REC_RESOLUTION" }, | ||
| { LIRC_CAN_SEND_PULSE, "LIRC_CAN_SEND_PULSE" }, | ||
| { LIRC_CAN_SET_TRANSMITTER_MASK, "LIRC_CAN_SET_TRANSMITTER_MASK" }, | ||
| { LIRC_CAN_SET_SEND_CARRIER, "LIRC_CAN_SET_SEND_CARRIER" }, | ||
| { LIRC_CAN_SET_SEND_DUTY_CYCLE, "LIRC_CAN_SET_SEND_DUTY_CYCLE" }, | ||
| { LIRC_CAN_SET_REC_CARRIER, "LIRC_CAN_SET_REC_CARRIER" }, | ||
| { LIRC_CAN_SET_REC_CARRIER_RANGE, "LIRC_CAN_SET_REC_CARRIER_RANGE" }, | ||
| { LIRC_CAN_USE_WIDEBAND_RECEIVER, "LIRC_CAN_USE_WIDEBAND_RECEIVER" }, | ||
| { LIRC_CAN_MEASURE_CARRIER, "LIRC_CAN_MEASURE_CARRIER" }, | ||
| { LIRC_CAN_SET_REC_TIMEOUT, "LIRC_CAN_SET_REC_TIMEOUT" }, | ||
| }; |
There was a problem hiding this comment.
We replaced this approach with xlat files roughly 8 years ago, please create a xlat file instead.
There was a problem hiding this comment.
I've done this using xlat now. This is much nicer
There was a problem hiding this comment.
Given that linux/lirc.h is bundled, there is no need to test LIRC_* constants,
so please add #unconditional to these xlat files.
src/lirc.c
Outdated
| switch (code) { | ||
| case LIRC_GET_FEATURES: | ||
| printflags(lirc_features, value, "LIRC_CAN_???"); | ||
| break; | ||
| case LIRC_GET_SEND_MODE: | ||
| case LIRC_GET_REC_MODE: | ||
| case LIRC_SET_SEND_MODE: | ||
| case LIRC_SET_REC_MODE: | ||
| printxval(lirc_modes, value, "LIRC_MODE_???"); | ||
| break; | ||
| default: | ||
| PRINT_VAL_D(value); | ||
| break; | ||
| } |
There was a problem hiding this comment.
Since value is obtained from tracee's memory, its printing should be wrapped with
tprint_indirect_begin();
...
tprint_indirect_end();
See e.g. decode_fs_ioc_flags as an example.
Also, tprint_arg_next(); should precede any printing, this includes umove_or_printaddr.
|
Ready for review |
tests/ioctl_lirc.c
Outdated
| printf("ioctl(-1, %s, [LIRC_MODE_LIRCCODE]) = %s\n", | ||
| XLAT_STR(LIRC_SET_REC_MODE), errstr); | ||
|
|
||
| // read ioctl |
There was a problem hiding this comment.
/* read ioctls */
To make the test work in case of no syscall tampering,
the return value of do_ioctl should be tested and acted appropriately,
see, for eexample, tests/ioctl_gpio.c
also, please add do_ioctl(LIRC_GET_FEATURES, 0); or something like this to cover the true branch of if (umove_or_printaddr(tcp, arg, &value)).
There was a problem hiding this comment.
OK, there is ioctl(-1, LIRC_GET_FEATURES, 0) in INJECT_RETVAL variant, it's probably enough.
At least the coverage test reports that all lines are covered.
There was a problem hiding this comment.
I've added this case. Please let me know what you think.
Codecov Report
@@ Coverage Diff @@
## master #208 +/- ##
=======================================
Coverage 89.96% 89.96%
=======================================
Files 287 288 +1
Lines 24049 24073 +24
=======================================
+ Hits 21635 21658 +23
- Misses 2414 2415 +1
Continue to review full report at Codecov.
|
|
I've also changed the decoding to printing the unsigned value. That's how the kernel interprets the value. |
tests/Makefile.am
Outdated
| @@ -200,6 +200,8 @@ check_PROGRAMS = $(PURE_EXECUTABLES) \ | |||
| ioctl_kd-success-s1024-Xabbrev \ | |||
| ioctl_kd-success-s1024-Xraw \ | |||
| ioctl_kd-success-s1024-Xverbose \ | |||
| ioctl_lirc \ | |||
There was a problem hiding this comment.
ioctl_lirc is redundant in this list because it's listed in pure_executables.list;
ioctl_lirc-success has to be kept here, see below.
tests/pure_executables.list
Outdated
| @@ -209,6 +209,8 @@ ioctl_hdio-v-Xabbrev | |||
| ioctl_hdio-v-Xraw | |||
| ioctl_hdio-v-Xverbose | |||
| ioctl_inotify | |||
| ioctl_lirc | |||
| ioctl_lirc-success | |||
There was a problem hiding this comment.
*-success executables shouldn't be listed here because they are not pure enough.
|
Ready for the next round of review |
|
A copy of an e-mail with [1] https://lists.strace.io/pipermail/strace-devel/2022-February/010957.html |
Fixed.
Rernamed to lirc_ioctl.c
If I tried to fix this, the output of: becomes Which we don't want.
I've added an early switch like dm does.
Fixed. Binary would be nicer than hex though.
There was no order. I've ordered them.
I've added LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SEND_LIRCCODE (may be used in
Removed XLAT_STR()
Added tests.
Alas.
XLAT_STR() removed.
Add tests for this.
I've changed to 80s style comments.
I think that is tested now.
Tests added.
Fixed.
Yes, this is true. LIRC_GET_LENGTH is not a thing anymore since kernel v4.16, |
|
Ready for review again. |
|
On Mon, Feb 07, 2022 at 10:58:01AM -0800, Sean Young wrote:
[...]
> > + if (_IOC_DIR(code) == _IOC_READ && entering(tcp))
> > + return 0;
> > +
> > + tprint_arg_next();
> > +
>
> It is nicer to return after printing the comma (even though it leads
> to a bit more ugly code), as this leads to
>
> scname(arg1, <unfinished...>
> <resumed scname> arg2...
>
> output instead of
>
> scname(arg1<unfinished>
> <resumed scname> , arg2...
If I tried to fix this, the output of:
```
do_ioctl(LIRC_GET_FEATURES, (unsigned int*)0x40000);
```
becomes
```
ioctl(-1, LIRC_GET_FEATURES, , 0x40000)
```
Which we don't want.
Could you try this:
if (entering(tcp)) {
tprint_arg_next();
if (_IOC_DIR(code) == _IOC_READ)
return 0;
}
|
That works, done. |
|
On Mon, Feb 07, 2022 at 11:00:43AM -0800, Sean Young via Strace-devel wrote:
Ready for review again.
Looks great, thank you.
diff --git a/tests/ioctl_lirc.c b/tests/ioctl_lirc.c
new file mode 100644
index 0000000..df601e9
--- /dev/null
+++ b/tests/ioctl_lirc.c
+ value = 40000;
+ do_ioctl(LIRC_SET_REC_CARRIER_RANGE, &value);
+ printf("ioctl(-1, IPMICTL_SET_MAINTENANCE_MODE_CMD or LIRC_SET_REC_CARRIER_RANGE, [40000]) = %s\n",
+ errstr);
In strace, string literals tend to be wrapped to be under 80 columns
as well:
printf("ioctl(-1, IPMICTL_SET_MAINTENANCE_MODE_CMD"
" or LIRC_SET_REC_CARRIER_RANGE, [40000]) = %s\n",
errstr);
+ printf("ioctl(-1, LIRC_GET_FEATURES, [LIRC_CAN_SEND_PULSE|LIRC_CAN_SET_SEND_DUTY_CYCLE|LIRC_CAN_USE_WIDEBAND_RECEIVER|LIRC_CAN_GET_REC_RESOLUTION|0x8000000]) = %s\n",
+ errstr);
printf("ioctl(-1, LIRC_GET_FEATURES, [LIRC_CAN_SEND_PULSE"
"|LIRC_CAN_SET_SEND_DUTY_CYCLE|LIRC_CAN_USE_WIDEBAND_RECEIVER"
"|LIRC_CAN_GET_REC_RESOLUTION|0x8000000]) = %s\n",
errstr);
+ do_ioctl(LIRC_GET_FEATURES, (unsigned int*)0x40000);
It is usually formatted as follows in strace:
do_ioctl(LIRC_GET_FEATURES, (unsigned int *) 0x40000);
Since these are just minor formatting nits,
Reviewed-by: Eugene Syromyatnikov ***@***.***>
…
--
Reply to this email directly or view it on GitHub:
#208 (comment)
You are receiving this because you are subscribed to this thread.
Message ID: ***@***.***>
--
Strace-devel mailing list
***@***.***
https://lists.strace.io/mailman/listinfo/strace-devel
|
|
On Mon, Feb 07, 2022 at 10:58:01AM -0800, Sean Young wrote:
> There are also
> LIRC_CAN_SEND_RAW, LIRC_CAN_SEND_MODE2, LIRC_CAN_SEND_SCANCODE,
> LIRC_CAN_SEND_LIRCCODE, LIRC_CAN_REC_RAW, LIRC_CAN_REC_PULSE,
> LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SET_REC_FILTER, LIRC_CAN_GET_REC_RESOLUTION,
> and LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE flags seem to be missing.
I've added LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SEND_LIRCCODE (may be used in
pre-4.16 kernels) and LIRC_CAN_GET_REC_RESOLUTION was just missing.
The other
onces were never implemented, out-of-tree or not. They should have never
been added to the header in the first place.
Generally it is worth adding such constants to the xlat file in
comments (with a reason why they were omitted), so those who are looking
at it afterwards won't spend too much time to figure out they are
missing (as it is not pointed out in the kernel header explicitly, only
in the documentation).
> LIRC_GET_LENGTH commands doesn't seem to be checked.
Yes, this is true. LIRC_GET_LENGTH is not a thing anymore since kernel v4.16,
however I've added a test.
strace is expected to be compatible with a range of kernel versions
(as old as 2.6.18, supposedly).
|
I'm (slowly) removing these entries, see https://www.spinics.net/lists/linux-media/msg206895.html and https://git.linuxtv.org/media_stage.git/commit/?id=b2a90f4fcb146d0e033203ab646f0fd22cfa947f |
I've fixed the formatting issues and added your Reviewed-by: tag. Thanks! |
* NEWS: Mention this change. * bundled/linux/include/uapi/linux/lirc.h: New file. * bundled/Makefile.am (EXTRA_DIST): Add it. * src/lirc_ioctl.c: New file. * src/Makefile.am (libstrace_a_SOURCES): Add it. * src/defs.h (DECL_IOCTL(lirc)): New declaration. * src/ioctl.c (ioctl_decode) <case 'i'>: Call kd_ioctl. * src/xlat/lirc_features.in: New file. * src/xlat/lirc_modes.in: Likewise. * tests/ioctl_lirc.c: New file. * tests/ioctl_lirc-success.c: Likewise. * tests/Makefile.am (check_PROGRAMS): Add ioctl_lirc-success. * tests/gen_tests.in (ioctl_lirc, ioctl_lirc-success): New tests. * tests/pure_executables.list: Add ioctl_lirc. * tests/.gitignore: Add ioctl_lirc and ioctl_lirc-success. Reviewed-by: Eugene Syromyatnikov <evgsyr@gmail.com> Signed-off-by: Sean Young <sean@mess.org> Reviewed-by: Dmitry V. Levin <ldv@strace.io>
it would be useful to decode read/write on the chardev too, but this would require tracking what lirc mode the file is in (from the ioctls), and having a hook on read/write. Not sure that is something strace can do.