<p></p>
<blockquote>
<p dir="auto">A copy of an e-mail with <code>Message-ID: <20220204061024.GA10415@obsidian></code>[1] that for whatever reason hasn't been forwarded to github:</p>
<pre><code>On Thu, Feb 03, 2022 at 07:51:18AM -0800, Sean Young via Strace-devel wrote:
> Ready for the next round of review
On Wed, Jan 26, 2022 at 10:22:01AM +0000, Sean Young wrote:
> Signed-off-by: Sean Young <sean@mess.org>
> ---
> NEWS | 1 +
> bundled/Makefile.am | 1 +
> bundled/linux/include/uapi/linux/lirc.h | 232 ++++++++++++++++++++++++++++++++
> src/Makefile.am | 1 +
> src/defs.h | 1 +
> src/ioctl.c | 2 +
> src/lirc.c | 46 +++++++
> src/xlat/lirc_features.in | 13 ++
> src/xlat/lirc_modes.in | 6 +
> tests/.gitignore | 2 +
> tests/Makefile.am | 1 +
> tests/gen_tests.in | 2 +
> tests/ioctl_lirc-success.c | 2 +
> tests/ioctl_lirc.c | 173 ++++++++++++++++++++++++
> tests/pure_executables.list | 1 +
> 15 files changed, 484 insertions(+)
> create mode 100644 bundled/linux/include/uapi/linux/lirc.h
> create mode 100644 src/lirc.c
> create mode 100644 src/xlat/lirc_features.in
> create mode 100644 src/xlat/lirc_modes.in
> create mode 100644 tests/ioctl_lirc-success.c
> create mode 100644 tests/ioctl_lirc.c
>
> diff --git a/NEWS b/NEWS
> index 6c8f859..c933d11 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -12,6 +12,7 @@ Noteworthy changes in release ?.?? (????-??-??)
> * Implemented decoding of PR_SET_VMA operation of prctl syscall.
> * Updated lists of FAN_*, IORING_*, IOSQE_*, KVM_*, MODULE_INIT_*, TCA_ACT_*,
> and *_MAGIC constants.
> + * Implemented decoding of lirc ioctl commands.
"LIRC", maybe? I've thought it's an abbreviation.
</code></pre>
</blockquote>
<p dir="auto">Fixed.</p>
<blockquote>
<blockquote>
<p dir="auto">diff --git a/src/Makefile.am b/src/Makefile.am<br>
index dc9fd93..c000593 100644<br>
--- a/src/Makefile.am<br>
+++ b/src/Makefile.am<br>
@@ -184,6 +184,7 @@ libstrace_a_SOURCES = <br>
link.c <br>
linux/x32/asm_stat.h <br>
linux/x86_64/asm_stat.h \</p>
<ul dir="auto">
<li>lirc.c \</li>
</ul>
</blockquote>
<p dir="auto">I think for ioctl interfaces the naming is *_ioctl.c as of late,<br>
but I have no strong preference here (and there are files like dm.c).</p>
</blockquote>
<p dir="auto">Rernamed to lirc_ioctl.c</p>
<blockquote>
<blockquote>
<p dir="auto">diff --git a/src/lirc.c b/src/lirc.c<br>
new file mode 100644<br>
index 0000000..6459dc2<br>
--- /dev/null<br>
+++ b/src/lirc.c<br>
@@ -0,0 +1,46 @@<br>
+/*</p>
<ul dir="auto">
<li>
<ul dir="auto">
<li>Copyright (c) 2022 Sean Young <a href="mailto:sean@mess.org">sean@mess.org</a></li>
</ul>
</li>
<li>
<ul dir="auto">
<li></li>
</ul>
</li>
<li>
<ul dir="auto">
<li>SPDX-License-Identifier: LGPL-2.1-or-later</li>
</ul>
</li>
<li>*/</li>
<li></li>
</ul>
<p dir="auto">+#include "defs.h"<br>
+#include <linux/lirc.h><br>
+#include "xlat/lirc_features.h"<br>
+#include "xlat/lirc_modes.h"<br>
+<br>
+int<br>
+lirc_ioctl(struct tcb *const tcp, const unsigned int code,</p>
<ul dir="auto">
<li>
<pre><code> const kernel_ulong_t arg)
</code></pre>
</li>
</ul>
<p dir="auto">+{</p>
<ul dir="auto">
<li>unsigned int value;</li>
<li></li>
</ul>
</blockquote>
<blockquote>
<ul dir="auto">
<li>if (_IOC_DIR(code) == _IOC_READ && entering(tcp))</li>
<li>
<pre><code> return 0;
</code></pre>
</li>
<li></li>
<li>tprint_arg_next();</li>
<li></li>
</ul>
</blockquote>
<p dir="auto">It is nicer to return after printing the comma (even though it leads<br>
to a bit more ugly code), as this leads to</p>
<pre><code>scname(arg1, <unfinished...>
<resumed scname> arg2...
</code></pre>
<p dir="auto">output instead of</p>
<pre><code>scname(arg1<unfinished>
<resumed scname> , arg2...
</code></pre>
</blockquote>
<p dir="auto">If I tried to fix this, the output of:</p>
<pre><code> do_ioctl(LIRC_GET_FEATURES, (unsigned int*)0x40000);
</code></pre>
<p dir="auto">becomes</p>
<pre><code>ioctl(-1, LIRC_GET_FEATURES, , 0x40000)
</code></pre>
<p dir="auto">Which we don't want.</p>
<blockquote>
<blockquote>
<ul dir="auto">
<li>switch (code) {</li>
<li>case LIRC_GET_FEATURES:</li>
<li>
<pre><code> printflags(lirc_features, value, "LIRC_CAN_???");
</code></pre>
</li>
<li>
<pre><code> break;
</code></pre>
</li>
<li>case LIRC_GET_SEND_MODE:</li>
<li>case LIRC_GET_REC_MODE:</li>
<li>case LIRC_SET_SEND_MODE:</li>
<li>case LIRC_SET_REC_MODE:</li>
<li>
<pre><code> printxval(lirc_modes, value, "LIRC_MODE_???");
</code></pre>
</li>
<li>
<pre><code> break;
</code></pre>
</li>
<li>default:</li>
<li>
<pre><code> PRINT_VAL_U(value);
</code></pre>
</li>
</ul>
</blockquote>
<p dir="auto">The main issue here is that this code assumes that only LIRC_* ioctls<br>
have 'i' IOC type, which is generally not true (it is shared with i8k,<br>
IPMI, IIO and a couple of i915 ioctl commands). This can be approached<br>
either by performing early filtering by ioctl cmd (like it is done in<br>
dm), or by explicitly listing all supported ioctls in the decoding switch<br>
statements and returning RVAL_DECODED in the default case (as it is done<br>
in seccomp and kd ioctl decoders, for example). Both options will lead<br>
to some duplication, though, either with listing all of the LIRC<br>
commands, or re-factoring code to handle tracee memory reading and<br>
printing inside</p>
</blockquote>
<p dir="auto">I've added an early switch like dm does.</p>
<blockquote>
<p dir="auto">I think that at least LIRC_SET_TRANSMITTER_MASK command argument is worth<br>
printing in hexadecimal and not decimal as it seems to be a bit mask.</p>
</blockquote>
<p dir="auto">Fixed. Binary would be nicer than hex though.</p>
<blockquote>
<p dir="auto">There is a set of functions for printing an integer referenced<br>
by an address, printnum_*, but I'm not sure if it is worth using here.</p>
<blockquote>
<p dir="auto">diff --git a/src/xlat/lirc_features.in b/src/xlat/lirc_features.in<br>
new file mode 100644<br>
index 0000000..6bd45a4<br>
--- /dev/null<br>
+++ b/src/xlat/lirc_features.in<br>
@@ -0,0 +1,13 @@<br>
+#unconditional<br>
+LIRC_CAN_REC_SCANCODE<br>
+LIRC_CAN_REC_MODE2<br>
+LIRC_CAN_GET_REC_RESOLUTION<br>
+LIRC_CAN_SEND_PULSE<br>
+LIRC_CAN_SET_TRANSMITTER_MASK<br>
+LIRC_CAN_SET_SEND_CARRIER<br>
+LIRC_CAN_SET_SEND_DUTY_CYCLE<br>
+LIRC_CAN_SET_REC_CARRIER<br>
+LIRC_CAN_SET_REC_CARRIER_RANGE<br>
+LIRC_CAN_USE_WIDEBAND_RECEIVER<br>
+LIRC_CAN_MEASURE_CARRIER<br>
+LIRC_CAN_SET_REC_TIMEOUT</p>
</blockquote>
<p dir="auto">I'm unable grok the order of this xlat; usually, flags are sorted from the<br>
LSB to MSB, unless there are reasons to have a different order (usually,<br>
when a symbolic value represents a combination of flags).</p>
</blockquote>
<p dir="auto">There was no order. I've ordered them.</p>
<blockquote>
<p dir="auto">There are also<br>
LIRC_CAN_SEND_RAW, LIRC_CAN_SEND_MODE2, LIRC_CAN_SEND_SCANCODE,<br>
LIRC_CAN_SEND_LIRCCODE, LIRC_CAN_REC_RAW, LIRC_CAN_REC_PULSE,<br>
LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SET_REC_FILTER, LIRC_CAN_GET_REC_RESOLUTION,<br>
and LIRC_CAN_SET_REC_DUTY_CYCLE_RANGE flags seem to be missing.</p>
</blockquote>
<p dir="auto">I've added LIRC_CAN_REC_LIRCCODE, LIRC_CAN_SEND_LIRCCODE (may be used in<br>
pre-4.16 kernels) and LIRC_CAN_GET_REC_RESOLUTION was just missing. The other<br>
onces were never implemented, out-of-tree or not. They should have never<br>
been added to the header in the first place.</p>
<blockquote>
<blockquote>
<p dir="auto">diff --git a/tests/ioctl_lirc.c b/tests/ioctl_lirc.c<br>
new file mode 100644<br>
index 0000000..6f7fdc0<br>
--- /dev/null<br>
+++ b/tests/ioctl_lirc.c</p>
</blockquote>
<blockquote>
<ul dir="auto">
<li>value = 12000;</li>
<li>do_ioctl(LIRC_SET_REC_TIMEOUT, &value);</li>
<li>printf("ioctl(-1, %s, [12000]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_SET_REC_TIMEOUT), errstr);
</code></pre>
</li>
</ul>
</blockquote>
<p dir="auto">XLAT_STR macro is used for generating a string in accordance with<br>
XLAT_VERBOSE and XLAT_RAW macro constant settings, which are used<br>
for testing output of strace with different xlat verbosity settings (see<br>
*-X{abbrev,raw,verbose}.c tests for an example), otherwise it is<br>
useless. On the other hand, having such tests would be nice<br>
for a decoder that prints named constants, even though xlat usage<br>
in the lirc decoder currently rather trivial.</p>
</blockquote>
<p dir="auto">Removed XLAT_STR()</p>
<blockquote>
<p dir="auto">It is also worth checking strace behaviour when the memory can't<br>
be accessed or the pointer is NULL.</p>
</blockquote>
<p dir="auto">Added tests.</p>
<blockquote>
<blockquote>
<ul dir="auto">
<li>value = 40000;</li>
<li>do_ioctl(LIRC_SET_REC_CARRIER_RANGE, &value);</li>
<li>printf("ioctl(-1, IPMICTL_SET_MAINTENANCE_MODE_CMD or %s, [40000]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_SET_REC_CARRIER_RANGE), errstr);
</code></pre>
</li>
</ul>
</blockquote>
<p dir="auto">This will generate incorrect if ether XLAT_VERBOSE or XLAT_RAW are set.</p>
<p dir="auto">Also, this output probably signifies the issue with assuming that<br>
every command is a LIRC command, as IPMICTL_SET_MAINTENANCE_MODE_CMD<br>
argument is a named constant (IPMI_MAINTENANCE_MODE_*). But, alas,<br>
strace doesn't currently implement capabilities that would allow<br>
to narrow down the ioctl cmd selection based on ioctl fd.</p>
</blockquote>
<p dir="auto">Alas.</p>
<blockquote>
<blockquote>
<ul dir="auto">
<li>value = 2;</li>
<li>do_ioctl(LIRC_SET_SEND_MODE, &value);</li>
<li>printf("ioctl(-1, %s, [LIRC_MODE_PULSE]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_SET_SEND_MODE), errstr);
</code></pre>
</li>
</ul>
</blockquote>
<p dir="auto">Either LIRC_MODE_PULSE is to be wrapped within XLAT_STR in order to<br>
enable proper string generation in different xla modes, or XLAT_STR<br>
usage for LIRC_SET_SEND_MODE is redundant.</p>
</blockquote>
<p dir="auto">XLAT_STR() removed.</p>
<blockquote>
<p dir="auto">Also, it is worth checking<br>
that 3 (and probably some other values) is decoded as an unknown mode<br>
and not a combination of LIRC_MODE_RAW and LIRC_MODE_PULSE: it is not<br>
uncommon to mix up printxlat and printflags usage.</p>
</blockquote>
<p dir="auto">Add tests for this.</p>
<blockquote>
<p dir="auto">Also, LIRC_SET_TRANSMITTER_MASK doesn't seem to be checked.</p>
<blockquote>
<ul dir="auto">
<li>// read ioctl</li>
</ul>
</blockquote>
<p dir="auto">C++-style comments are not generally used in strace code.</p>
</blockquote>
<p dir="auto">I've changed to 80s style comments.</p>
<blockquote>
<p dir="auto">Also, it is worth checking decoding of the read ioctl command when they<br>
can't read (due to non-zero syscall return value) as well, to avoid accidentally<br>
decoding of an argument on entering when it is supposed to be decoded in exiting.</p>
</blockquote>
<p dir="auto">I think that is tested now.</p>
<blockquote>
<blockquote>
<p dir="auto">+#ifdef INJECT_RETVAL</p>
<ul dir="auto">
<li>value = LIRC_CAN_SEND_PULSE | LIRC_CAN_SET_SEND_DUTY_CYCLE | LIRC_CAN_GET_REC_RESOLUTION | LIRC_CAN_USE_WIDEBAND_RECEIVER;</li>
<li>do_ioctl(LIRC_GET_FEATURES, &value);</li>
<li>printf("ioctl(-1, %s, [LIRC_CAN_GET_REC_RESOLUTION|LIRC_CAN_SEND_PULSE|LIRC_CAN_SET_SEND_DUTY_CYCLE|LIRC_CAN_USE_WIDEBAND_RECEIVER]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_FEATURES), errstr);
</code></pre>
</li>
</ul>
</blockquote>
<p dir="auto">For flag sets, it is also worth checking how 0 or unknown flags are<br>
decoded.</p>
</blockquote>
<p dir="auto">Tests added.</p>
<blockquote>
<p dir="auto">Also, these lines are over-length, strace source code tends to keep<br>
line length under 80.</p>
</blockquote>
<p dir="auto">Fixed.</p>
<blockquote>
<blockquote>
<ul dir="auto">
<li>value = 1;</li>
<li>do_ioctl(LIRC_GET_REC_MODE, &value);</li>
<li>printf("ioctl(-1, %s, [LIRC_MODE_RAW]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_REC_MODE), errstr);
</code></pre>
</li>
<li></li>
<li>do_ioctl(LIRC_GET_SEND_MODE, &value);</li>
<li>printf("ioctl(-1, %s, [LIRC_MODE_PULSE]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_SEND_MODE), errstr);
</code></pre>
</li>
<li></li>
<li>value = 120;</li>
<li>do_ioctl(LIRC_GET_REC_RESOLUTION, &value);</li>
<li>printf("ioctl(-1, %s, [120]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_REC_RESOLUTION), errstr);
</code></pre>
</li>
<li></li>
<li>value = 120000;</li>
<li>do_ioctl(LIRC_GET_REC_TIMEOUT, &value);</li>
<li>printf("ioctl(-1, %s, [120000]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_REC_TIMEOUT), errstr);
</code></pre>
</li>
<li></li>
<li>value = 1100;</li>
<li>do_ioctl(LIRC_GET_MIN_TIMEOUT, &value);</li>
<li>printf("ioctl(-1, I2OVALIDATE or %s, [1100]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_MIN_TIMEOUT), errstr);
</code></pre>
</li>
<li></li>
<li>value = 10100;</li>
<li>do_ioctl(LIRC_GET_MAX_TIMEOUT, &value);</li>
<li>printf("ioctl(-1, %s, [10100]) = %s\n",</li>
<li>
<pre><code> XLAT_STR(LIRC_GET_MAX_TIMEOUT), errstr);
</code></pre>
</li>
</ul>
</blockquote>
<p dir="auto">LIRC_GET_LENGTH commands doesn't seem to be checked.</p>
</blockquote>
<p dir="auto">Yes, this is true. LIRC_GET_LENGTH is not a thing anymore since kernel v4.16,<br>
however I've added a test.</p>
<p style="font-size:small;-webkit-text-size-adjust:none;color:#666;">—<br />Reply to this email directly, <a href="https://github.com/strace/strace/pull/208#issuecomment-1031807778">view it on GitHub</a>, or <a href="https://github.com/notifications/unsubscribe-auth/AOVBTR7TDJZN5DJMT3U73JTU2AI3ZANCNFSM5M2WOHAQ">unsubscribe</a>.<br />Triage notifications on the go with GitHub Mobile for <a href="https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675">iOS</a> or <a href="https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub">Android</a>.
<br />You are receiving this because you are subscribed to this thread.<img src="https://github.com/notifications/beacon/AOVBTR727S4YVWLTUTGP6PLU2AI3ZA5CNFSM5M2WOHA2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOHWACGIQ.gif" height="1" width="1" alt="" /><span style="color: transparent; font-size: 0; display: none; visibility: hidden; overflow: hidden; opacity: 0; width: 0; height: 0; max-width: 0; max-height: 0; mso-hide: all">Message ID: <span><strace/strace/pull/208/c1031807778</span><span>@</span><span>github</span><span>.</span><span>com></span></span></p>
<script type="application/ld+json">[
{
"@context": "http://schema.org",
"@type": "EmailMessage",
"potentialAction": {
"@type": "ViewAction",
"target": "https://github.com/strace/strace/pull/208#issuecomment-1031807778",
"url": "https://github.com/strace/strace/pull/208#issuecomment-1031807778",
"name": "View Pull Request"
},
"description": "View this Pull Request on GitHub",
"publisher": {
"@type": "Organization",
"name": "GitHub",
"url": "https://github.com"
}
}
]</script>