[PATCH i-g-t v3 07/10] runner: Fix use of newline on arguments
Gustavo Sousa
gustavo.sousa at intel.com
Fri Feb 7 20:15:26 UTC 2025
Quoting Lucas De Marchi (2025-02-03 15:09:04-03:00)
>Currently there's no support for newlines on arguments passed to runner.
>However it's also a silent failure:
>
> # igt_runner --test-list '/tmp/test
> list2.txt' build/tests/ /tmp/results
>
> # head /tmp/results/metadata.txt
> disk_usage_limit : 0
> test_list : /tmp/test
> list2.txt
> name : results
> ...
>
> # ./build/runner/igt_resume /tmp/results
> [9840425.334900] All tests already executed.
> resume failed at generating results
> Done.
>
>Embedding a newline like this is very dubious for test-list, but it's
>used for e.g. hooks. In future we will add the command line to the
>metadata and possibly migrate the hooks, so add support for
>escaping/unescaping the string on save/restore.
>
>The method chosen is slightly different than the one used for hooks:
>instead of adding a escape char and keeping the char escaped, this just
>prefers using an hex representation of the char with a \x<HEX>h
>sequence. This makes it easier when unescaping since the reader can
>continue reading one line per iteration. In future this can also be
>adopted by the hooks or even migrating the hooks to use metadata.txt.
>
>Another fix is that now we just skip null values on the serialization
>side. Previously it would serialize "(null)" and then load that string
>instead of NULL. Add code_coverage_script to the runner_test to cover
>that, which would previously fail.
>
>Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>---
>
>- Fix NULL string (bug already present)
>- Add sanity check on the unescape_str() side to differentiate invalid
> from empty strings. Now unescape_str() returns negative in cases of
> error and there's better validation for the escaped char
>
> runner/runner_tests.c | 1 +
> runner/settings.c | 85 +++++++++++++++++++++++++++++++++++++++++--
> 2 files changed, 83 insertions(+), 3 deletions(-)
>
>diff --git a/runner/runner_tests.c b/runner/runner_tests.c
>index 8441763f2..93b3ebc9f 100644
>--- a/runner/runner_tests.c
>+++ b/runner/runner_tests.c
>@@ -200,6 +200,7 @@ static void assert_settings_equal(struct settings *one, struct settings *two)
> igt_assert_eq(one->use_watchdog, two->use_watchdog);
> igt_assert_eqstr(one->test_root, two->test_root);
> igt_assert_eqstr(one->results_path, two->results_path);
>+ igt_assert_eqstr(one->code_coverage_script, two->code_coverage_script);
> igt_assert_eq(one->piglit_style_dmesg, two->piglit_style_dmesg);
> igt_assert_eq(one->dmesg_warn_level, two->dmesg_warn_level);
> igt_assert_eq(one->prune_mode, two->prune_mode);
>diff --git a/runner/settings.c b/runner/settings.c
>index 693c5484e..03402a385 100644
>--- a/runner/settings.c
>+++ b/runner/settings.c
>@@ -7,6 +7,7 @@
> #include <errno.h>
> #include <fcntl.h>
> #include <getopt.h>
>+#include <inttypes.h>
> #include <libgen.h>
> #include <limits.h>
> #include <stdio.h>
>@@ -1052,10 +1053,80 @@ static bool serialize_hook_strs(struct settings *settings, int dirfd)
> return true;
> }
>
>+/*
>+ * Serialize @s to @f, escaping '\' and '\n'. See unescape_str()
>+ */
>+static void escape_str(const char *s, FILE *f)
>+{
>+ while (*s) {
>+ size_t len = strcspn(s, "\\\n");
>+
>+ if (len > 0) {
>+ fwrite(s, len, 1, f);
>+ s += len;
>+ }
>+
>+ if (*s) {
>+ fprintf(f, "\\x%xh", *s);
I was thinking we could have used %02x and avoided the "h" suffix... But
I guess the "h" suffix makes the unescape logic simpler.
>+ s++;
>+ }
>+ }
>+}
>+
>+/*
>+ * Unescape a '\' and '\n': undo escape_str
>+ *
>+ * Exacpe chars using the form '\x<hex>h' so they don't interfere with the line
Exacpe?
>+ * parser.
>+ *
>+ * Return the number of chars saved in buf and optionally
>+ * the number of chars scanned in @n_src if that is non-nul.
>+ */
>+static ssize_t unescape_str(char *buf, size_t *n_src)
>+{
>+ size_t dst_len = 0;
>+ char *s = buf;
>+
>+ while (*s) {
>+ char next = *(s + 1);
>+
>+ if (*s != '\\') {
>+ buf[dst_len++] = *s++;
>+ } else if (next == 'x') {
>+ unsigned long num;
>+
>+ s += 2;
>+
>+ num = strtoul(s, &s, 16);
Well, I think strtoul() is too permissive here, it allows the str to
start with whitespaces and some prefixes that are not generated by
escape_str().
But I guess that's acceptable if we only care about unescape_str() being
able to process what was generated by escape_str(), as you commented
earlier.
Reviewed-by: Gustavo Sousa <gustavo.sousa at intel.com>
>+ /* cover both error due to overflow or invalid char */
>+ if (num > UINT8_MAX || *s != 'h')
>+ return -EINVAL;
>+
>+ buf[dst_len++] = num;
>+ s++;
>+ } else {
>+ return -EINVAL;
>+ }
>+ }
>+
>+ buf[dst_len] = '\0';
>+
>+ if (n_src)
>+ *n_src = s - buf;
>+
>+ return dst_len;
>+}
>+
> #define SERIALIZE_LINE(f, s, name, fmt) fprintf(f, "%s : " fmt "\n", #name, s->name)
> #define SERIALIZE_INT(f, s, name) SERIALIZE_LINE(f, s, name, "%d")
> #define SERIALIZE_UL(f, s, name) SERIALIZE_LINE(f, s, name, "%lu")
>-#define SERIALIZE_STR(f, s, name) SERIALIZE_LINE(f, s, name, "%s")
>+#define SERIALIZE_STR(f, s, name) do { \
>+ if (s->name) { \
>+ fputs(#name " : ", f); \
>+ escape_str(s->name, f); \
>+ fputc('\n', f); \
>+ } \
>+ } while (0)
> bool serialize_settings(struct settings *settings)
> {
> FILE *f;
>@@ -1171,9 +1242,17 @@ static char *parse_str(char **val)
> {
> char *ret = *val;
>
>- *val = NULL;
>+ /*
>+ * Unescaping a string is guaranteed to produce a string that is
>+ * smaller or of the same size. Just modify it in place and leak the
>+ * buffer
>+ */
>+ if (unescape_str(ret, NULL) >= 0) {
>+ *val = NULL;
>+ return ret;
>+ }
>
>- return ret;
>+ return NULL;
> }
>
> #define PARSE_LINE(s, name, val, field, _f) \
>--
>2.48.1
>
More information about the igt-dev
mailing list