| Message ID | 20230206150416.4604-13-farosas@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qtests vs. default devices | expand |
On 06/02/2023 16.04, Fabiano Rosas wrote: > The QEMU binary can be built with a varied set of features/devices > which are opaque to the tests. Add a centralized point for parsing and > validating the command line. > > Tests can now be skipped with the following pattern: > > qts = qtest_init(args); > if (!qts) { > return; > } > > For now, the only validation is that the -device options all > correspond to devices that are actually present in the build. > > Signed-off-by: Fabiano Rosas <farosas@suse.de> > --- > Would this be better than checking for missing devices in individual > tests? This is certainly an interesting idea! ... some things still bug me, though: - We still need to change all the calling sites (to check for !qts) ... so the effort seems to be in a similar ballpark as adding qtest_has_device() to the various problematic tests - This will now call qtest_has_device for each and every device in the parameter list, even if it is not necessary. And at least the first call to qtest_has_device() is rather expensive since it has to fire up a separate QEMU to retrieve the list of supported the devices. So adding this to all tests might cause a slow-down to the tests... - It could maybe even hide bugs if you don't look closely, e.g. if you have a typo in the device name in a test, the test then gets skipped automatically instead of failing ... ok, that's unlikely for new tests where you look closely, but still, it gives me slightly bad feeling. So I think I rather tend to go for explicit calls to qtest_has_device() as you did in your first 11 patches. Anyway, I'm interested in what do others think of this? Any other opinions? Thomas
Thomas Huth <thuth@redhat.com> writes: > On 06/02/2023 16.04, Fabiano Rosas wrote: >> The QEMU binary can be built with a varied set of features/devices >> which are opaque to the tests. Add a centralized point for parsing and >> validating the command line. >> >> Tests can now be skipped with the following pattern: >> >> qts = qtest_init(args); >> if (!qts) { >> return; >> } >> >> For now, the only validation is that the -device options all >> correspond to devices that are actually present in the build. >> >> Signed-off-by: Fabiano Rosas <farosas@suse.de> >> --- >> Would this be better than checking for missing devices in individual >> tests? > > This is certainly an interesting idea! ... some things still bug me, though: > > - We still need to change all the calling sites (to check for > !qts) ... so the effort seems to be in a similar ballpark as > adding qtest_has_device() to the various problematic tests Just notice that this series does not cover _all_ -device uses, only the ones that refer to devices disabled by --without-default-devices. So we might need to come back to this when some CONFIG changes, new devices are added, new tests, etc. > - This will now call qtest_has_device for each and every device > in the parameter list, even if it is not necessary. And at > least the first call to qtest_has_device() is rather expensive > since it has to fire up a separate QEMU to retrieve the list > of supported the devices. So adding this to all tests might > cause a slow-down to the tests... Yes, that was my main concern. We could have something like this patch but as a helper that tests can call. Initially, I had thought of: if (qtest_validate_args(args)) { qts = qtest_init(args); } > - It could maybe even hide bugs if you don't look closely, e.g. > if you have a typo in the device name in a test, the test then > gets skipped automatically instead of failing ... ok, that's > unlikely for new tests where you look closely, but still, it > gives me slightly bad feeling. I agree. In fact I have been looking into making the same change (as this patch) in the avocado tests, which of course all fail without-default-devices. There it's considerably simpler (because Python), but I'm still thinking about how to avoid hiding a legitimate failure. > So I think I rather tend to go for explicit calls to qtest_has_device() as > you did in your first 11 patches. Ok, I'll send just them for v2, unless anyone else has something to say.
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c index d658222a19..7920fd1506 100644 --- a/tests/qtest/libqtest.c +++ b/tests/qtest/libqtest.c @@ -479,10 +479,145 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args) return s; } +enum qemu_options { + DEVICE = G_TOKEN_LAST + 1, + DRIVER = G_TOKEN_LAST + 2, +}; + +static void _add_option(GHashTable *ht, const char *key, const char *val) +{ + GList *list = g_hash_table_lookup(ht, key); + + if (!list) { + list = g_list_append(list, g_strdup(val)); + g_hash_table_insert(ht, g_strdup(key), list); + } else { + list = g_list_append(list, g_strdup(val)); + } +} + +static void _parse_device_json(GHashTable *opts, GScanner *top_scanner) +{ + GScanner *scanner = g_scanner_new(top_scanner->config); + gchar *text; + + assert(top_scanner->token == G_TOKEN_STRING); + text = g_strdup(top_scanner->value.v_string); + + g_scanner_scope_add_symbol(scanner, 0, "driver", GINT_TO_POINTER(DRIVER)); + g_scanner_input_text(scanner, text, strlen(text)); + + do { + g_scanner_get_next_token(scanner); + switch ((enum qemu_options)scanner->token) { + case DRIVER: + /* -device "{'driver':'dev' */ + g_scanner_get_next_token(scanner); + + switch (scanner->token) { + case G_TOKEN_IDENTIFIER: + _add_option(opts, "devices", scanner->value.v_string); + break; + + default: /* invalid */ + _add_option(opts, "devices", NULL); + } + break; + default: + break; + } + g_scanner_peek_next_token(scanner); + } while (scanner->next_token != G_TOKEN_EOF && + scanner->next_token != G_TOKEN_ERROR); + + g_scanner_destroy(scanner); + g_free(text); +} + +static void qtest_parse_args(GHashTable *opts, const char *args) +{ + GScanner *scanner = g_scanner_new(NULL); + + scanner->input_name = "qtest args"; + scanner->config->symbol_2_token = 1; + scanner->config->scan_float = 0; + scanner->config->scan_string_sq = 0; + scanner->config->cset_skip_characters = g_strdup(" \t\n':"); + scanner->config->cset_identifier_first = g_strdup("-" G_CSET_a_2_z + G_CSET_A_2_Z + G_CSET_DIGITS), + scanner->config->cset_identifier_nth = g_strdup("-_." G_CSET_a_2_z + G_CSET_A_2_Z G_CSET_DIGITS), + + g_scanner_scope_add_symbol(scanner, 0, "-device", GINT_TO_POINTER(DEVICE)); + + g_scanner_input_text(scanner, args, strlen(args)); + + do { + g_scanner_get_next_token(scanner); + + switch ((enum qemu_options)scanner->token) { + case DEVICE: + g_scanner_get_next_token(scanner); + + switch (scanner->token) { + case G_TOKEN_IDENTIFIER: /* -device dev */ + _add_option(opts, "devices", scanner->value.v_string); + break; + + case G_TOKEN_STRING: /* -device "{'driver':'dev' */ + _parse_device_json(opts, scanner); + break; + + default: /* invalid */ + _add_option(opts, "devices", NULL); + } + break; + default: + break; + } + g_scanner_peek_next_token(scanner); + } while (scanner->next_token != G_TOKEN_EOF && + scanner->next_token != G_TOKEN_ERROR); + + g_scanner_destroy(scanner); +} + +bool qtest_validate_args(const char *args, char **msg) +{ + GHashTable *opts = g_hash_table_new(g_str_hash, g_str_equal); + GList *l; + bool rc = true; + + qtest_parse_args(opts, args); + + for (l = g_hash_table_lookup(opts, "devices"); l != NULL; l = l->next) { + if (!l->data || !qtest_has_device(l->data)) { + *msg = g_strdup_printf("Device %s is not available", + (char *)l->data); + rc = false; + break; + } + } + g_hash_table_unref(opts); + return rc; +} + QTestState *qtest_init(const char *extra_args) { - QTestState *s = qtest_init_without_qmp_handshake(extra_args); + QTestState *s; QDict *greeting; +/* + * char *err_msg; + * + * if (!qtest_validate_args(extra_args, &err_msg)) { + * g_test_skip(err_msg); + * g_free(err_msg); + * + * return NULL; + * } + */ + s = qtest_init_without_qmp_handshake(extra_args); /* Read the QMP greeting and then do the handshake */ greeting = qtest_qmp_receive(s); diff --git a/tests/qtest/libqtest.h b/tests/qtest/libqtest.h index fcf1c3c3b3..01a07c448a 100644 --- a/tests/qtest/libqtest.h +++ b/tests/qtest/libqtest.h @@ -832,4 +832,16 @@ void qtest_qom_set_bool(QTestState *s, const char *path, const char *property, * Returns: Value retrieved from property. */ bool qtest_qom_get_bool(QTestState *s, const char *path, const char *property); + +/** + * qtest_validate_args: + * @args: arguments to validate, exactly as they would be passed + * into qtest_init. + * @err_msg: String with the reason for the failure, if any. + * + * Validates the command line (args) for options that are incompatible + * with the current QEMU build. + */ +bool qtest_validate_args(const char *args, char **err_msg); + #endif
The QEMU binary can be built with a varied set of features/devices which are opaque to the tests. Add a centralized point for parsing and validating the command line. Tests can now be skipped with the following pattern: qts = qtest_init(args); if (!qts) { return; } For now, the only validation is that the -device options all correspond to devices that are actually present in the build. Signed-off-by: Fabiano Rosas <farosas@suse.de> --- Would this be better than checking for missing devices in individual tests? --- tests/qtest/libqtest.c | 137 ++++++++++++++++++++++++++++++++++++++++- tests/qtest/libqtest.h | 12 ++++ 2 files changed, 148 insertions(+), 1 deletion(-)