* [PATCH 00/18] tests: introduce testing coverage for TLS with migration
@ 2022-03-02 17:49 Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 01/18] tests: fix encoding of IP addresses in x509 certs Daniel P. Berrangé
` (17 more replies)
0 siblings, 18 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
This significantly expands the migration test suite to cover testing
with TLS over TCP and UNIX sockets, with both PSK (pre shared keys)
and x509 credentials, and for both single and multifd scenarios.
It identified one bug in handling PSK credentials with UNIX sockets,
but other than that everything was operating as expected.
To minimize the impact on code duplication alopt of refactoring is
done of the migration tests to introduce a common helper for running
the migration process. The various tests mostly just have to provide
a callback to set a few parameters/capabilities before migration
starts, and sometimes a callback to cleanup or validate after
completion/failure.
Daniel P. Berrangé (18):
tests: fix encoding of IP addresses in x509 certs
tests: improve error message when saving TLS PSK file fails
tests: support QTEST_TRACE env variable
tests: print newline after QMP response in qtest logs
tests: add more helper macros for creating TLS x509 certs
crypto: mandate a hostname when checking x509 creds on a client
migration: fix use of TLS PSK credentials with a UNIX socket
tests: merge code for UNIX and TCP migration pre-copy tests
tests: introduce ability to provide hooks for migration precopy test
tests: switch migration FD passing test to use common precopy helper
tests: expand the migration precopy helper to support failures
tests: add migration tests of TLS with PSK credentials
tests: add migration tests of TLS with x509 credentials
tests: convert XBZRLE migration test to use common helper
tests: convert multifd migration tests to use common helper
tests: add multifd migration tests of TLS with PSK credentials
tests: add multifd migration tests of TLS with x509 credentials
tests: ensure migration status isn't reported as failed
crypto/tlssession.c | 6 +
meson.build | 1 +
migration/tls.c | 4 -
tests/qtest/libqtest.c | 11 +-
tests/qtest/meson.build | 12 +-
tests/qtest/migration-helpers.c | 13 +
tests/qtest/migration-helpers.h | 1 +
tests/qtest/migration-test.c | 1041 +++++++++++++++++++++-----
tests/unit/crypto-tls-psk-helpers.c | 20 +-
tests/unit/crypto-tls-psk-helpers.h | 1 +
tests/unit/crypto-tls-x509-helpers.c | 16 +-
tests/unit/crypto-tls-x509-helpers.h | 53 ++
tests/unit/test-crypto-tlssession.c | 11 +-
13 files changed, 1004 insertions(+), 186 deletions(-)
--
2.34.1
^ permalink raw reply [flat|nested] 48+ messages in thread
* [PATCH 01/18] tests: fix encoding of IP addresses in x509 certs
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 02/18] tests: improve error message when saving TLS PSK file fails Daniel P. Berrangé
` (16 subsequent siblings)
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
We need to encode just the address bytes, not the whole struct sockaddr
data. Add a test case to validate that we're matching on SAN IP
addresses correctly.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/unit/crypto-tls-x509-helpers.c | 16 +++++++++++++---
tests/unit/test-crypto-tlssession.c | 11 +++++++++--
2 files changed, 22 insertions(+), 5 deletions(-)
diff --git a/tests/unit/crypto-tls-x509-helpers.c b/tests/unit/crypto-tls-x509-helpers.c
index fc609b3fd4..e9937f60d8 100644
--- a/tests/unit/crypto-tls-x509-helpers.c
+++ b/tests/unit/crypto-tls-x509-helpers.c
@@ -168,9 +168,19 @@ test_tls_get_ipaddr(const char *addrstr,
hints.ai_flags = AI_NUMERICHOST;
g_assert(getaddrinfo(addrstr, NULL, &hints, &res) == 0);
- *datalen = res->ai_addrlen;
- *data = g_new(char, *datalen);
- memcpy(*data, res->ai_addr, *datalen);
+ if (res->ai_family == AF_INET) {
+ struct sockaddr_in *in = (struct sockaddr_in *)res->ai_addr;
+ *datalen = sizeof(in->sin_addr);
+ *data = g_new(char, *datalen);
+ memcpy(*data, &in->sin_addr, *datalen);
+ } else if (res->ai_family == AF_INET6) {
+ struct sockaddr_in6 *in = (struct sockaddr_in6 *)res->ai_addr;
+ *datalen = sizeof(in->sin6_addr);
+ *data = g_new(char, *datalen);
+ memcpy(*data, &in->sin6_addr, *datalen);
+ } else {
+ g_assert_not_reached();
+ }
freeaddrinfo(res);
}
diff --git a/tests/unit/test-crypto-tlssession.c b/tests/unit/test-crypto-tlssession.c
index 5f0da9192c..a6935d8497 100644
--- a/tests/unit/test-crypto-tlssession.c
+++ b/tests/unit/test-crypto-tlssession.c
@@ -512,12 +512,19 @@ int main(int argc, char **argv)
false, true, "wiki.qemu.org", NULL);
TEST_SESS_REG(altname4, cacertreq.filename,
+ servercertalt1req.filename, clientcertreq.filename,
+ false, false, "192.168.122.1", NULL);
+ TEST_SESS_REG(altname5, cacertreq.filename,
+ servercertalt1req.filename, clientcertreq.filename,
+ false, false, "fec0::dead:beaf", NULL);
+
+ TEST_SESS_REG(altname6, cacertreq.filename,
servercertalt2req.filename, clientcertreq.filename,
false, true, "qemu.org", NULL);
- TEST_SESS_REG(altname5, cacertreq.filename,
+ TEST_SESS_REG(altname7, cacertreq.filename,
servercertalt2req.filename, clientcertreq.filename,
false, false, "www.qemu.org", NULL);
- TEST_SESS_REG(altname6, cacertreq.filename,
+ TEST_SESS_REG(altname8, cacertreq.filename,
servercertalt2req.filename, clientcertreq.filename,
false, false, "wiki.qemu.org", NULL);
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 02/18] tests: improve error message when saving TLS PSK file fails
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 01/18] tests: fix encoding of IP addresses in x509 certs Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 6:52 ` Peter Xu
2022-03-02 17:49 ` [PATCH 03/18] tests: support QTEST_TRACE env variable Daniel P. Berrangé
` (15 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/unit/crypto-tls-psk-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/unit/crypto-tls-psk-helpers.c b/tests/unit/crypto-tls-psk-helpers.c
index 7f8a488961..4bea7c6fa2 100644
--- a/tests/unit/crypto-tls-psk-helpers.c
+++ b/tests/unit/crypto-tls-psk-helpers.c
@@ -30,7 +30,7 @@ void test_tls_psk_init(const char *pskfile)
fp = fopen(pskfile, "w");
if (fp == NULL) {
- g_critical("Failed to create pskfile %s", pskfile);
+ g_critical("Failed to create pskfile %s: %s", pskfile, strerror(errno));
abort();
}
/* Don't do this in real applications! Use psktool. */
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 03/18] tests: support QTEST_TRACE env variable
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 01/18] tests: fix encoding of IP addresses in x509 certs Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 02/18] tests: improve error message when saving TLS PSK file fails Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 6:53 ` Peter Xu
2022-03-07 10:06 ` Thomas Huth
2022-03-02 17:49 ` [PATCH 04/18] tests: print newline after QMP response in qtest logs Daniel P. Berrangé
` (14 subsequent siblings)
17 siblings, 2 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
When debugging failing qtests it is useful to be able to turn on trace
output to stderr. The QTEST_TRACE env variable contents get injected
as a '-trace <str>' command line arg
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/libqtest.c | 8 ++++++--
1 file changed, 6 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index 41f4da4e54..a85f8a6d05 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -260,6 +260,9 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
gchar *qmp_socket_path;
gchar *command;
const char *qemu_binary = qtest_qemu_binary();
+ const char *trace = g_getenv("QTEST_TRACE");
+ g_autofree char *tracearg = trace ?
+ g_strdup_printf("-trace %s ", trace) : g_strdup("");
s = g_new(QTestState, 1);
@@ -282,14 +285,15 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
qtest_add_abrt_handler(kill_qemu_hook_func, s);
- command = g_strdup_printf("exec %s "
+ command = g_strdup_printf("exec %s %s"
"-qtest unix:%s "
"-qtest-log %s "
"-chardev socket,path=%s,id=char0 "
"-mon chardev=char0,mode=control "
"-display none "
"%s"
- " -accel qtest", qemu_binary, socket_path,
+ " -accel qtest",
+ qemu_binary, tracearg, socket_path,
getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
qmp_socket_path,
extra_args ?: "");
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (2 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 03/18] tests: support QTEST_TRACE env variable Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 6:51 ` Peter Xu
2022-03-02 17:49 ` [PATCH 05/18] tests: add more helper macros for creating TLS x509 certs Daniel P. Berrangé
` (13 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
The QMP commands have a trailing newline, but the response does not.
This makes the qtest logs hard to follow as the next QMP command
appears in the same line as the previous QMP response.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/libqtest.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
index a85f8a6d05..79c3edcf4b 100644
--- a/tests/qtest/libqtest.c
+++ b/tests/qtest/libqtest.c
@@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
}
json_message_parser_feed(&qmp.parser, &c, 1);
}
+ if (log) {
+ g_assert(write(2, "\n", 1) == 1);
+ }
json_message_parser_destroy(&qmp.parser);
return qmp.response;
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 05/18] tests: add more helper macros for creating TLS x509 certs
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (3 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 04/18] tests: print newline after QMP response in qtest logs Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 06/18] crypto: mandate a hostname when checking x509 creds on a client Daniel P. Berrangé
` (12 subsequent siblings)
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
These macros are more suited to the general consumers of certs in the
test suite, where we don't need to exercise every single possible
permutation.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/unit/crypto-tls-x509-helpers.h | 53 ++++++++++++++++++++++++++++
1 file changed, 53 insertions(+)
diff --git a/tests/unit/crypto-tls-x509-helpers.h b/tests/unit/crypto-tls-x509-helpers.h
index cf6329e653..247e7160eb 100644
--- a/tests/unit/crypto-tls-x509-helpers.h
+++ b/tests/unit/crypto-tls-x509-helpers.h
@@ -26,6 +26,9 @@
#include <libtasn1.h>
+#define QCRYPTO_TLS_TEST_CLIENT_NAME "ACME QEMU Client"
+#define QCRYPTO_TLS_TEST_CLIENT_HOSTILE_NAME "ACME Hostile Client"
+
/*
* This contains parameter about how to generate
* certificates.
@@ -118,6 +121,56 @@ void test_tls_cleanup(const char *keyfile);
}; \
test_tls_generate_cert(&varname, NULL)
+# define TLS_ROOT_REQ_SIMPLE(varname, fname) \
+ QCryptoTLSTestCertReq varname = { \
+ .filename = fname, \
+ .cn = "qemu-CA", \
+ .basicConstraintsEnable = true, \
+ .basicConstraintsCritical = true, \
+ .basicConstraintsIsCA = true, \
+ .keyUsageEnable = true, \
+ .keyUsageCritical = true, \
+ .keyUsageValue = GNUTLS_KEY_KEY_CERT_SIGN, \
+ }; \
+ test_tls_generate_cert(&varname, NULL)
+
+# define TLS_CERT_REQ_SIMPLE_CLIENT(varname, cavarname, cname, fname) \
+ QCryptoTLSTestCertReq varname = { \
+ .filename = fname, \
+ .cn = cname, \
+ .basicConstraintsEnable = true, \
+ .basicConstraintsCritical = true, \
+ .basicConstraintsIsCA = false, \
+ .keyUsageEnable = true, \
+ .keyUsageCritical = true, \
+ .keyUsageValue = \
+ GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, \
+ .keyPurposeEnable = true, \
+ .keyPurposeCritical = true, \
+ .keyPurposeOID1 = GNUTLS_KP_TLS_WWW_CLIENT, \
+ }; \
+ test_tls_generate_cert(&varname, cavarname.crt)
+
+# define TLS_CERT_REQ_SIMPLE_SERVER(varname, cavarname, fname, \
+ hostname, ipaddr) \
+ QCryptoTLSTestCertReq varname = { \
+ .filename = fname, \
+ .cn = hostname ? hostname : ipaddr, \
+ .altname1 = hostname, \
+ .ipaddr1 = ipaddr, \
+ .basicConstraintsEnable = true, \
+ .basicConstraintsCritical = true, \
+ .basicConstraintsIsCA = false, \
+ .keyUsageEnable = true, \
+ .keyUsageCritical = true, \
+ .keyUsageValue = \
+ GNUTLS_KEY_DIGITAL_SIGNATURE | GNUTLS_KEY_KEY_ENCIPHERMENT, \
+ .keyPurposeEnable = true, \
+ .keyPurposeCritical = true, \
+ .keyPurposeOID1 = GNUTLS_KP_TLS_WWW_SERVER, \
+ }; \
+ test_tls_generate_cert(&varname, cavarname.crt)
+
extern const asn1_static_node pkix_asn1_tab[];
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 06/18] crypto: mandate a hostname when checking x509 creds on a client
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (4 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 05/18] tests: add more helper macros for creating TLS x509 certs Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket Daniel P. Berrangé
` (11 subsequent siblings)
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
Currently the TLS session object assumes that the caller will always
provide a hostname when using x509 creds on a client endpoint. This
relies on the caller to detect and report an error if the user has
configured QEMU with x509 credentials on a UNIX socket. The migration
code has such a check, but it is too broad, reporting an error when
the user has configured QEMU with PSK credentials on a UNIX socket,
where hostnames are irrelevant.
Putting the check into the TLS session object credentials validation
code ensures we report errors in only the scenario that matters.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
crypto/tlssession.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/crypto/tlssession.c b/crypto/tlssession.c
index a8db8c76d1..b302d835d2 100644
--- a/crypto/tlssession.c
+++ b/crypto/tlssession.c
@@ -373,6 +373,12 @@ qcrypto_tls_session_check_certificate(QCryptoTLSSession *session,
session->hostname);
goto error;
}
+ } else {
+ if (session->creds->endpoint ==
+ QCRYPTO_TLS_CREDS_ENDPOINT_CLIENT) {
+ error_setg(errp, "No hostname for certificate validation");
+ goto error;
+ }
}
}
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (5 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 06/18] crypto: mandate a hostname when checking x509 creds on a client Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 7:08 ` Peter Xu
2022-03-02 17:49 ` [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests Daniel P. Berrangé
` (10 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
The migration TLS code has a check mandating that a hostname be
available when starting a TLS session. This is expected when using
x509 credentials, but is bogus for PSK and anonymous credentials
as neither involve hostname validation.
The TLS crdentials object gained suitable error reporting in the
case of TLS with x509 credentials, so there is no longer any need
for the migration code to do its own (incorrect) validation.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
migration/tls.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/migration/tls.c b/migration/tls.c
index ca1ea3bbdd..32c384a8b6 100644
--- a/migration/tls.c
+++ b/migration/tls.c
@@ -137,10 +137,6 @@ QIOChannelTLS *migration_tls_client_create(MigrationState *s,
if (s->parameters.tls_hostname && *s->parameters.tls_hostname) {
hostname = s->parameters.tls_hostname;
}
- if (!hostname) {
- error_setg(errp, "No hostname available for TLS");
- return NULL;
- }
tioc = qio_channel_tls_new_client(
ioc, creds, hostname, errp);
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (6 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 7:16 ` Peter Xu
2022-03-07 10:11 ` Thomas Huth
2022-03-02 17:49 ` [PATCH 09/18] tests: introduce ability to provide hooks for migration precopy test Daniel P. Berrangé
` (9 subsequent siblings)
17 siblings, 2 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
The test cases differ only in the URI they provide to the migration
commands, and the ability to set the dirty_ring mode. This code is
trivially merged into a common helper.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
roms/seabios | 2 +-
tests/qtest/migration-test.c | 86 ++++++++++++++++--------------------
2 files changed, 40 insertions(+), 48 deletions(-)
diff --git a/roms/seabios b/roms/seabios
index 6a62e0cb0d..2dd4b9b3f8 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit 6a62e0cb0dfe9cd28b70547dbea5caf76847c3a9
+Subproject commit 2dd4b9b3f84019668719344b40dba79d681be41c
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7b42f6fd90..c1058dc944 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -796,19 +796,34 @@ static void test_baddest(void)
test_migrate_end(from, to, false);
}
-static void test_precopy_unix_common(bool dirty_ring)
+/*
+ * Common helper for running a precopy migration test
+ *
+ * @listen_uri: the URI for the dst QEMU to listen on
+ * @connect_uri: the URI for the src QEMU to connect to
+ * @dirty_ring: true to use dirty ring tracking
+ *
+ * If @connect_uri is NULL, then it will query the dst
+ * QEMU for its actual listening address and use that
+ * as the connect address. This allows for dynamically
+ * picking a free TCP port.
+ */
+static void test_precopy_common(const char *listen_uri,
+ const char *connect_uri,
+ bool dirty_ring)
{
- g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
MigrateStart *args = migrate_start_new();
+ g_autofree char *local_connect_uri = NULL;
QTestState *from, *to;
args->use_dirty_ring = dirty_ring;
- if (test_migrate_start(&from, &to, uri, args)) {
+ if (test_migrate_start(&from, &to, listen_uri, args)) {
return;
}
- /* We want to pick a speed slow enough that the test completes
+ /*
+ * We want to pick a speed slow enough that the test completes
* quickly, but that it doesn't complete precopy even on a slow
* machine, so also set the downtime.
*/
@@ -820,7 +835,12 @@ static void test_precopy_unix_common(bool dirty_ring)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
- migrate_qmp(from, uri, "{}");
+ if (!connect_uri) {
+ local_connect_uri = migrate_get_socket_address(to, "socket-address");
+ connect_uri = local_connect_uri;
+ }
+
+ migrate_qmp(from, connect_uri, "{}");
wait_for_migration_pass(from);
@@ -838,16 +858,23 @@ static void test_precopy_unix_common(bool dirty_ring)
test_migrate_end(from, to, true);
}
+static void test_precopy_unix_common(bool dirty_ring)
+{
+ g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+
+ test_precopy_common(uri,
+ uri,
+ dirty_ring);
+}
+
static void test_precopy_unix(void)
{
- /* Using default dirty logging */
- test_precopy_unix_common(false);
+ test_precopy_unix_common(false /* dirty_ring */);
}
static void test_precopy_unix_dirty_ring(void)
{
- /* Using dirty ring tracking */
- test_precopy_unix_common(true);
+ test_precopy_unix_common(true /* dirty_ring */);
}
#if 0
@@ -942,44 +969,9 @@ static void test_xbzrle_unix(void)
static void test_precopy_tcp(void)
{
- MigrateStart *args = migrate_start_new();
- g_autofree char *uri = NULL;
- QTestState *from, *to;
-
- if (test_migrate_start(&from, &to, "tcp:127.0.0.1:0", args)) {
- return;
- }
-
- /*
- * We want to pick a speed slow enough that the test completes
- * quickly, but that it doesn't complete precopy even on a slow
- * machine, so also set the downtime.
- */
- /* 1 ms should make it not converge*/
- migrate_set_parameter_int(from, "downtime-limit", 1);
- /* 1GB/s */
- migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
-
- /* Wait for the first serial output from the source */
- wait_for_serial("src_serial");
-
- uri = migrate_get_socket_address(to, "socket-address");
-
- migrate_qmp(from, uri, "{}");
-
- wait_for_migration_pass(from);
-
- migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME);
-
- if (!got_stop) {
- qtest_qmp_eventwait(from, "STOP");
- }
- qtest_qmp_eventwait(to, "RESUME");
-
- wait_for_serial("dest_serial");
- wait_for_migration_complete(from);
-
- test_migrate_end(from, to, true);
+ test_precopy_common("tcp:127.0.0.1:0",
+ NULL, /* connect_uri */
+ false /* dirty_ring */);
}
static void test_migrate_fd_proto(void)
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 09/18] tests: introduce ability to provide hooks for migration precopy test
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (7 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 7:19 ` Peter Xu
2022-03-02 17:49 ` [PATCH 10/18] tests: switch migration FD passing test to use common precopy helper Daniel P. Berrangé
` (8 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
There are alot of different scenarios to test with migration due to the
wide number of parameters and capabilities available. To enable sharing
of the basic precopy test scenario, we need to be able to set arbitrary
parameters and capabilities before the migration is initiated, but don't
want to have all this logic in the common helper function. Solve this
by defining two hooks that can be provided by the test case, one before
migration starts and one after migration finishes.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 41 ++++++++++++++++++++++++++++++++++++
1 file changed, 41 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index c1058dc944..2f2059cebc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -796,11 +796,37 @@ static void test_baddest(void)
test_migrate_end(from, to, false);
}
+/*
+ * A hook that runs after the src and dst QEMUs have been
+ * created, but before the migration is started. This can
+ * be used to set migration parameters and capabilities.
+ *
+ * Returns: NULL, or a pointer to opaque state to be
+ * later passed to the TestMigrateFinishHook
+ */
+typedef void * (*TestMigrateStartHook)(QTestState *from,
+ QTestState *to);
+
+/*
+ * A hook that runs after the migration has finished,
+ * regardless of whether it succeeded or failed, but
+ * before QEMU has terminated (unless it self-terminated
+ * due to migration error)
+ *
+ * @opaque is a pointer to state previously returned
+ * by the TestMigrateStartHook if any, or NULL.
+ */
+typedef void (*TestMigrateFinishHook)(QTestState *from,
+ QTestState *to,
+ void *opaque);
+
/*
* Common helper for running a precopy migration test
*
* @listen_uri: the URI for the dst QEMU to listen on
* @connect_uri: the URI for the src QEMU to connect to
+ * @start_hook: (optional) callback to run at start to set migration parameters
+ * @finish_hook: (optional) callback to run at finish to cleanup
* @dirty_ring: true to use dirty ring tracking
*
* If @connect_uri is NULL, then it will query the dst
@@ -810,11 +836,14 @@ static void test_baddest(void)
*/
static void test_precopy_common(const char *listen_uri,
const char *connect_uri,
+ TestMigrateStartHook start_hook,
+ TestMigrateFinishHook finish_hook,
bool dirty_ring)
{
MigrateStart *args = migrate_start_new();
g_autofree char *local_connect_uri = NULL;
QTestState *from, *to;
+ void *data_hook = NULL;
args->use_dirty_ring = dirty_ring;
@@ -832,6 +861,10 @@ static void test_precopy_common(const char *listen_uri,
/* 1GB/s */
migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
+ if (start_hook) {
+ data_hook = start_hook(from, to);
+ }
+
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
@@ -855,6 +888,10 @@ static void test_precopy_common(const char *listen_uri,
wait_for_serial("dest_serial");
wait_for_migration_complete(from);
+ if (finish_hook) {
+ finish_hook(from, to, data_hook);
+ }
+
test_migrate_end(from, to, true);
}
@@ -864,6 +901,8 @@ static void test_precopy_unix_common(bool dirty_ring)
test_precopy_common(uri,
uri,
+ NULL, /* start_hook */
+ NULL, /* finish_hook */
dirty_ring);
}
@@ -971,6 +1010,8 @@ static void test_precopy_tcp(void)
{
test_precopy_common("tcp:127.0.0.1:0",
NULL, /* connect_uri */
+ NULL, /* start_hook */
+ NULL, /* finish_hook */
false /* dirty_ring */);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 10/18] tests: switch migration FD passing test to use common precopy helper
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (8 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 09/18] tests: introduce ability to provide hooks for migration precopy test Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 7:22 ` Peter Xu
2022-03-02 17:49 ` [PATCH 11/18] tests: expand the migration precopy helper to support failures Daniel P. Berrangé
` (7 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
The combination of the start and finish hooks allow the FD passing
code to use the precopy helper
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 55 +++++++++++++-----------------------
1 file changed, 19 insertions(+), 36 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 2f2059cebc..2082c58e8b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1015,31 +1015,12 @@ static void test_precopy_tcp(void)
false /* dirty_ring */);
}
-static void test_migrate_fd_proto(void)
+static void *test_migrate_fd_start_hook(QTestState *from,
+ QTestState *to)
{
- MigrateStart *args = migrate_start_new();
- QTestState *from, *to;
+ QDict *rsp;
int ret;
int pair[2];
- QDict *rsp;
- const char *error_desc;
-
- if (test_migrate_start(&from, &to, "defer", args)) {
- return;
- }
-
- /*
- * We want to pick a speed slow enough that the test completes
- * quickly, but that it doesn't complete precopy even on a slow
- * machine, so also set the downtime.
- */
- /* 1 ms should make it not converge */
- migrate_set_parameter_int(from, "downtime-limit", 1);
- /* 1GB/s */
- migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
-
- /* Wait for the first serial output from the source */
- wait_for_serial("src_serial");
/* Create two connected sockets for migration */
ret = socketpair(PF_LOCAL, SOCK_STREAM, 0, pair);
@@ -1064,17 +1045,15 @@ static void test_migrate_fd_proto(void)
qobject_unref(rsp);
close(pair[1]);
- /* Start migration to the 2nd socket*/
- migrate_qmp(from, "fd:fd-mig", "{}");
-
- wait_for_migration_pass(from);
-
- migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME);
+ return NULL;
+}
- if (!got_stop) {
- qtest_qmp_eventwait(from, "STOP");
- }
- qtest_qmp_eventwait(to, "RESUME");
+static void test_migrate_fd_finish_hook(QTestState *from,
+ QTestState *to,
+ void *opaque)
+{
+ QDict *rsp;
+ const char *error_desc;
/* Test closing fds */
/* We assume, that QEMU removes named fd from its list,
@@ -1092,11 +1071,15 @@ static void test_migrate_fd_proto(void)
error_desc = qdict_get_str(qdict_get_qdict(rsp, "error"), "desc");
g_assert_cmpstr(error_desc, ==, "File descriptor named 'fd-mig' not found");
qobject_unref(rsp);
+}
- /* Complete migration */
- wait_for_serial("dest_serial");
- wait_for_migration_complete(from);
- test_migrate_end(from, to, true);
+static void test_migrate_fd_proto(void)
+{
+ test_precopy_common("defer",
+ "fd:fd-mig",
+ test_migrate_fd_start_hook,
+ test_migrate_fd_finish_hook,
+ false /* dirty_ring */);
}
static void do_test_validate_uuid(MigrateStart *args, bool should_fail)
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 11/18] tests: expand the migration precopy helper to support failures
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (9 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 10/18] tests: switch migration FD passing test to use common precopy helper Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 7:39 ` Peter Xu
2022-03-07 7:57 ` Peter Xu
2022-03-02 17:49 ` [PATCH 12/18] tests: add migration tests of TLS with PSK credentials Daniel P. Berrangé
` (6 subsequent siblings)
17 siblings, 2 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
The migration precopy testing helper function always expects the
migration to run to a completion state. There will be test scenarios
for TLS where expect either the client or server to fail the migration.
This expands the helper to cope with these scenarios.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 47 +++++++++++++++++++++++++++++-------
1 file changed, 38 insertions(+), 9 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 2082c58e8b..e40b408988 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -827,17 +827,32 @@ typedef void (*TestMigrateFinishHook)(QTestState *from,
* @connect_uri: the URI for the src QEMU to connect to
* @start_hook: (optional) callback to run at start to set migration parameters
* @finish_hook: (optional) callback to run at finish to cleanup
+ * @expect_fail: true if we expect migration to fail
+ * @dst_quit: true if we expect the dst QEMU to quit with an
+ * abnormal exit status on failure
* @dirty_ring: true to use dirty ring tracking
*
* If @connect_uri is NULL, then it will query the dst
* QEMU for its actual listening address and use that
* as the connect address. This allows for dynamically
* picking a free TCP port.
+ *
+ * If @expect_fail is true then we expect the migration process to
+ * fail instead of completing. There can be a variety of reasons
+ * and stages in which this may happen. If a failure is expected
+ * to happen at time of establishing the connection, then @dst_quit
+ * should be false to indicate that the dst QEMU is espected to
+ * stay running and accept future migration connections. If a
+ * failure is expected to happen while processing the migration
+ * stream, then @dst_quit should be true to indicate that the
+ * dst QEMU is expected to quit with non-zero exit status
*/
static void test_precopy_common(const char *listen_uri,
const char *connect_uri,
TestMigrateStartHook start_hook,
TestMigrateFinishHook finish_hook,
+ bool expect_fail,
+ bool dst_quit,
bool dirty_ring)
{
MigrateStart *args = migrate_start_new();
@@ -875,24 +890,32 @@ static void test_precopy_common(const char *listen_uri,
migrate_qmp(from, connect_uri, "{}");
- wait_for_migration_pass(from);
+ if (expect_fail) {
+ wait_for_migration_fail(from, !dst_quit);
- migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME);
+ if (dst_quit) {
+ qtest_set_expected_status(to, 1);
+ }
+ } else {
+ wait_for_migration_pass(from);
- if (!got_stop) {
- qtest_qmp_eventwait(from, "STOP");
- }
+ migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME);
- qtest_qmp_eventwait(to, "RESUME");
+ if (!got_stop) {
+ qtest_qmp_eventwait(from, "STOP");
+ }
- wait_for_serial("dest_serial");
- wait_for_migration_complete(from);
+ qtest_qmp_eventwait(to, "RESUME");
+
+ wait_for_serial("dest_serial");
+ wait_for_migration_complete(from);
+ }
if (finish_hook) {
finish_hook(from, to, data_hook);
}
- test_migrate_end(from, to, true);
+ test_migrate_end(from, to, !expect_fail);
}
static void test_precopy_unix_common(bool dirty_ring)
@@ -903,6 +926,8 @@ static void test_precopy_unix_common(bool dirty_ring)
uri,
NULL, /* start_hook */
NULL, /* finish_hook */
+ false, /* expect_fail */
+ false, /* dst_quit */
dirty_ring);
}
@@ -1012,6 +1037,8 @@ static void test_precopy_tcp(void)
NULL, /* connect_uri */
NULL, /* start_hook */
NULL, /* finish_hook */
+ false, /* expect_fail */
+ false, /* dst_quit */
false /* dirty_ring */);
}
@@ -1079,6 +1106,8 @@ static void test_migrate_fd_proto(void)
"fd:fd-mig",
test_migrate_fd_start_hook,
test_migrate_fd_finish_hook,
+ false, /* expect_fail */
+ false, /* dst_quit */
false /* dirty_ring */);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 12/18] tests: add migration tests of TLS with PSK credentials
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (10 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 11/18] tests: expand the migration precopy helper to support failures Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 10:12 ` Thomas Huth
2022-03-02 17:49 ` [PATCH 13/18] tests: add migration tests of TLS with x509 credentials Daniel P. Berrangé
` (5 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
This validates that we correctly handle migration success and failure
scenarios when using TLS with pre shared keys.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
roms/seabios | 2 +-
tests/qtest/meson.build | 7 +-
tests/qtest/migration-test.c | 180 ++++++++++++++++++++++++++--
tests/unit/crypto-tls-psk-helpers.c | 18 ++-
tests/unit/crypto-tls-psk-helpers.h | 1 +
5 files changed, 190 insertions(+), 18 deletions(-)
diff --git a/roms/seabios b/roms/seabios
index 2dd4b9b3f8..6a62e0cb0d 160000
--- a/roms/seabios
+++ b/roms/seabios
@@ -1 +1 @@
-Subproject commit 2dd4b9b3f84019668719344b40dba79d681be41c
+Subproject commit 6a62e0cb0dfe9cd28b70547dbea5caf76847c3a9
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index f33d84d19b..a95bb5def3 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -276,13 +276,18 @@ endif
tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
+migration_files = [files('migration-helpers.c')]
+if gnutls.found()
+ migration_files += [files('../unit/crypto-tls-psk-helpers.c'), gnutls]
+endif
+
qtests = {
'bios-tables-test': [io, 'boot-sector.c', 'acpi-utils.c', 'tpm-emu.c'],
'cdrom-test': files('boot-sector.c'),
'dbus-vmstate-test': files('migration-helpers.c') + dbus_vmstate1,
'erst-test': files('erst-test.c'),
'ivshmem-test': [rt, '../../contrib/ivshmem-server/ivshmem-server.c'],
- 'migration-test': files('migration-helpers.c'),
+ 'migration-test': migration_files,
'pxe-test': files('boot-sector.c'),
'qos-test': [chardev, io, qos_test_ss.apply(config_host, strict: false).sources()],
'tpm-crb-swtpm-test': [io, tpmemu_files],
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index e40b408988..744a9f8123 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -23,9 +23,13 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/qobject-output-visitor.h"
+#include "crypto/tlscredspsk.h"
#include "migration-helpers.h"
#include "tests/migration/migration-test.h"
+#ifdef CONFIG_GNUTLS
+# include "tests/unit/crypto-tls-psk-helpers.h"
+#endif
/* For dirty ring test; so far only x86_64 is supported */
#if defined(__linux__) && defined(HOST_X86_64)
@@ -658,6 +662,100 @@ static void test_migrate_end(QTestState *from, QTestState *to, bool test_dest)
cleanup("dest_serial");
}
+#ifdef CONFIG_GNUTLS
+struct TestMigrateTLSPSKData {
+ char *workdir;
+ char *workdiralt;
+ char *pskfile;
+ char *pskfilealt;
+};
+
+static void *
+test_migrate_tls_psk_start_common(QTestState *from,
+ QTestState *to,
+ bool mismatch)
+{
+ struct TestMigrateTLSPSKData *data =
+ g_new0(struct TestMigrateTLSPSKData, 1);
+ QDict *rsp;
+
+ data->workdir = g_strdup_printf("%s/tlscredspsk0", tmpfs);
+ data->pskfile = g_strdup_printf("%s/%s", data->workdir,
+ QCRYPTO_TLS_CREDS_PSKFILE);
+ mkdir(data->workdir, 0700);
+ test_tls_psk_init(data->pskfile);
+
+ if (mismatch) {
+ data->workdiralt = g_strdup_printf("%s/tlscredspskalt0", tmpfs);
+ data->pskfilealt = g_strdup_printf("%s/%s", data->workdiralt,
+ QCRYPTO_TLS_CREDS_PSKFILE);
+ mkdir(data->workdiralt, 0700);
+ test_tls_psk_init_alt(data->pskfilealt);
+ }
+
+ rsp = wait_command(from,
+ "{ 'execute': 'object-add',"
+ " 'arguments': { 'qom-type': 'tls-creds-psk',"
+ " 'id': 'tlscredspsk0',"
+ " 'endpoint': 'client',"
+ " 'dir': %s,"
+ " 'username': 'qemu'} }",
+ data->workdir);
+ qobject_unref(rsp);
+
+ rsp = wait_command(to,
+ "{ 'execute': 'object-add',"
+ " 'arguments': { 'qom-type': 'tls-creds-psk',"
+ " 'id': 'tlscredspsk0',"
+ " 'endpoint': 'server',"
+ " 'dir': %s } }",
+ mismatch ? data->workdiralt : data->workdir);
+ qobject_unref(rsp);
+
+ migrate_set_parameter_str(from, "tls-creds", "tlscredspsk0");
+ migrate_set_parameter_str(to, "tls-creds", "tlscredspsk0");
+
+ return data;
+}
+
+static void *
+test_migrate_tls_psk_start_match(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_psk_start_common(from, to, false);
+}
+
+static void *
+test_migrate_tls_psk_start_mismatch(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_psk_start_common(from, to, true);
+}
+
+static void
+test_migrate_tls_psk_finish(QTestState *from,
+ QTestState *to,
+ void *opaque)
+{
+ struct TestMigrateTLSPSKData *data = opaque;
+
+ test_tls_psk_cleanup(data->pskfile);
+ if (data->pskfilealt) {
+ test_tls_psk_cleanup(data->pskfilealt);
+ }
+ rmdir(data->workdir);
+ if (data->workdiralt) {
+ rmdir(data->workdiralt);
+ }
+
+ g_free(data->workdiralt);
+ g_free(data->pskfilealt);
+ g_free(data->workdir);
+ g_free(data->pskfile);
+ g_free(data);
+}
+#endif /* CONFIG_GNUTLS */
+
static int migrate_postcopy_prepare(QTestState **from_ptr,
QTestState **to_ptr,
MigrateStart *args)
@@ -918,27 +1016,45 @@ static void test_precopy_common(const char *listen_uri,
test_migrate_end(from, to, !expect_fail);
}
-static void test_precopy_unix_common(bool dirty_ring)
+
+static void test_precopy_unix_common(TestMigrateStartHook start_hook,
+ TestMigrateFinishHook finish_hook,
+ bool expect_fail,
+ bool dirty_ring)
{
g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
test_precopy_common(uri,
uri,
- NULL, /* start_hook */
- NULL, /* finish_hook */
- false, /* expect_fail */
+ start_hook,
+ finish_hook,
+ expect_fail,
false, /* dst_quit */
dirty_ring);
}
-static void test_precopy_unix(void)
+static void test_precopy_unix_plain(void)
{
- test_precopy_unix_common(false /* dirty_ring */);
+ test_precopy_unix_common(NULL, /* start_hook */
+ NULL, /* finish_hook */
+ false, /* expect_fail */
+ false /* dirty_ring */);
+}
+
+static void test_precopy_unix_tls_psk(void)
+{
+ test_precopy_unix_common(test_migrate_tls_psk_start_match,
+ test_migrate_tls_psk_finish,
+ false, /* expect_fail */
+ false /* dirty_ring */);
}
static void test_precopy_unix_dirty_ring(void)
{
- test_precopy_unix_common(true /* dirty_ring */);
+ test_precopy_unix_common(NULL, /* start_hook */
+ NULL, /* finish_hook */
+ false, /* clientReject */
+ true /* dirty_ring */);
}
#if 0
@@ -1031,17 +1147,43 @@ static void test_xbzrle_unix(void)
test_xbzrle(uri);
}
-static void test_precopy_tcp(void)
+static void test_precopy_tcp_common(TestMigrateStartHook start_hook,
+ TestMigrateFinishHook finish_hook,
+ bool expect_fail)
{
test_precopy_common("tcp:127.0.0.1:0",
NULL, /* connect_uri */
- NULL, /* start_hook */
- NULL, /* finish_hook */
- false, /* expect_fail */
+ start_hook,
+ finish_hook,
+ expect_fail,
false, /* dst_quit */
false /* dirty_ring */);
}
+
+static void test_precopy_tcp_plain(void)
+{
+ test_precopy_tcp_common(NULL, /* start_hook */
+ NULL, /* finish_hook */
+ false /* expect_fail */);
+}
+
+#ifdef CONFIG_GNUTLS
+static void test_precopy_tcp_tls_psk_match(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_psk_start_match,
+ test_migrate_tls_psk_finish,
+ false /* expect_fail */);
+}
+
+static void test_precopy_tcp_tls_psk_mismatch(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_psk_start_mismatch,
+ test_migrate_tls_psk_finish,
+ true /* expect_fail */);
+}
+#endif /* CONFIG_GNUTLS */
+
static void *test_migrate_fd_start_hook(QTestState *from,
QTestState *to)
{
@@ -1505,8 +1647,20 @@ int main(int argc, char **argv)
qtest_add_func("/migration/postcopy/unix", test_postcopy);
qtest_add_func("/migration/postcopy/recovery", test_postcopy_recovery);
qtest_add_func("/migration/bad_dest", test_baddest);
- qtest_add_func("/migration/precopy/unix", test_precopy_unix);
- qtest_add_func("/migration/precopy/tcp", test_precopy_tcp);
+ qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
+#ifdef CONFIG_GNUTLS
+ qtest_add_func("/migration/precopy/unix/tls/psk",
+ test_precopy_unix_tls_psk);
+#endif /* CONFIG_GNUTLS */
+
+ qtest_add_func("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
+#ifdef CONFIG_GNUTLS
+ qtest_add_func("/migration/precopy/tcp/tls/psk/match",
+ test_precopy_tcp_tls_psk_match);
+ qtest_add_func("/migration/precopy/tcp/tls/psk/mismatch",
+ test_precopy_tcp_tls_psk_mismatch);
+#endif /* CONFIG_GNUTLS */
+
/* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */
qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
qtest_add_func("/migration/fd_proto", test_migrate_fd_proto);
diff --git a/tests/unit/crypto-tls-psk-helpers.c b/tests/unit/crypto-tls-psk-helpers.c
index 4bea7c6fa2..511e08cc9c 100644
--- a/tests/unit/crypto-tls-psk-helpers.c
+++ b/tests/unit/crypto-tls-psk-helpers.c
@@ -24,7 +24,8 @@
#include "crypto-tls-psk-helpers.h"
#include "qemu/sockets.h"
-void test_tls_psk_init(const char *pskfile)
+static void
+test_tls_psk_init_common(const char *pskfile, const char *user, const char *key)
{
FILE *fp;
@@ -33,11 +34,22 @@ void test_tls_psk_init(const char *pskfile)
g_critical("Failed to create pskfile %s: %s", pskfile, strerror(errno));
abort();
}
- /* Don't do this in real applications! Use psktool. */
- fprintf(fp, "qemu:009d5638c40fde0c\n");
+ fprintf(fp, "%s:%s\n", user, key);
fclose(fp);
}
+void test_tls_psk_init(const char *pskfile)
+{
+ /* Don't hard code a key like this in real applications! Use psktool. */
+ test_tls_psk_init_common(pskfile, "qemu", "009d5638c40fde0c");
+}
+
+void test_tls_psk_init_alt(const char *pskfile)
+{
+ /* Don't hard code a key like this in real applications! Use psktool. */
+ test_tls_psk_init_common(pskfile, "qemu", "10ffa6a2c42f0388");
+}
+
void test_tls_psk_cleanup(const char *pskfile)
{
unlink(pskfile);
diff --git a/tests/unit/crypto-tls-psk-helpers.h b/tests/unit/crypto-tls-psk-helpers.h
index faa645c629..67f8bdda71 100644
--- a/tests/unit/crypto-tls-psk-helpers.h
+++ b/tests/unit/crypto-tls-psk-helpers.h
@@ -24,6 +24,7 @@
#include <gnutls/gnutls.h>
void test_tls_psk_init(const char *keyfile);
+void test_tls_psk_init_alt(const char *keyfile);
void test_tls_psk_cleanup(const char *keyfile);
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 13/18] tests: add migration tests of TLS with x509 credentials
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (11 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 12/18] tests: add migration tests of TLS with PSK credentials Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 14/18] tests: convert XBZRLE migration test to use common helper Daniel P. Berrangé
` (4 subsequent siblings)
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
This validates that we correctly handle migration success and failure
scenarios when using TLS with x509 certificates. There are quite a few
different scenarios that matter in relation to hostname validation.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
meson.build | 1 +
tests/qtest/meson.build | 5 +
tests/qtest/migration-test.c | 366 +++++++++++++++++++++++++++++++++--
3 files changed, 361 insertions(+), 11 deletions(-)
diff --git a/meson.build b/meson.build
index 8df40bfac4..06d2175bdf 100644
--- a/meson.build
+++ b/meson.build
@@ -1548,6 +1548,7 @@ config_host_data.set('CONFIG_KEYUTILS', keyutils.found())
config_host_data.set('CONFIG_GETTID', has_gettid)
config_host_data.set('CONFIG_GNUTLS', gnutls.found())
config_host_data.set('CONFIG_GNUTLS_CRYPTO', gnutls_crypto.found())
+config_host_data.set('CONFIG_TASN1', tasn1.found())
config_host_data.set('CONFIG_GCRYPT', gcrypt.found())
config_host_data.set('CONFIG_NETTLE', nettle.found())
config_host_data.set('CONFIG_QEMU_PRIVATE_XTS', xts == 'private')
diff --git a/tests/qtest/meson.build b/tests/qtest/meson.build
index a95bb5def3..91dc36fb9b 100644
--- a/tests/qtest/meson.build
+++ b/tests/qtest/meson.build
@@ -279,6 +279,11 @@ tpmemu_files = ['tpm-emu.c', 'tpm-util.c', 'tpm-tests.c']
migration_files = [files('migration-helpers.c')]
if gnutls.found()
migration_files += [files('../unit/crypto-tls-psk-helpers.c'), gnutls]
+
+ if tasn1.found()
+ migration_files += [files('../unit/crypto-tls-x509-helpers.c',
+ '../unit/pkix_asn1_tab.c'), tasn1]
+ endif
endif
qtests = {
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 744a9f8123..4040443caa 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -29,6 +29,9 @@
#include "tests/migration/migration-test.h"
#ifdef CONFIG_GNUTLS
# include "tests/unit/crypto-tls-psk-helpers.h"
+# ifdef CONFIG_TASN1
+# include "tests/unit/crypto-tls-x509-helpers.h"
+# endif
#endif
/* For dirty ring test; so far only x86_64 is supported */
@@ -754,6 +757,237 @@ test_migrate_tls_psk_finish(QTestState *from,
g_free(data->pskfile);
g_free(data);
}
+
+#ifdef CONFIG_TASN1
+struct TestMigrateTLSX509Data {
+ char *workdir;
+ char *keyfile;
+ char *cacert;
+ char *servercert;
+ char *serverkey;
+ char *clientcert;
+ char *clientkey;
+};
+
+static void *
+test_migrate_tls_x509_start_common(QTestState *from,
+ QTestState *to,
+ bool verifyclient,
+ bool clientcert,
+ bool hostileclient,
+ bool authzclient,
+ const char *certhostname,
+ const char *certipaddr)
+{
+ struct TestMigrateTLSX509Data *data =
+ g_new0(struct TestMigrateTLSX509Data, 1);
+ QDict *rsp;
+
+ data->workdir = g_strdup_printf("%s/tlscredsx5090", tmpfs);
+ data->keyfile = g_strdup_printf("%s/key.pem", data->workdir);
+
+ data->cacert = g_strdup_printf("%s/ca-cert.pem", data->workdir);
+ data->serverkey = g_strdup_printf("%s/server-key.pem", data->workdir);
+ data->servercert = g_strdup_printf("%s/server-cert.pem", data->workdir);
+ if (clientcert) {
+ data->clientkey = g_strdup_printf("%s/client-key.pem", data->workdir);
+ data->clientcert = g_strdup_printf("%s/client-cert.pem", data->workdir);
+ }
+
+ mkdir(data->workdir, 0700);
+
+ test_tls_init(data->keyfile);
+ g_assert(link(data->keyfile, data->serverkey) == 0);
+ if (clientcert) {
+ g_assert(link(data->keyfile, data->clientkey) == 0);
+ }
+
+ TLS_ROOT_REQ_SIMPLE(cacertreq, data->cacert);
+ if (clientcert) {
+ TLS_CERT_REQ_SIMPLE_CLIENT(servercertreq, cacertreq,
+ hostileclient ?
+ QCRYPTO_TLS_TEST_CLIENT_HOSTILE_NAME :
+ QCRYPTO_TLS_TEST_CLIENT_NAME,
+ data->clientcert);
+ }
+
+ TLS_CERT_REQ_SIMPLE_SERVER(clientcertreq, cacertreq,
+ data->servercert,
+ certhostname, certipaddr);
+
+ rsp = wait_command(from,
+ "{ 'execute': 'object-add',"
+ " 'arguments': { 'qom-type': 'tls-creds-x509',"
+ " 'id': 'tlscredsx509client0',"
+ " 'endpoint': 'client',"
+ " 'dir': %s,"
+ " 'sanity-check': true,"
+ " 'verify-peer': true} }",
+ data->workdir);
+ qobject_unref(rsp);
+ migrate_set_parameter_str(from, "tls-creds", "tlscredsx509client0");
+ if (certhostname) {
+ migrate_set_parameter_str(from, "tls-hostname", certhostname);
+ }
+
+ rsp = wait_command(to,
+ "{ 'execute': 'object-add',"
+ " 'arguments': { 'qom-type': 'tls-creds-x509',"
+ " 'id': 'tlscredsx509server0',"
+ " 'endpoint': 'server',"
+ " 'dir': %s,"
+ " 'sanity-check': true,"
+ " 'verify-peer': %i} }",
+ data->workdir, verifyclient);
+ qobject_unref(rsp);
+ migrate_set_parameter_str(to, "tls-creds", "tlscredsx509server0");
+
+ if (authzclient) {
+ rsp = wait_command(to,
+ "{ 'execute': 'object-add',"
+ " 'arguments': { 'qom-type': 'authz-simple',"
+ " 'id': 'tlsauthz0',"
+ " 'identity': %s} }",
+ "CN=" QCRYPTO_TLS_TEST_CLIENT_NAME);
+ migrate_set_parameter_str(to, "tls-authz", "tlsauthz0");
+ }
+
+ return data;
+}
+
+/*
+ * The normal case: match server's cert hostname against
+ * whatever host we were telling QEMU to connect to (if any)
+ */
+static void *
+test_migrate_tls_x509_start_default_host(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ true, /* verifyclient */
+ true, /* clientcert */
+ false, /* hostileclient */
+ false, /* authzclient */
+ NULL,
+ "127.0.0.1");
+}
+
+/*
+ * The unusual case: the server's cert is different from
+ * the address we're telling QEMU to connect to (if any),
+ * so we must give QEMU an explicit hostname to validate
+ */
+static void *
+test_migrate_tls_x509_start_override_host(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ true, /* verifyclient */
+ true, /* clientcert */
+ false, /* hostileclient */
+ false, /* authzclient */
+ "qemu.org",
+ NULL);
+}
+
+/*
+ * The unusual case: the server's cert is different from
+ * the address we're telling QEMU to connect to, and so we
+ * expect the client to reject the server
+ */
+static void *
+test_migrate_tls_x509_start_mismatch_host(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ true, /* verifyclient */
+ true, /* clientcert */
+ false, /* hostileclient */
+ false, /* authzclient */
+ NULL,
+ "10.0.0.1");
+}
+
+static void *
+test_migrate_tls_x509_start_friendly_client(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ true, /* verifyclient */
+ true, /* clientcert */
+ false, /* hostileclient */
+ true, /* authzclient */
+ NULL,
+ "127.0.0.1");
+}
+
+static void *
+test_migrate_tls_x509_start_hostile_client(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ true, /* verifyclient */
+ true, /* clientcert */
+ true, /* hostileclient */
+ true, /* authzclient */
+ NULL,
+ "127.0.0.1");
+}
+
+/*
+ * The case with no client certificate presented,
+ * and no server verification
+ */
+static void *
+test_migrate_tls_x509_start_allow_anonymous_client(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ false, /* verifyclient */
+ false, /* clientcert */
+ false, /* hostileclient */
+ false, /* authzclient */
+ NULL,
+ "127.0.0.1");
+}
+
+/*
+ * The case with no client certificate presented,
+ * and server verification rejecting
+ */
+static void *
+test_migrate_tls_x509_start_reject_anonymous_client(QTestState *from,
+ QTestState *to)
+{
+ return test_migrate_tls_x509_start_common(from, to,
+ true, /* verifyclient */
+ false, /* clientcert */
+ false, /* hostileclient */
+ false, /* authzclient */
+ NULL,
+ "127.0.0.1");
+}
+
+static void
+test_migrate_tls_x509_finish(QTestState *from,
+ QTestState *to,
+ void *opaque)
+{
+ struct TestMigrateTLSX509Data *data = opaque;
+
+ test_tls_cleanup(data->keyfile);
+ unlink(data->cacert);
+ unlink(data->servercert);
+ unlink(data->serverkey);
+ unlink(data->clientcert);
+ unlink(data->clientkey);
+ rmdir(data->workdir);
+
+ g_free(data->workdir);
+ g_free(data->keyfile);
+ g_free(data);
+}
+#endif /* CONFIG_TASN1 */
#endif /* CONFIG_GNUTLS */
static int migrate_postcopy_prepare(QTestState **from_ptr,
@@ -1020,6 +1254,7 @@ static void test_precopy_common(const char *listen_uri,
static void test_precopy_unix_common(TestMigrateStartHook start_hook,
TestMigrateFinishHook finish_hook,
bool expect_fail,
+ bool dst_quit,
bool dirty_ring)
{
g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
@@ -1029,7 +1264,7 @@ static void test_precopy_unix_common(TestMigrateStartHook start_hook,
start_hook,
finish_hook,
expect_fail,
- false, /* dst_quit */
+ dst_quit,
dirty_ring);
}
@@ -1038,24 +1273,49 @@ static void test_precopy_unix_plain(void)
test_precopy_unix_common(NULL, /* start_hook */
NULL, /* finish_hook */
false, /* expect_fail */
+ false, /* dst_quit */
false /* dirty_ring */);
}
+static void test_precopy_unix_dirty_ring(void)
+{
+ test_precopy_unix_common(NULL, /* start_hook */
+ NULL, /* finish_hook */
+ false, /* clientReject */
+ false, /* dst_quit */
+ true /* dirty_ring */);
+}
+
+#ifdef CONFIG_GNUTLS
static void test_precopy_unix_tls_psk(void)
{
test_precopy_unix_common(test_migrate_tls_psk_start_match,
test_migrate_tls_psk_finish,
false, /* expect_fail */
+ false, /* dst_quit */
false /* dirty_ring */);
}
-static void test_precopy_unix_dirty_ring(void)
+#ifdef CONFIG_TASN1
+static void test_precopy_unix_tls_x509_default_host(void)
{
- test_precopy_unix_common(NULL, /* start_hook */
- NULL, /* finish_hook */
- false, /* clientReject */
- true /* dirty_ring */);
+ test_precopy_unix_common(test_migrate_tls_x509_start_default_host,
+ test_migrate_tls_x509_finish,
+ true, /* expect_fail */
+ true, /* dst_quit */
+ false /* dirty_ring */);
+}
+
+static void test_precopy_unix_tls_x509_override_host(void)
+{
+ test_precopy_unix_common(test_migrate_tls_x509_start_override_host,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false, /* dst_quit */
+ false /* dirty_ring */);
}
+#endif /* CONFIG_TASN1 */
+#endif /* CONFIG_GNUTLS */
#if 0
/* Currently upset on aarch64 TCG */
@@ -1149,14 +1409,15 @@ static void test_xbzrle_unix(void)
static void test_precopy_tcp_common(TestMigrateStartHook start_hook,
TestMigrateFinishHook finish_hook,
- bool expect_fail)
+ bool expect_fail,
+ bool dst_quit)
{
test_precopy_common("tcp:127.0.0.1:0",
NULL, /* connect_uri */
start_hook,
finish_hook,
expect_fail,
- false, /* dst_quit */
+ dst_quit,
false /* dirty_ring */);
}
@@ -1165,7 +1426,8 @@ static void test_precopy_tcp_plain(void)
{
test_precopy_tcp_common(NULL, /* start_hook */
NULL, /* finish_hook */
- false /* expect_fail */);
+ false, /* expect_fail */
+ false /* dst_quit */);
}
#ifdef CONFIG_GNUTLS
@@ -1173,15 +1435,75 @@ static void test_precopy_tcp_tls_psk_match(void)
{
test_precopy_tcp_common(test_migrate_tls_psk_start_match,
test_migrate_tls_psk_finish,
- false /* expect_fail */);
+ false, /* expect_fail */
+ false /* dst_quit */);
}
static void test_precopy_tcp_tls_psk_mismatch(void)
{
test_precopy_tcp_common(test_migrate_tls_psk_start_mismatch,
test_migrate_tls_psk_finish,
- true /* expect_fail */);
+ true, /* expect_fail */
+ false /* dst_quit */);
+}
+
+#ifdef CONFIG_TASN1
+static void test_precopy_tcp_tls_x509_default_host(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_default_host,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_precopy_tcp_tls_x509_override_host(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_override_host,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_precopy_tcp_tls_x509_mismatch_host(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_mismatch_host,
+ test_migrate_tls_x509_finish,
+ true, /* expect_fail */
+ true /* dst_quit */);
+}
+
+static void test_precopy_tcp_tls_x509_friendly_client(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_friendly_client,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_precopy_tcp_tls_x509_hostile_client(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_hostile_client,
+ test_migrate_tls_x509_finish,
+ true, /* expect_quit */
+ false /* dst_quit */);
+}
+
+static void test_precopy_tcp_tls_x509_allow_anonymous_client(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_allow_anonymous_client,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_precopy_tcp_tls_x509_reject_anonymous_client(void)
+{
+ test_precopy_tcp_common(test_migrate_tls_x509_start_reject_anonymous_client,
+ test_migrate_tls_x509_finish,
+ true, /* expect_fail */
+ false /* dst_quit */);
}
+#endif /* CONFIG_TASN1 */
#endif /* CONFIG_GNUTLS */
static void *test_migrate_fd_start_hook(QTestState *from,
@@ -1651,6 +1973,12 @@ int main(int argc, char **argv)
#ifdef CONFIG_GNUTLS
qtest_add_func("/migration/precopy/unix/tls/psk",
test_precopy_unix_tls_psk);
+#ifdef CONFIG_TASN1
+ qtest_add_func("/migration/precopy/unix/tls/x509/default-host",
+ test_precopy_unix_tls_x509_default_host);
+ qtest_add_func("/migration/precopy/unix/tls/x509/override-host",
+ test_precopy_unix_tls_x509_override_host);
+#endif /* CONFIG_TASN1 */
#endif /* CONFIG_GNUTLS */
qtest_add_func("/migration/precopy/tcp/plain", test_precopy_tcp_plain);
@@ -1659,6 +1987,22 @@ int main(int argc, char **argv)
test_precopy_tcp_tls_psk_match);
qtest_add_func("/migration/precopy/tcp/tls/psk/mismatch",
test_precopy_tcp_tls_psk_mismatch);
+#ifdef CONFIG_TASN1
+ qtest_add_func("/migration/precopy/tcp/tls/x509/default-host",
+ test_precopy_tcp_tls_x509_default_host);
+ qtest_add_func("/migration/precopy/tcp/tls/x509/override-host",
+ test_precopy_tcp_tls_x509_override_host);
+ qtest_add_func("/migration/precopy/tcp/tls/x509/mismatch-host",
+ test_precopy_tcp_tls_x509_mismatch_host);
+ qtest_add_func("/migration/precopy/tcp/tls/x509/friendly-client",
+ test_precopy_tcp_tls_x509_friendly_client);
+ qtest_add_func("/migration/precopy/tcp/tls/x509/hostile-client",
+ test_precopy_tcp_tls_x509_hostile_client);
+ qtest_add_func("/migration/precopy/tcp/tls/x509/allow-anonymous-client",
+ test_precopy_tcp_tls_x509_allow_anonymous_client);
+ qtest_add_func("/migration/precopy/tcp/tls/x509/reject-anonymous-client",
+ test_precopy_tcp_tls_x509_reject_anonymous_client);
+#endif /* CONFIG_TASN1 */
#endif /* CONFIG_GNUTLS */
/* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 14/18] tests: convert XBZRLE migration test to use common helper
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (12 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 13/18] tests: add migration tests of TLS with x509 credentials Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 8:01 ` Peter Xu
2022-03-02 17:49 ` [PATCH 15/18] tests: convert multifd migration tests " Daniel P. Berrangé
` (3 subsequent siblings)
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
Most of the XBZRLE migration test logic is common with the rest of the
precopy tests, so it can use the helper with just one small tweak.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 70 ++++++++++++++----------------------
1 file changed, 26 insertions(+), 44 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4040443caa..9896fcb134 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1162,6 +1162,7 @@ typedef void (*TestMigrateFinishHook)(QTestState *from,
* @expect_fail: true if we expect migration to fail
* @dst_quit: true if we expect the dst QEMU to quit with an
* abnormal exit status on failure
+ * @iterations: number of migration passes to wait for
* @dirty_ring: true to use dirty ring tracking
*
* If @connect_uri is NULL, then it will query the dst
@@ -1185,6 +1186,7 @@ static void test_precopy_common(const char *listen_uri,
TestMigrateFinishHook finish_hook,
bool expect_fail,
bool dst_quit,
+ unsigned int iterations,
bool dirty_ring)
{
MigrateStart *args = migrate_start_new();
@@ -1229,7 +1231,9 @@ static void test_precopy_common(const char *listen_uri,
qtest_set_expected_status(to, 1);
}
} else {
- wait_for_migration_pass(from);
+ while (iterations--) {
+ wait_for_migration_pass(from);
+ }
migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME);
@@ -1255,6 +1259,7 @@ static void test_precopy_unix_common(TestMigrateStartHook start_hook,
TestMigrateFinishHook finish_hook,
bool expect_fail,
bool dst_quit,
+ unsigned int iterations,
bool dirty_ring)
{
g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
@@ -1265,6 +1270,7 @@ static void test_precopy_unix_common(TestMigrateStartHook start_hook,
finish_hook,
expect_fail,
dst_quit,
+ iterations,
dirty_ring);
}
@@ -1274,6 +1280,7 @@ static void test_precopy_unix_plain(void)
NULL, /* finish_hook */
false, /* expect_fail */
false, /* dst_quit */
+ 1, /* iterations */
false /* dirty_ring */);
}
@@ -1283,6 +1290,7 @@ static void test_precopy_unix_dirty_ring(void)
NULL, /* finish_hook */
false, /* clientReject */
false, /* dst_quit */
+ 1, /* iterations */
true /* dirty_ring */);
}
@@ -1293,6 +1301,7 @@ static void test_precopy_unix_tls_psk(void)
test_migrate_tls_psk_finish,
false, /* expect_fail */
false, /* dst_quit */
+ 1, /* iterations */
false /* dirty_ring */);
}
@@ -1303,6 +1312,7 @@ static void test_precopy_unix_tls_x509_default_host(void)
test_migrate_tls_x509_finish,
true, /* expect_fail */
true, /* dst_quit */
+ 1, /* iterations */
false /* dirty_ring */);
}
@@ -1312,6 +1322,7 @@ static void test_precopy_unix_tls_x509_override_host(void)
test_migrate_tls_x509_finish,
false, /* expect_fail */
false, /* dst_quit */
+ 1, /* iterations */
false /* dirty_ring */);
}
#endif /* CONFIG_TASN1 */
@@ -1354,57 +1365,26 @@ static void test_ignore_shared(void)
}
#endif
-static void test_xbzrle(const char *uri)
+static void *
+test_migrate_xbzrle_start(QTestState *from,
+ QTestState *to)
{
- MigrateStart *args = migrate_start_new();
- QTestState *from, *to;
-
- if (test_migrate_start(&from, &to, uri, args)) {
- return;
- }
-
- /*
- * We want to pick a speed slow enough that the test completes
- * quickly, but that it doesn't complete precopy even on a slow
- * machine, so also set the downtime.
- */
- /* 1 ms should make it not converge*/
- migrate_set_parameter_int(from, "downtime-limit", 1);
- /* 1GB/s */
- migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
-
migrate_set_parameter_int(from, "xbzrle-cache-size", 33554432);
migrate_set_capability(from, "xbzrle", true);
migrate_set_capability(to, "xbzrle", true);
- /* Wait for the first serial output from the source */
- wait_for_serial("src_serial");
- migrate_qmp(from, uri, "{}");
-
- wait_for_migration_pass(from);
- /* Make sure we have 2 passes, so the xbzrle cache gets a workout */
- wait_for_migration_pass(from);
-
- /* 1000ms should converge */
- migrate_set_parameter_int(from, "downtime-limit", 1000);
-
- if (!got_stop) {
- qtest_qmp_eventwait(from, "STOP");
- }
- qtest_qmp_eventwait(to, "RESUME");
-
- wait_for_serial("dest_serial");
- wait_for_migration_complete(from);
-
- test_migrate_end(from, to, true);
+ return NULL;
}
-static void test_xbzrle_unix(void)
+static void test_precopy_unix_xbzrle(void)
{
- g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
-
- test_xbzrle(uri);
+ test_precopy_unix_common(test_migrate_xbzrle_start,
+ NULL, /* finish_hook */
+ false, /* expect_fail */
+ false, /* dst_quit */
+ 2, /* iterations */
+ false /* dirty_ring */);
}
static void test_precopy_tcp_common(TestMigrateStartHook start_hook,
@@ -1418,6 +1398,7 @@ static void test_precopy_tcp_common(TestMigrateStartHook start_hook,
finish_hook,
expect_fail,
dst_quit,
+ 1, /* iterations */
false /* dirty_ring */);
}
@@ -1572,6 +1553,7 @@ static void test_migrate_fd_proto(void)
test_migrate_fd_finish_hook,
false, /* expect_fail */
false, /* dst_quit */
+ 1, /* iterations */
false /* dirty_ring */);
}
@@ -1970,6 +1952,7 @@ int main(int argc, char **argv)
qtest_add_func("/migration/postcopy/recovery", test_postcopy_recovery);
qtest_add_func("/migration/bad_dest", test_baddest);
qtest_add_func("/migration/precopy/unix/plain", test_precopy_unix_plain);
+ qtest_add_func("/migration/precopy/unix/xbzrle", test_precopy_unix_xbzrle);
#ifdef CONFIG_GNUTLS
qtest_add_func("/migration/precopy/unix/tls/psk",
test_precopy_unix_tls_psk);
@@ -2006,7 +1989,6 @@ int main(int argc, char **argv)
#endif /* CONFIG_GNUTLS */
/* qtest_add_func("/migration/ignore_shared", test_ignore_shared); */
- qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
qtest_add_func("/migration/fd_proto", test_migrate_fd_proto);
qtest_add_func("/migration/validate_uuid", test_validate_uuid);
qtest_add_func("/migration/validate_uuid_error", test_validate_uuid_error);
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 15/18] tests: convert multifd migration tests to use common helper
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (13 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 14/18] tests: convert XBZRLE migration test to use common helper Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 16/18] tests: add multifd migration tests of TLS with PSK credentials Daniel P. Berrangé
` (2 subsequent siblings)
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
Most of the multifd migration test logic is common with the rest of the
precopy tests, so it can use the helper without difficulty. The only
exception of the multifd cancellation test which tries to run multiple
migrations in a row.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 75 +++++++++++++++++++-----------------
1 file changed, 39 insertions(+), 36 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9896fcb134..7c69268aa8 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1716,26 +1716,12 @@ static void test_migrate_auto_converge(void)
test_migrate_end(from, to, true);
}
-static void test_multifd_tcp(const char *method)
+static void *
+test_migration_precopy_tcp_multifd_start_common(QTestState *from,
+ QTestState *to,
+ const char *method)
{
- MigrateStart *args = migrate_start_new();
- QTestState *from, *to;
QDict *rsp;
- g_autofree char *uri = NULL;
-
- if (test_migrate_start(&from, &to, "defer", args)) {
- return;
- }
-
- /*
- * We want to pick a speed slow enough that the test completes
- * quickly, but that it doesn't complete precopy even on a slow
- * machine, so also set the downtime.
- */
- /* 1 ms should make it not converge*/
- migrate_set_parameter_int(from, "downtime-limit", 1);
- /* 1GB/s */
- migrate_set_parameter_int(from, "max-bandwidth", 1000000000);
migrate_set_parameter_int(from, "multifd-channels", 16);
migrate_set_parameter_int(to, "multifd-channels", 16);
@@ -1751,41 +1737,58 @@ static void test_multifd_tcp(const char *method)
" 'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
qobject_unref(rsp);
- /* Wait for the first serial output from the source */
- wait_for_serial("src_serial");
-
- uri = migrate_get_socket_address(to, "socket-address");
-
- migrate_qmp(from, uri, "{}");
+ return NULL;
+}
- wait_for_migration_pass(from);
+static void *
+test_migration_precopy_tcp_multifd_start(QTestState *from,
+ QTestState *to)
+{
+ return test_migration_precopy_tcp_multifd_start_common(from, to, "none");
+}
- migrate_set_parameter_int(from, "downtime-limit", CONVERGE_DOWNTIME);
+static void *
+test_migration_precopy_tcp_multifd_zlib_start(QTestState *from,
+ QTestState *to)
+{
+ return test_migration_precopy_tcp_multifd_start_common(from, to, "zlib");
+}
- if (!got_stop) {
- qtest_qmp_eventwait(from, "STOP");
- }
- qtest_qmp_eventwait(to, "RESUME");
+#ifdef CONFIG_ZSTD
+static void *
+test_migration_precopy_tcp_multifd_zstd_start(QTestState *from,
+ QTestState *to)
+{
+ return test_migration_precopy_tcp_multifd_start_common(from, to, "zstd");
+}
+#endif /* CONFIG_ZSTD */
- wait_for_serial("dest_serial");
- wait_for_migration_complete(from);
- test_migrate_end(from, to, true);
+static void test_multifd_tcp_common(TestMigrateStartHook start_hook)
+{
+ test_precopy_common("defer",
+ NULL, /* connect_uri */
+ start_hook,
+ NULL, /* finish_hook */
+ false, /* expect_fail */
+ false, /* dst_quit */
+ 1, /* iterations */
+ false /* dirty_ring */);
}
static void test_multifd_tcp_none(void)
{
- test_multifd_tcp("none");
+ test_multifd_tcp_common(test_migration_precopy_tcp_multifd_start);
}
static void test_multifd_tcp_zlib(void)
{
- test_multifd_tcp("zlib");
+ test_multifd_tcp_common(test_migration_precopy_tcp_multifd_zlib_start);
}
#ifdef CONFIG_ZSTD
static void test_multifd_tcp_zstd(void)
{
- test_multifd_tcp("zstd");
+ test_multifd_tcp_common(test_migration_precopy_tcp_multifd_zstd_start);
}
#endif
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 16/18] tests: add multifd migration tests of TLS with PSK credentials
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (14 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 15/18] tests: convert multifd migration tests " Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 17/18] tests: add multifd migration tests of TLS with x509 credentials Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 18/18] tests: ensure migration status isn't reported as failed Daniel P. Berrangé
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
This validates that we correctly handle multifd migration success
and failure scenarios when using TLS with pre shared keys.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 94 ++++++++++++++++++++++++++++--------
1 file changed, 75 insertions(+), 19 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7c69268aa8..506c6996e0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1717,9 +1717,9 @@ static void test_migrate_auto_converge(void)
}
static void *
-test_migration_precopy_tcp_multifd_start_common(QTestState *from,
- QTestState *to,
- const char *method)
+test_migrate_precopy_tcp_multifd_start_common(QTestState *from,
+ QTestState *to,
+ const char *method)
{
QDict *rsp;
@@ -1741,25 +1741,25 @@ test_migration_precopy_tcp_multifd_start_common(QTestState *from,
}
static void *
-test_migration_precopy_tcp_multifd_start(QTestState *from,
- QTestState *to)
+test_migrate_precopy_tcp_multifd_start(QTestState *from,
+ QTestState *to)
{
- return test_migration_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
}
static void *
-test_migration_precopy_tcp_multifd_zlib_start(QTestState *from,
- QTestState *to)
+test_migrate_precopy_tcp_multifd_zlib_start(QTestState *from,
+ QTestState *to)
{
- return test_migration_precopy_tcp_multifd_start_common(from, to, "zlib");
+ return test_migrate_precopy_tcp_multifd_start_common(from, to, "zlib");
}
#ifdef CONFIG_ZSTD
static void *
-test_migration_precopy_tcp_multifd_zstd_start(QTestState *from,
- QTestState *to)
+test_migrate_precopy_tcp_multifd_zstd_start(QTestState *from,
+ QTestState *to)
{
- return test_migration_precopy_tcp_multifd_start_common(from, to, "zstd");
+ return test_migrate_precopy_tcp_multifd_start_common(from, to, "zstd");
}
#endif /* CONFIG_ZSTD */
@@ -1777,18 +1777,64 @@ static void test_multifd_tcp_common(TestMigrateStartHook start_hook)
static void test_multifd_tcp_none(void)
{
- test_multifd_tcp_common(test_migration_precopy_tcp_multifd_start);
+ test_multifd_tcp_common(test_migrate_precopy_tcp_multifd_start);
}
static void test_multifd_tcp_zlib(void)
{
- test_multifd_tcp_common(test_migration_precopy_tcp_multifd_zlib_start);
+ test_multifd_tcp_common(test_migrate_precopy_tcp_multifd_zlib_start);
}
#ifdef CONFIG_ZSTD
static void test_multifd_tcp_zstd(void)
{
- test_multifd_tcp_common(test_migration_precopy_tcp_multifd_zstd_start);
+ test_multifd_tcp_common(test_migrate_precopy_tcp_multifd_zstd_start);
+}
+#endif
+
+#ifdef CONFIG_GNUTLS
+static void test_multifd_tcp_tls_common(TestMigrateStartHook start_hook,
+ TestMigrateFinishHook finish_hook,
+ bool expect_fail)
+{
+ test_precopy_common("defer",
+ NULL, /* connect_uri */
+ start_hook,
+ finish_hook,
+ expect_fail,
+ false, /* dst_quit */
+ 1, /* iterations */
+ false /* dirty_ring */);
+}
+
+static void *
+test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_psk_start_match(from, to);
+}
+
+static void *
+test_migrate_multifd_tcp_tls_psk_start_mismatch(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_psk_start_mismatch(from, to);
+}
+
+static void test_multifd_tcp_tls_psk_match(void)
+{
+ test_multifd_tcp_tls_common(test_migrate_multifd_tcp_tls_psk_start_match,
+ test_migrate_tls_psk_finish,
+ false /* expect_fail */);
+}
+
+static void test_multifd_tcp_tls_psk_mismatch(void)
+{
+ test_multifd_tcp_tls_common(test_migrate_multifd_tcp_tls_psk_start_mismatch,
+ test_migrate_tls_psk_finish,
+ true /* expect_fail */);
}
#endif
@@ -2001,12 +2047,22 @@ int main(int argc, char **argv)
test_validate_uuid_dst_not_set);
qtest_add_func("/migration/auto_converge", test_migrate_auto_converge);
- qtest_add_func("/migration/multifd/tcp/none", test_multifd_tcp_none);
- qtest_add_func("/migration/multifd/tcp/cancel", test_multifd_tcp_cancel);
- qtest_add_func("/migration/multifd/tcp/zlib", test_multifd_tcp_zlib);
+ qtest_add_func("/migration/multifd/tcp/plain/none",
+ test_multifd_tcp_none);
+ qtest_add_func("/migration/multifd/tcp/plain/cancel",
+ test_multifd_tcp_cancel);
+ qtest_add_func("/migration/multifd/tcp/plain/zlib",
+ test_multifd_tcp_zlib);
#ifdef CONFIG_ZSTD
- qtest_add_func("/migration/multifd/tcp/zstd", test_multifd_tcp_zstd);
+ qtest_add_func("/migration/multifd/tcp/plain/zstd",
+ test_multifd_tcp_zstd);
#endif
+#ifdef CONFIG_GNUTLS
+ qtest_add_func("/migration/multifd/tcp/tls/psk/match",
+ test_multifd_tcp_tls_psk_match);
+ qtest_add_func("/migration/multifd/tcp/tls/psk/mismatch",
+ test_multifd_tcp_tls_psk_mismatch);
+#endif /* CONFIG_GNUTLS */
if (kvm_dirty_ring_supported()) {
qtest_add_func("/migration/dirty_ring",
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 17/18] tests: add multifd migration tests of TLS with x509 credentials
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (15 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 16/18] tests: add multifd migration tests of TLS with PSK credentials Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 18/18] tests: ensure migration status isn't reported as failed Daniel P. Berrangé
17 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
This validates that we correctly handle multifd migration success
and failure scenarios when using TLS with x509 certificates. There
are quite a few different scenarios that matter in relation to
hostname validation, but we skip a couple as we can assume that
the non-multifd coverage applies to some extent.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-test.c | 135 ++++++++++++++++++++++++++++++++---
1 file changed, 126 insertions(+), 9 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 506c6996e0..95ae843e1b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1795,20 +1795,21 @@ static void test_multifd_tcp_zstd(void)
#ifdef CONFIG_GNUTLS
static void test_multifd_tcp_tls_common(TestMigrateStartHook start_hook,
TestMigrateFinishHook finish_hook,
- bool expect_fail)
+ bool expect_fail,
+ bool dst_quit)
{
test_precopy_common("defer",
NULL, /* connect_uri */
start_hook,
finish_hook,
expect_fail,
- false, /* dst_quit */
+ dst_quit,
1, /* iterations */
false /* dirty_ring */);
}
static void *
-test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
+test_migrate_multifd_tls_psk_start_match(QTestState *from,
QTestState *to)
{
test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
@@ -1816,27 +1817,131 @@ test_migrate_multifd_tcp_tls_psk_start_match(QTestState *from,
}
static void *
-test_migrate_multifd_tcp_tls_psk_start_mismatch(QTestState *from,
+test_migrate_multifd_tls_psk_start_mismatch(QTestState *from,
QTestState *to)
{
test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
return test_migrate_tls_psk_start_mismatch(from, to);
}
+#ifdef CONFIG_TASN1
+static void *
+test_migrate_multifd_tls_x509_start_default_host(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_x509_start_default_host(from, to);
+}
+
+static void *
+test_migrate_multifd_tls_x509_start_override_host(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_x509_start_override_host(from, to);
+}
+
+static void *
+test_migrate_multifd_tls_x509_start_mismatch_host(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_x509_start_mismatch_host(from, to);
+}
+
+static void *
+test_migrate_multifd_tls_x509_start_allow_anonymous_client(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_x509_start_allow_anonymous_client(from, to);
+}
+
+static void *
+test_migrate_multifd_tls_x509_start_reject_anonymous_client(QTestState *from,
+ QTestState *to)
+{
+ test_migrate_precopy_tcp_multifd_start_common(from, to, "none");
+ return test_migrate_tls_x509_start_reject_anonymous_client(from, to);
+}
+#endif /* CONFIG_TASN1 */
+
static void test_multifd_tcp_tls_psk_match(void)
{
- test_multifd_tcp_tls_common(test_migrate_multifd_tcp_tls_psk_start_match,
+ test_multifd_tcp_tls_common(test_migrate_multifd_tls_psk_start_match,
test_migrate_tls_psk_finish,
- false /* expect_fail */);
+ false, /* expect_fail */
+ false /* dst_quit */);
}
static void test_multifd_tcp_tls_psk_mismatch(void)
{
- test_multifd_tcp_tls_common(test_migrate_multifd_tcp_tls_psk_start_mismatch,
+ test_multifd_tcp_tls_common(test_migrate_multifd_tls_psk_start_mismatch,
test_migrate_tls_psk_finish,
- true /* expect_fail */);
+ true, /* expect_fail */
+ false /* dst_quit */);
}
-#endif
+
+#ifdef CONFIG_TASN1
+static void test_multifd_tcp_tls_x509_default_host(void)
+{
+ test_multifd_tcp_tls_common(
+ test_migrate_multifd_tls_x509_start_default_host,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_multifd_tcp_tls_x509_override_host(void)
+{
+ test_multifd_tcp_tls_common(
+ test_migrate_multifd_tls_x509_start_override_host,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_multifd_tcp_tls_x509_mismatch_host(void)
+{
+ /*
+ * This has different behaviour to the non-multifd case.
+ *
+ * In non-multifd case when client aborts due to mismatched
+ * cert host, the server has already started trying to load
+ * migration state, and so it exits with I/O failure.
+ *
+ * In multifd case when client aborts due to mismatched
+ * cert host, the server is still waiting for the other
+ * multifd connections to arrive so hasn't started trying
+ * to load migration state, and thus just aborts the migration
+ * without exiting
+ */
+ test_multifd_tcp_tls_common(
+ test_migrate_multifd_tls_x509_start_mismatch_host,
+ test_migrate_tls_x509_finish,
+ true, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_multifd_tcp_tls_x509_allow_anonymous_client(void)
+{
+ test_multifd_tcp_tls_common(
+ test_migrate_multifd_tls_x509_start_allow_anonymous_client,
+ test_migrate_tls_x509_finish,
+ false, /* expect_fail */
+ false /* dst_quit */);
+}
+
+static void test_multifd_tcp_tls_x509_reject_anonymous_client(void)
+{
+ test_multifd_tcp_tls_common(
+ test_migrate_multifd_tls_x509_start_reject_anonymous_client,
+ test_migrate_tls_x509_finish,
+ true, /* expect_fail */
+ false /* dst_quit */);
+}
+#endif /* CONFIG_TASN1 */
+#endif /* CONFIHG_GNUTLS */
/*
* This test does:
@@ -2062,6 +2167,18 @@ int main(int argc, char **argv)
test_multifd_tcp_tls_psk_match);
qtest_add_func("/migration/multifd/tcp/tls/psk/mismatch",
test_multifd_tcp_tls_psk_mismatch);
+#ifdef CONFIG_TASN1
+ qtest_add_func("/migration/multifd/tcp/tls/x509/default-host",
+ test_multifd_tcp_tls_x509_default_host);
+ qtest_add_func("/migration/multifd/tcp/tls/x509/override-host",
+ test_multifd_tcp_tls_x509_override_host);
+ qtest_add_func("/migration/multifd/tcp/tls/x509/mismatch-host",
+ test_multifd_tcp_tls_x509_mismatch_host);
+ qtest_add_func("/migration/multifd/tcp/tls/x509/allow-anonymous-client",
+ test_multifd_tcp_tls_x509_allow_anonymous_client);
+ qtest_add_func("/migration/multifd/tcp/tls/x509/reject-anonymous-client",
+ test_multifd_tcp_tls_x509_reject_anonymous_client);
+#endif /* CONFIG_TASN1 */
#endif /* CONFIG_GNUTLS */
if (kvm_dirty_ring_supported()) {
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* [PATCH 18/18] tests: ensure migration status isn't reported as failed
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
` (16 preceding siblings ...)
2022-03-02 17:49 ` [PATCH 17/18] tests: add multifd migration tests of TLS with x509 credentials Daniel P. Berrangé
@ 2022-03-02 17:49 ` Daniel P. Berrangé
2022-03-07 8:09 ` Peter Xu
17 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-02 17:49 UTC (permalink / raw)
To: qemu-devel
Cc: Laurent Vivier, Thomas Huth, Daniel P. Berrangé,
Juan Quintela, Dr. David Alan Gilbert, Peter Xu, Paolo Bonzini
Various methods in the migration test call 'query_migrate' to fetch the
current status and then access a particular field. Almost all of these
cases expect the migration to be in a non-failed state. In the case of
'wait_for_migration_pass' in particular, if the status is 'failed' then
it will get into an infinite loop. By validating that the status is
not 'failed' the test suite will assert rather than hang when getting
into an unexpected state.
Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
---
tests/qtest/migration-helpers.c | 13 +++++++++++++
tests/qtest/migration-helpers.h | 1 +
tests/qtest/migration-test.c | 6 +++---
3 files changed, 17 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 4ee26014b7..a6aa59e4e6 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -107,6 +107,19 @@ QDict *migrate_query(QTestState *who)
return wait_command(who, "{ 'execute': 'query-migrate' }");
}
+QDict *migrate_query_not_failed(QTestState *who)
+{
+ const char *status;
+ QDict *rsp = migrate_query(who);
+ status = qdict_get_str(rsp, "status");
+ if (g_str_equal(status, "failed")) {
+ g_printerr("query-migrate shows failed migration: %s\n",
+ qdict_get_str(rsp, "error-desc"));
+ }
+ g_assert(!g_str_equal(status, "failed"));
+ return rsp;
+}
+
/*
* Note: caller is responsible to free the returned object via
* g_free() after use
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index d63bba9630..b710ece67e 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -26,6 +26,7 @@ GCC_FMT_ATTR(3, 4)
void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
QDict *migrate_query(QTestState *who);
+QDict *migrate_query_not_failed(QTestState *who);
void wait_for_migration_status(QTestState *who,
const char *goal, const char **ungoals);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 95ae843e1b..3570a7895c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -181,7 +181,7 @@ static int64_t read_ram_property_int(QTestState *who, const char *property)
QDict *rsp_return, *rsp_ram;
int64_t result;
- rsp_return = migrate_query(who);
+ rsp_return = migrate_query_not_failed(who);
if (!qdict_haskey(rsp_return, "ram")) {
/* Still in setup */
result = 0;
@@ -198,7 +198,7 @@ static int64_t read_migrate_property_int(QTestState *who, const char *property)
QDict *rsp_return;
int64_t result;
- rsp_return = migrate_query(who);
+ rsp_return = migrate_query_not_failed(who);
result = qdict_get_try_int(rsp_return, property, 0);
qobject_unref(rsp_return);
return result;
@@ -213,7 +213,7 @@ static void read_blocktime(QTestState *who)
{
QDict *rsp_return;
- rsp_return = migrate_query(who);
+ rsp_return = migrate_query_not_failed(who);
g_assert(qdict_haskey(rsp_return, "postcopy-blocktime"));
qobject_unref(rsp_return);
}
--
2.34.1
^ permalink raw reply related [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-02 17:49 ` [PATCH 04/18] tests: print newline after QMP response in qtest logs Daniel P. Berrangé
@ 2022-03-07 6:51 ` Peter Xu
2022-03-07 10:06 ` Daniel P. Berrangé
0 siblings, 1 reply; 48+ messages in thread
From: Peter Xu @ 2022-03-07 6:51 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> The QMP commands have a trailing newline, but the response does not.
> This makes the qtest logs hard to follow as the next QMP command
> appears in the same line as the previous QMP response.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> tests/qtest/libqtest.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index a85f8a6d05..79c3edcf4b 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> }
> json_message_parser_feed(&qmp.parser, &c, 1);
> }
> + if (log) {
> + g_assert(write(2, "\n", 1) == 1);
> + }
Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 02/18] tests: improve error message when saving TLS PSK file fails
2022-03-02 17:49 ` [PATCH 02/18] tests: improve error message when saving TLS PSK file fails Daniel P. Berrangé
@ 2022-03-07 6:52 ` Peter Xu
0 siblings, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 6:52 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:16PM +0000, Daniel P. Berrangé wrote:
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 03/18] tests: support QTEST_TRACE env variable
2022-03-02 17:49 ` [PATCH 03/18] tests: support QTEST_TRACE env variable Daniel P. Berrangé
@ 2022-03-07 6:53 ` Peter Xu
2022-03-07 10:06 ` Thomas Huth
1 sibling, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 6:53 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:17PM +0000, Daniel P. Berrangé wrote:
> When debugging failing qtests it is useful to be able to turn on trace
> output to stderr. The QTEST_TRACE env variable contents get injected
> as a '-trace <str>' command line arg
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket
2022-03-02 17:49 ` [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket Daniel P. Berrangé
@ 2022-03-07 7:08 ` Peter Xu
2022-03-07 10:08 ` Daniel P. Berrangé
0 siblings, 1 reply; 48+ messages in thread
From: Peter Xu @ 2022-03-07 7:08 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:21PM +0000, Daniel P. Berrangé wrote:
> The migration TLS code has a check mandating that a hostname be
> available when starting a TLS session. This is expected when using
> x509 credentials, but is bogus for PSK and anonymous credentials
> as neither involve hostname validation.
>
> The TLS crdentials object gained suitable error reporting in the
> case of TLS with x509 credentials, so there is no longer any need
> for the migration code to do its own (incorrect) validation.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Do we need a Fixes tag for this?
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests
2022-03-02 17:49 ` [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests Daniel P. Berrangé
@ 2022-03-07 7:16 ` Peter Xu
2022-03-07 10:11 ` Thomas Huth
1 sibling, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 7:16 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:22PM +0000, Daniel P. Berrangé wrote:
> The test cases differ only in the URI they provide to the migration
> commands, and the ability to set the dirty_ring mode. This code is
> trivially merged into a common helper.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 09/18] tests: introduce ability to provide hooks for migration precopy test
2022-03-02 17:49 ` [PATCH 09/18] tests: introduce ability to provide hooks for migration precopy test Daniel P. Berrangé
@ 2022-03-07 7:19 ` Peter Xu
0 siblings, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 7:19 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:23PM +0000, Daniel P. Berrangé wrote:
> There are alot of different scenarios to test with migration due to the
> wide number of parameters and capabilities available. To enable sharing
> of the basic precopy test scenario, we need to be able to set arbitrary
> parameters and capabilities before the migration is initiated, but don't
> want to have all this logic in the common helper function. Solve this
> by defining two hooks that can be provided by the test case, one before
> migration starts and one after migration finishes.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 10/18] tests: switch migration FD passing test to use common precopy helper
2022-03-02 17:49 ` [PATCH 10/18] tests: switch migration FD passing test to use common precopy helper Daniel P. Berrangé
@ 2022-03-07 7:22 ` Peter Xu
0 siblings, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 7:22 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:24PM +0000, Daniel P. Berrangé wrote:
> The combination of the start and finish hooks allow the FD passing
> code to use the precopy helper
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 11/18] tests: expand the migration precopy helper to support failures
2022-03-02 17:49 ` [PATCH 11/18] tests: expand the migration precopy helper to support failures Daniel P. Berrangé
@ 2022-03-07 7:39 ` Peter Xu
2022-03-07 10:10 ` Daniel P. Berrangé
2022-03-07 7:57 ` Peter Xu
1 sibling, 1 reply; 48+ messages in thread
From: Peter Xu @ 2022-03-07 7:39 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote:
> The migration precopy testing helper function always expects the
> migration to run to a completion state. There will be test scenarios
> for TLS where expect either the client or server to fail the migration.
> This expands the helper to cope with these scenarios.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> tests/qtest/migration-test.c | 47 +++++++++++++++++++++++++++++-------
> 1 file changed, 38 insertions(+), 9 deletions(-)
>
> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> index 2082c58e8b..e40b408988 100644
> --- a/tests/qtest/migration-test.c
> +++ b/tests/qtest/migration-test.c
> @@ -827,17 +827,32 @@ typedef void (*TestMigrateFinishHook)(QTestState *from,
> * @connect_uri: the URI for the src QEMU to connect to
> * @start_hook: (optional) callback to run at start to set migration parameters
> * @finish_hook: (optional) callback to run at finish to cleanup
> + * @expect_fail: true if we expect migration to fail
> + * @dst_quit: true if we expect the dst QEMU to quit with an
> + * abnormal exit status on failure
"dst_quit" sounds a bit confusing to me, as setting dst_quit=false seems to
mean "dest qemu should not quit" but it's actually for checking an abnormal
quit only.
Rename may work. Or, IMHO it's nicer if we could merge the two parameters:
@expected_result: What is the expectation of this migration test
typedef enum {
/* This test should succeed, the default */
MIG_TEST_SUCCEED = 0,
/* This test should fail, dest qemu should keep alive */
MIG_TEST_FAIL,
/* This test should fail, dest qemu should fail with abnormal status */
MIG_TEST_FAIL_DEST_QUIT_ERR,
};
Because fundamentally the two parameters are correlated, e.g. there is no
combination of expect_fail==false && dst_quit==true.
No strong opinion, though.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 11/18] tests: expand the migration precopy helper to support failures
2022-03-02 17:49 ` [PATCH 11/18] tests: expand the migration precopy helper to support failures Daniel P. Berrangé
2022-03-07 7:39 ` Peter Xu
@ 2022-03-07 7:57 ` Peter Xu
2022-03-10 16:18 ` Daniel P. Berrangé
1 sibling, 1 reply; 48+ messages in thread
From: Peter Xu @ 2022-03-07 7:57 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote:
> static void test_precopy_common(const char *listen_uri,
> const char *connect_uri,
> TestMigrateStartHook start_hook,
> TestMigrateFinishHook finish_hook,
> + bool expect_fail,
> + bool dst_quit,
> bool dirty_ring)
> {
> MigrateStart *args = migrate_start_new();
> @@ -875,24 +890,32 @@ static void test_precopy_common(const char *listen_uri,
>
> migrate_qmp(from, connect_uri, "{}");
>
> - wait_for_migration_pass(from);
> + if (expect_fail) {
> + wait_for_migration_fail(from, !dst_quit);
Two more thoughts..
(1) Shall we move MigrateStart creation to be even upper? Then we avoid
passing over these parameters but merge these new parameters into
MigrateStart too. After all we used to have similar long lists of
params and we merged them into MigrateStart.
(2) Shall we leverage MigrateStart.hide_stderr? I saw a bunch of errors
dumped even if all things run as expected.
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 14/18] tests: convert XBZRLE migration test to use common helper
2022-03-02 17:49 ` [PATCH 14/18] tests: convert XBZRLE migration test to use common helper Daniel P. Berrangé
@ 2022-03-07 8:01 ` Peter Xu
0 siblings, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 8:01 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:28PM +0000, Daniel P. Berrangé wrote:
> @@ -1255,6 +1259,7 @@ static void test_precopy_unix_common(TestMigrateStartHook start_hook,
> TestMigrateFinishHook finish_hook,
> bool expect_fail,
> bool dst_quit,
> + unsigned int iterations,
> bool dirty_ring)
> {
> g_autofree char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
> @@ -1265,6 +1270,7 @@ static void test_precopy_unix_common(TestMigrateStartHook start_hook,
> finish_hook,
> expect_fail,
> dst_quit,
> + iterations,
> dirty_ring);
> }
>
> @@ -1274,6 +1280,7 @@ static void test_precopy_unix_plain(void)
> NULL, /* finish_hook */
> false, /* expect_fail */
> false, /* dst_quit */
> + 1, /* iterations */
> false /* dirty_ring */);
> }
>
> @@ -1283,6 +1290,7 @@ static void test_precopy_unix_dirty_ring(void)
> NULL, /* finish_hook */
> false, /* clientReject */
> false, /* dst_quit */
> + 1, /* iterations */
> true /* dirty_ring */);
> }
>
> @@ -1293,6 +1301,7 @@ static void test_precopy_unix_tls_psk(void)
> test_migrate_tls_psk_finish,
> false, /* expect_fail */
> false, /* dst_quit */
> + 1, /* iterations */
> false /* dirty_ring */);
> }
>
> @@ -1303,6 +1312,7 @@ static void test_precopy_unix_tls_x509_default_host(void)
> test_migrate_tls_x509_finish,
> true, /* expect_fail */
> true, /* dst_quit */
> + 1, /* iterations */
> false /* dirty_ring */);
> }
>
> @@ -1312,6 +1322,7 @@ static void test_precopy_unix_tls_x509_override_host(void)
> test_migrate_tls_x509_finish,
> false, /* expect_fail */
> false, /* dst_quit */
> + 1, /* iterations */
> false /* dirty_ring */);
Another side benefit to merge parameters (e.g. the new "iterations") into
MigrateStart (which I mentioned in the other thread) is that we don't need
to touch the value in every test if there's a default, because we will set
the default in migrate_start_new() and we only need a tweak on tests that
want to overwrite the default values.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 18/18] tests: ensure migration status isn't reported as failed
2022-03-02 17:49 ` [PATCH 18/18] tests: ensure migration status isn't reported as failed Daniel P. Berrangé
@ 2022-03-07 8:09 ` Peter Xu
0 siblings, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 8:09 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Wed, Mar 02, 2022 at 05:49:32PM +0000, Daniel P. Berrangé wrote:
> Various methods in the migration test call 'query_migrate' to fetch the
> current status and then access a particular field. Almost all of these
> cases expect the migration to be in a non-failed state. In the case of
> 'wait_for_migration_pass' in particular, if the status is 'failed' then
> it will get into an infinite loop. By validating that the status is
> not 'failed' the test suite will assert rather than hang when getting
> into an unexpected state.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-07 6:51 ` Peter Xu
@ 2022-03-07 10:06 ` Daniel P. Berrangé
2022-03-07 10:09 ` Thomas Huth
0 siblings, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-07 10:06 UTC (permalink / raw)
To: Peter Xu
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > The QMP commands have a trailing newline, but the response does not.
> > This makes the qtest logs hard to follow as the next QMP command
> > appears in the same line as the previous QMP response.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > tests/qtest/libqtest.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > index a85f8a6d05..79c3edcf4b 100644
> > --- a/tests/qtest/libqtest.c
> > +++ b/tests/qtest/libqtest.c
> > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > }
> > json_message_parser_feed(&qmp.parser, &c, 1);
> > }
> > + if (log) {
> > + g_assert(write(2, "\n", 1) == 1);
> > + }
>
> Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
You need to check the return value of write() otherwise you'll get a
compile failure due to a warn_unused_result attribute annotation.
I don't think G_DISABLE_ASSERT is a problem as we're not defining
that in our code.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 03/18] tests: support QTEST_TRACE env variable
2022-03-02 17:49 ` [PATCH 03/18] tests: support QTEST_TRACE env variable Daniel P. Berrangé
2022-03-07 6:53 ` Peter Xu
@ 2022-03-07 10:06 ` Thomas Huth
1 sibling, 0 replies; 48+ messages in thread
From: Thomas Huth @ 2022-03-07 10:06 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Laurent Vivier, Paolo Bonzini, Dr. David Alan Gilbert, Peter Xu,
Juan Quintela
On 02/03/2022 18.49, Daniel P. Berrangé wrote:
> When debugging failing qtests it is useful to be able to turn on trace
> output to stderr. The QTEST_TRACE env variable contents get injected
> as a '-trace <str>' command line arg
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> tests/qtest/libqtest.c | 8 ++++++--
> 1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> index 41f4da4e54..a85f8a6d05 100644
> --- a/tests/qtest/libqtest.c
> +++ b/tests/qtest/libqtest.c
> @@ -260,6 +260,9 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
> gchar *qmp_socket_path;
> gchar *command;
> const char *qemu_binary = qtest_qemu_binary();
> + const char *trace = g_getenv("QTEST_TRACE");
> + g_autofree char *tracearg = trace ?
> + g_strdup_printf("-trace %s ", trace) : g_strdup("");
>
> s = g_new(QTestState, 1);
>
> @@ -282,14 +285,15 @@ QTestState *qtest_init_without_qmp_handshake(const char *extra_args)
>
> qtest_add_abrt_handler(kill_qemu_hook_func, s);
>
> - command = g_strdup_printf("exec %s "
> + command = g_strdup_printf("exec %s %s"
> "-qtest unix:%s "
> "-qtest-log %s "
> "-chardev socket,path=%s,id=char0 "
> "-mon chardev=char0,mode=control "
> "-display none "
> "%s"
> - " -accel qtest", qemu_binary, socket_path,
> + " -accel qtest",
> + qemu_binary, tracearg, socket_path,
> getenv("QTEST_LOG") ? "/dev/fd/2" : "/dev/null",
> qmp_socket_path,
> extra_args ?: "");
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket
2022-03-07 7:08 ` Peter Xu
@ 2022-03-07 10:08 ` Daniel P. Berrangé
0 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-07 10:08 UTC (permalink / raw)
To: Peter Xu
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 03:08:53PM +0800, Peter Xu wrote:
> On Wed, Mar 02, 2022 at 05:49:21PM +0000, Daniel P. Berrangé wrote:
> > The migration TLS code has a check mandating that a hostname be
> > available when starting a TLS session. This is expected when using
> > x509 credentials, but is bogus for PSK and anonymous credentials
> > as neither involve hostname validation.
> >
> > The TLS crdentials object gained suitable error reporting in the
> > case of TLS with x509 credentials, so there is no longer any need
> > for the migration code to do its own (incorrect) validation.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>
> Do we need a Fixes tag for this?
It is fuzzy as we never really intended for UNIX sockets to use TLS
originally.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-07 10:06 ` Daniel P. Berrangé
@ 2022-03-07 10:09 ` Thomas Huth
2022-03-07 10:20 ` Peter Xu
2022-03-10 10:55 ` Daniel P. Berrangé
0 siblings, 2 replies; 48+ messages in thread
From: Thomas Huth @ 2022-03-07 10:09 UTC (permalink / raw)
To: Daniel P. Berrangé, Peter Xu
Cc: Laurent Vivier, Paolo Bonzini, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert
On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
>> On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
>>> The QMP commands have a trailing newline, but the response does not.
>>> This makes the qtest logs hard to follow as the next QMP command
>>> appears in the same line as the previous QMP response.
>>>
>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>> ---
>>> tests/qtest/libqtest.c | 3 +++
>>> 1 file changed, 3 insertions(+)
>>>
>>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>>> index a85f8a6d05..79c3edcf4b 100644
>>> --- a/tests/qtest/libqtest.c
>>> +++ b/tests/qtest/libqtest.c
>>> @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
>>> }
>>> json_message_parser_feed(&qmp.parser, &c, 1);
>>> }
>>> + if (log) {
>>> + g_assert(write(2, "\n", 1) == 1);
>>> + }
>>
>> Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
>
> You need to check the return value of write() otherwise you'll get a
> compile failure due to a warn_unused_result attribute annotation.
>
> I don't think G_DISABLE_ASSERT is a problem as we're not defining
> that in our code.
You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
Anyway:
Reviewed-by: Thomas Huth <thuth@redhat.com>
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 11/18] tests: expand the migration precopy helper to support failures
2022-03-07 7:39 ` Peter Xu
@ 2022-03-07 10:10 ` Daniel P. Berrangé
0 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-07 10:10 UTC (permalink / raw)
To: Peter Xu
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 03:39:22PM +0800, Peter Xu wrote:
> On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote:
> > The migration precopy testing helper function always expects the
> > migration to run to a completion state. There will be test scenarios
> > for TLS where expect either the client or server to fail the migration.
> > This expands the helper to cope with these scenarios.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > tests/qtest/migration-test.c | 47 +++++++++++++++++++++++++++++-------
> > 1 file changed, 38 insertions(+), 9 deletions(-)
> >
> > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
> > index 2082c58e8b..e40b408988 100644
> > --- a/tests/qtest/migration-test.c
> > +++ b/tests/qtest/migration-test.c
> > @@ -827,17 +827,32 @@ typedef void (*TestMigrateFinishHook)(QTestState *from,
> > * @connect_uri: the URI for the src QEMU to connect to
> > * @start_hook: (optional) callback to run at start to set migration parameters
> > * @finish_hook: (optional) callback to run at finish to cleanup
> > + * @expect_fail: true if we expect migration to fail
> > + * @dst_quit: true if we expect the dst QEMU to quit with an
> > + * abnormal exit status on failure
>
> "dst_quit" sounds a bit confusing to me, as setting dst_quit=false seems to
> mean "dest qemu should not quit" but it's actually for checking an abnormal
> quit only.
>
> Rename may work. Or, IMHO it's nicer if we could merge the two parameters:
>
> @expected_result: What is the expectation of this migration test
>
> typedef enum {
> /* This test should succeed, the default */
> MIG_TEST_SUCCEED = 0,
> /* This test should fail, dest qemu should keep alive */
> MIG_TEST_FAIL,
> /* This test should fail, dest qemu should fail with abnormal status */
> MIG_TEST_FAIL_DEST_QUIT_ERR,
> };
>
> Because fundamentally the two parameters are correlated, e.g. there is no
> combination of expect_fail==false && dst_quit==true.
>
> No strong opinion, though.
Using enums is more clear to someone reading code, so a good idea.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests
2022-03-02 17:49 ` [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests Daniel P. Berrangé
2022-03-07 7:16 ` Peter Xu
@ 2022-03-07 10:11 ` Thomas Huth
2022-03-10 11:00 ` Daniel P. Berrangé
1 sibling, 1 reply; 48+ messages in thread
From: Thomas Huth @ 2022-03-07 10:11 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Laurent Vivier, Paolo Bonzini, Dr. David Alan Gilbert, Peter Xu,
Juan Quintela
On 02/03/2022 18.49, Daniel P. Berrangé wrote:
> The test cases differ only in the URI they provide to the migration
> commands, and the ability to set the dirty_ring mode. This code is
> trivially merged into a common helper.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> roms/seabios | 2 +-
> tests/qtest/migration-test.c | 86 ++++++++++++++++--------------------
> 2 files changed, 40 insertions(+), 48 deletions(-)
>
> diff --git a/roms/seabios b/roms/seabios
> index 6a62e0cb0d..2dd4b9b3f8 160000
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit 6a62e0cb0dfe9cd28b70547dbea5caf76847c3a9
> +Subproject commit 2dd4b9b3f84019668719344b40dba79d681be41c
Did you really want to update the submodule here?
Thomas
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 12/18] tests: add migration tests of TLS with PSK credentials
2022-03-02 17:49 ` [PATCH 12/18] tests: add migration tests of TLS with PSK credentials Daniel P. Berrangé
@ 2022-03-07 10:12 ` Thomas Huth
0 siblings, 0 replies; 48+ messages in thread
From: Thomas Huth @ 2022-03-07 10:12 UTC (permalink / raw)
To: Daniel P. Berrangé, qemu-devel
Cc: Laurent Vivier, Paolo Bonzini, Dr. David Alan Gilbert, Peter Xu,
Juan Quintela
On 02/03/2022 18.49, Daniel P. Berrangé wrote:
> This validates that we correctly handle migration success and failure
> scenarios when using TLS with pre shared keys.
>
> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> ---
> roms/seabios | 2 +-
> tests/qtest/meson.build | 7 +-
> tests/qtest/migration-test.c | 180 ++++++++++++++++++++++++++--
> tests/unit/crypto-tls-psk-helpers.c | 18 ++-
> tests/unit/crypto-tls-psk-helpers.h | 1 +
> 5 files changed, 190 insertions(+), 18 deletions(-)
>
> diff --git a/roms/seabios b/roms/seabios
> index 2dd4b9b3f8..6a62e0cb0d 160000
> --- a/roms/seabios
> +++ b/roms/seabios
> @@ -1 +1 @@
> -Subproject commit 2dd4b9b3f84019668719344b40dba79d681be41c
> +Subproject commit 6a62e0cb0dfe9cd28b70547dbea5caf76847c3a9
Ah, here is the revert - so the previous change was by accident, indeed!
Thomas
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-07 10:09 ` Thomas Huth
@ 2022-03-07 10:20 ` Peter Xu
2022-03-10 10:55 ` Daniel P. Berrangé
1 sibling, 0 replies; 48+ messages in thread
From: Peter Xu @ 2022-03-07 10:20 UTC (permalink / raw)
To: Thomas Huth
Cc: Laurent Vivier, Daniel P. Berrangé,
Juan Quintela, qemu-devel, Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > > > The QMP commands have a trailing newline, but the response does not.
> > > > This makes the qtest logs hard to follow as the next QMP command
> > > > appears in the same line as the previous QMP response.
> > > >
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > > tests/qtest/libqtest.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > index a85f8a6d05..79c3edcf4b 100644
> > > > --- a/tests/qtest/libqtest.c
> > > > +++ b/tests/qtest/libqtest.c
> > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > }
> > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > }
> > > > + if (log) {
> > > > + g_assert(write(2, "\n", 1) == 1);
> > > > + }
> > >
> > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> >
> > You need to check the return value of write() otherwise you'll get a
> > compile failure due to a warn_unused_result attribute annotation.
> >
> > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > that in our code.
>
> You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
Ah that's better indeed. :)
We could also fix the write() above this one too - that one captures the
retcode into len but it simply dropped it, afaict.
Thanks,
--
Peter Xu
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-07 10:09 ` Thomas Huth
2022-03-07 10:20 ` Peter Xu
@ 2022-03-10 10:55 ` Daniel P. Berrangé
2022-03-10 11:11 ` Marc-André Lureau
1 sibling, 1 reply; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-10 10:55 UTC (permalink / raw)
To: Thomas Huth
Cc: Laurent Vivier, Juan Quintela, qemu-devel, Peter Xu,
Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > > > The QMP commands have a trailing newline, but the response does not.
> > > > This makes the qtest logs hard to follow as the next QMP command
> > > > appears in the same line as the previous QMP response.
> > > >
> > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > ---
> > > > tests/qtest/libqtest.c | 3 +++
> > > > 1 file changed, 3 insertions(+)
> > > >
> > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > index a85f8a6d05..79c3edcf4b 100644
> > > > --- a/tests/qtest/libqtest.c
> > > > +++ b/tests/qtest/libqtest.c
> > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > }
> > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > }
> > > > + if (log) {
> > > > + g_assert(write(2, "\n", 1) == 1);
> > > > + }
> > >
> > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> >
> > You need to check the return value of write() otherwise you'll get a
> > compile failure due to a warn_unused_result attribute annotation.
> >
> > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > that in our code.
>
> You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
I don't think we need to do that, per existing common practice:
$ git grep '\bg_assert(' | wc -l
2912
$ git grep '\bg_assert(' tests | wc -l
2296
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests
2022-03-07 10:11 ` Thomas Huth
@ 2022-03-10 11:00 ` Daniel P. Berrangé
0 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-10 11:00 UTC (permalink / raw)
To: Thomas Huth
Cc: Laurent Vivier, Juan Quintela, qemu-devel, Peter Xu,
Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 11:11:07AM +0100, Thomas Huth wrote:
> On 02/03/2022 18.49, Daniel P. Berrangé wrote:
> > The test cases differ only in the URI they provide to the migration
> > commands, and the ability to set the dirty_ring mode. This code is
> > trivially merged into a common helper.
> >
> > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > ---
> > roms/seabios | 2 +-
> > tests/qtest/migration-test.c | 86 ++++++++++++++++--------------------
> > 2 files changed, 40 insertions(+), 48 deletions(-)
> >
> > diff --git a/roms/seabios b/roms/seabios
> > index 6a62e0cb0d..2dd4b9b3f8 160000
> > --- a/roms/seabios
> > +++ b/roms/seabios
> > @@ -1 +1 @@
> > -Subproject commit 6a62e0cb0dfe9cd28b70547dbea5caf76847c3a9
> > +Subproject commit 2dd4b9b3f84019668719344b40dba79d681be41c
>
> Did you really want to update the submodule here?
Of course not :-(
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 10:55 ` Daniel P. Berrangé
@ 2022-03-10 11:11 ` Marc-André Lureau
2022-03-10 11:35 ` Daniel P. Berrangé
0 siblings, 1 reply; 48+ messages in thread
From: Marc-André Lureau @ 2022-03-10 11:11 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela,
Dr. David Alan Gilbert, Peter Xu, QEMU, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 2250 bytes --]
Hi
On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> > On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > > > > The QMP commands have a trailing newline, but the response does
> not.
> > > > > This makes the qtest logs hard to follow as the next QMP command
> > > > > appears in the same line as the previous QMP response.
> > > > >
> > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > ---
> > > > > tests/qtest/libqtest.c | 3 +++
> > > > > 1 file changed, 3 insertions(+)
> > > > >
> > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > > index a85f8a6d05..79c3edcf4b 100644
> > > > > --- a/tests/qtest/libqtest.c
> > > > > +++ b/tests/qtest/libqtest.c
> > > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > > }
> > > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > > }
> > > > > + if (log) {
> > > > > + g_assert(write(2, "\n", 1) == 1);
> > > > > + }
> > > >
> > > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> > >
> > > You need to check the return value of write() otherwise you'll get a
> > > compile failure due to a warn_unused_result attribute annotation.
> > >
> > > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > > that in our code.
> >
> > You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
>
> I don't think we need to do that, per existing common practice:
>
> $ git grep '\bg_assert(' | wc -l
> 2912
>
> $ git grep '\bg_assert(' tests | wc -l
> 2296
>
>
On the topic of assert() usage, it would be nice to state clearly when to
assert() or g_assert().
g_assert() behaviour is claimed to be more consistent than assert() across
platforms.
Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or -DG_DISABLE_ASSERT.
I would remove assert.h and prevent from using it back, but I might be
missing some reasons to still use it.
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 3256 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 11:11 ` Marc-André Lureau
@ 2022-03-10 11:35 ` Daniel P. Berrangé
2022-03-10 11:50 ` Marc-André Lureau
` (3 more replies)
0 siblings, 4 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-10 11:35 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laurent Vivier, Thomas Huth, Juan Quintela,
Dr. David Alan Gilbert, Peter Xu, QEMU, Paolo Bonzini
On Thu, Mar 10, 2022 at 03:11:08PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
> > On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> > > On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > > > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > > > > > The QMP commands have a trailing newline, but the response does
> > not.
> > > > > > This makes the qtest logs hard to follow as the next QMP command
> > > > > > appears in the same line as the previous QMP response.
> > > > > >
> > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > ---
> > > > > > tests/qtest/libqtest.c | 3 +++
> > > > > > 1 file changed, 3 insertions(+)
> > > > > >
> > > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > > > index a85f8a6d05..79c3edcf4b 100644
> > > > > > --- a/tests/qtest/libqtest.c
> > > > > > +++ b/tests/qtest/libqtest.c
> > > > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > > > }
> > > > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > > > }
> > > > > > + if (log) {
> > > > > > + g_assert(write(2, "\n", 1) == 1);
> > > > > > + }
> > > > >
> > > > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> > > >
> > > > You need to check the return value of write() otherwise you'll get a
> > > > compile failure due to a warn_unused_result attribute annotation.
> > > >
> > > > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > > > that in our code.
> > >
> > > You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
> >
> > I don't think we need to do that, per existing common practice:
> >
> > $ git grep '\bg_assert(' | wc -l
> > 2912
> >
> > $ git grep '\bg_assert(' tests | wc -l
> > 2296
> >
> >
> On the topic of assert() usage, it would be nice to state clearly when to
> assert() or g_assert().
>
> g_assert() behaviour is claimed to be more consistent than assert() across
> platforms.
>
> Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or -DG_DISABLE_ASSERT.
Personally I would make QEMU build error if NDEBUG or G_DISABLE_ASSERT
was defined, as running with asserts disabled will lead to unsafe code.
We rely on asserts firing to avoid compromise of QEMU when faced with
malicious data.
> I would remove assert.h and prevent from using it back, but I might be
> missing some reasons to still use it.
As a metric we've got massive use of both families of asset
$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://' | sort | uniq -c
17 assert (
5196 assert(
2913 g_assert(
29 g_assert_cmpfloat(
662 g_assert_cmphex(
1745 g_assert_cmpint(
20 g_assert_cmpmem(
407 g_assert_cmpstr(
756 g_assert_cmpuint(
169 g_assert_false(
138 g_assert_nonnull(
22 g_assert_null(
167 g_assert_true(
But for the tests/ subdir, g_assert family is a clear favourite
$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort | uniq -c
1 assert (
759 assert(
2297 g_assert(
29 g_assert_cmpfloat(
661 g_assert_cmphex(
1744 g_assert_cmpint(
20 g_assert_cmpmem(
406 g_assert_cmpstr(
754 g_assert_cmpuint(
169 g_assert_false(
138 g_assert_nonnull(
22 g_assert_null(
167 g_assert_true(
This split doesn't appear to have changed all that much over time.
Going back to v3.0.0 we see similar ballpark figures
$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://' | sort | uniq -c
18 assert (
3766 assert(
2210 g_assert(
23 g_assert_cmpfloat(
310 g_assert_cmphex(
1352 g_assert_cmpint(
11 g_assert_cmpmem(
324 g_assert_cmpstr(
489 g_assert_cmpuint(
88 g_assert_false(
73 g_assert_nonnull(
8 g_assert_null(
46 g_assert_true(
[berrange@catbus qemu]$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort | uniq -c
566 assert(
1876 g_assert(
23 g_assert_cmpfloat(
309 g_assert_cmphex(
1351 g_assert_cmpint(
10 g_assert_cmpmem(
323 g_assert_cmpstr(
488 g_assert_cmpuint(
88 g_assert_false(
73 g_assert_nonnull(
8 g_assert_null(
46 g_assert_true(
Removing either 'assert' or g_assert would be a massive amount of code
churn, for no real functional benefit.
I would personally encourage the more specific g_assert_* variants, to
improve the error messages, but that's really minor.
IMHO we can accept that all of the above are valid to use and worry
about more important things.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 11:35 ` Daniel P. Berrangé
@ 2022-03-10 11:50 ` Marc-André Lureau
2022-03-10 12:02 ` Daniel P. Berrangé
2022-03-10 11:53 ` Marc-André Lureau
` (2 subsequent siblings)
3 siblings, 1 reply; 48+ messages in thread
From: Marc-André Lureau @ 2022-03-10 11:50 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela,
Dr. David Alan Gilbert, Peter Xu, QEMU, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 5706 bytes --]
Hi
On Thu, Mar 10, 2022 at 3:35 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
> On Thu, Mar 10, 2022 at 03:11:08PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> > > > On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > > > > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > > > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé
> wrote:
> > > > > > > The QMP commands have a trailing newline, but the response does
> > > not.
> > > > > > > This makes the qtest logs hard to follow as the next QMP
> command
> > > > > > > appears in the same line as the previous QMP response.
> > > > > > >
> > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > > ---
> > > > > > > tests/qtest/libqtest.c | 3 +++
> > > > > > > 1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > > > > index a85f8a6d05..79c3edcf4b 100644
> > > > > > > --- a/tests/qtest/libqtest.c
> > > > > > > +++ b/tests/qtest/libqtest.c
> > > > > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > > > > }
> > > > > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > > > > }
> > > > > > > + if (log) {
> > > > > > > + g_assert(write(2, "\n", 1) == 1);
> > > > > > > + }
> > > > > >
> > > > > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> > > > >
> > > > > You need to check the return value of write() otherwise you'll get
> a
> > > > > compile failure due to a warn_unused_result attribute annotation.
> > > > >
> > > > > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > > > > that in our code.
> > > >
> > > > You could use g_assert_true() - that's not affected by
> G_DISABLE_ASSERT.
> > >
> > > I don't think we need to do that, per existing common practice:
> > >
> > > $ git grep '\bg_assert(' | wc -l
> > > 2912
> > >
> > > $ git grep '\bg_assert(' tests | wc -l
> > > 2296
> > >
> > >
> > On the topic of assert() usage, it would be nice to state clearly when to
> > assert() or g_assert().
> >
> > g_assert() behaviour is claimed to be more consistent than assert()
> across
> > platforms.
> >
> > Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or
> -DG_DISABLE_ASSERT.
>
> Personally I would make QEMU build error if NDEBUG or G_DISABLE_ASSERT
> was defined, as running with asserts disabled will lead to unsafe code.
> We rely on asserts firing to avoid compromise of QEMU when faced with
> malicious data.
>
agreed
>
> > I would remove assert.h and prevent from using it back, but I might be
> > missing some reasons to still use it.
>
> As a metric we've got massive use of both families of asset
>
> $ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://'
> | sort | uniq -c
> 17 assert (
> 5196 assert(
> 2913 g_assert(
> 29 g_assert_cmpfloat(
> 662 g_assert_cmphex(
> 1745 g_assert_cmpint(
> 20 g_assert_cmpmem(
> 407 g_assert_cmpstr(
> 756 g_assert_cmpuint(
> 169 g_assert_false(
> 138 g_assert_nonnull(
> 22 g_assert_null(
> 167 g_assert_true(
>
> But for the tests/ subdir, g_assert family is a clear favourite
>
> $ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e
> 's/.*://' | sort | uniq -c
> 1 assert (
> 759 assert(
> 2297 g_assert(
> 29 g_assert_cmpfloat(
> 661 g_assert_cmphex(
> 1744 g_assert_cmpint(
> 20 g_assert_cmpmem(
> 406 g_assert_cmpstr(
> 754 g_assert_cmpuint(
> 169 g_assert_false(
> 138 g_assert_nonnull(
> 22 g_assert_null(
> 167 g_assert_true(
>
>
> This split doesn't appear to have changed all that much over time.
> Going back to v3.0.0 we see similar ballpark figures
>
> $ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://'
> | sort | uniq -c
> 18 assert (
> 3766 assert(
> 2210 g_assert(
> 23 g_assert_cmpfloat(
> 310 g_assert_cmphex(
> 1352 g_assert_cmpint(
> 11 g_assert_cmpmem(
> 324 g_assert_cmpstr(
> 489 g_assert_cmpuint(
> 88 g_assert_false(
> 73 g_assert_nonnull(
> 8 g_assert_null(
> 46 g_assert_true(
> [berrange@catbus qemu]$ git grep -E -o
> '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort |
> uniq -c
> 566 assert(
> 1876 g_assert(
> 23 g_assert_cmpfloat(
> 309 g_assert_cmphex(
> 1351 g_assert_cmpint(
> 10 g_assert_cmpmem(
> 323 g_assert_cmpstr(
> 488 g_assert_cmpuint(
> 88 g_assert_false(
> 73 g_assert_nonnull(
> 8 g_assert_null(
> 46 g_assert_true(
>
>
> Removing either 'assert' or g_assert would be a massive amount of code
> churn, for no real functional benefit.
>
> I would personally encourage the more specific g_assert_* variants, to
> improve the error messages, but that's really minor.
>
> IMHO we can accept that all of the above are valid to use and worry
> about more important things.
>
Beside the usage inconsistency & the potential platform inconsistencies, it
makes code reorganization/split (libslirp-like) a bit more annoying,
because <assert.h> is only included in osdep.h.
Also g_assert*() family of functions help more precise usage patterns.
g_assert_not_reached() is better than assert(false) or just abort() imho
(it seems we use both).
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 7614 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 11:35 ` Daniel P. Berrangé
2022-03-10 11:50 ` Marc-André Lureau
@ 2022-03-10 11:53 ` Marc-André Lureau
2022-03-10 12:08 ` Thomas Huth
2022-03-10 13:35 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 48+ messages in thread
From: Marc-André Lureau @ 2022-03-10 11:53 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela,
Dr. David Alan Gilbert, Peter Xu, QEMU, Paolo Bonzini
[-- Attachment #1: Type: text/plain, Size: 414 bytes --]
Hi
On Thu, Mar 10, 2022 at 3:35 PM Daniel P. Berrangé <berrange@redhat.com>
wrote:
>
> Removing either 'assert' or g_assert would be a massive amount of code
> churn, for no real functional benefit.
>
>
Well, a few thousands of lines that are trivially regexp. And we can make
use of git blame ignore-rev (https://michaelheap.com/git-ignore-rev/) to
skip those cleanups.
--
Marc-André Lureau
[-- Attachment #2: Type: text/html, Size: 857 bytes --]
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 11:50 ` Marc-André Lureau
@ 2022-03-10 12:02 ` Daniel P. Berrangé
0 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-10 12:02 UTC (permalink / raw)
To: Marc-André Lureau
Cc: Laurent Vivier, Thomas Huth, Juan Quintela,
Dr. David Alan Gilbert, Peter Xu, QEMU, Paolo Bonzini
On Thu, Mar 10, 2022 at 03:50:58PM +0400, Marc-André Lureau wrote:
> Hi
>
> On Thu, Mar 10, 2022 at 3:35 PM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
>
> > On Thu, Mar 10, 2022 at 03:11:08PM +0400, Marc-André Lureau wrote:
> > > Hi
> > >
> > > On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
> > > wrote:
> > >
> > > > On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> > > > > On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > > > > > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > > > > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé
> > wrote:
> > > > > > > > The QMP commands have a trailing newline, but the response does
> > > > not.
> > > > > > > > This makes the qtest logs hard to follow as the next QMP
> > command
> > > > > > > > appears in the same line as the previous QMP response.
> > > > > > > >
> > > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > > > ---
> > > > > > > > tests/qtest/libqtest.c | 3 +++
> > > > > > > > 1 file changed, 3 insertions(+)
> > > > > > > >
> > > > > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > > > > > index a85f8a6d05..79c3edcf4b 100644
> > > > > > > > --- a/tests/qtest/libqtest.c
> > > > > > > > +++ b/tests/qtest/libqtest.c
> > > > > > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > > > > > }
> > > > > > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > > > > > }
> > > > > > > > + if (log) {
> > > > > > > > + g_assert(write(2, "\n", 1) == 1);
> > > > > > > > + }
> > > > > > >
> > > > > > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> > > > > >
> > > > > > You need to check the return value of write() otherwise you'll get
> > a
> > > > > > compile failure due to a warn_unused_result attribute annotation.
> > > > > >
> > > > > > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > > > > > that in our code.
> > > > >
> > > > > You could use g_assert_true() - that's not affected by
> > G_DISABLE_ASSERT.
> > > >
> > > > I don't think we need to do that, per existing common practice:
> > > >
> > > > $ git grep '\bg_assert(' | wc -l
> > > > 2912
> > > >
> > > > $ git grep '\bg_assert(' tests | wc -l
> > > > 2296
> > > >
> > > >
> > > On the topic of assert() usage, it would be nice to state clearly when to
> > > assert() or g_assert().
> > >
> > > g_assert() behaviour is claimed to be more consistent than assert()
> > across
> > > platforms.
> > >
> > > Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or
> > -DG_DISABLE_ASSERT.
> >
> > Personally I would make QEMU build error if NDEBUG or G_DISABLE_ASSERT
> > was defined, as running with asserts disabled will lead to unsafe code.
> > We rely on asserts firing to avoid compromise of QEMU when faced with
> > malicious data.
> >
>
> agreed
Such a good idea that it was already done 5 years ago :-)
commit 262a69f4282e44426c7a132138581d400053e0a1
Author: Eric Blake <eblake@redhat.com>
Date: Mon Sep 11 16:13:20 2017 -0500
osdep.h: Prohibit disabling assert() in supported builds
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 11:35 ` Daniel P. Berrangé
2022-03-10 11:50 ` Marc-André Lureau
2022-03-10 11:53 ` Marc-André Lureau
@ 2022-03-10 12:08 ` Thomas Huth
2022-03-10 13:35 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 48+ messages in thread
From: Thomas Huth @ 2022-03-10 12:08 UTC (permalink / raw)
To: Daniel P. Berrangé, Marc-André Lureau
Cc: Laurent Vivier, Juan Quintela, Dr. David Alan Gilbert, Peter Xu,
QEMU, Paolo Bonzini
On 10/03/2022 12.35, Daniel P. Berrangé wrote:
> On Thu, Mar 10, 2022 at 03:11:08PM +0400, Marc-André Lureau wrote:
>> Hi
>>
>> On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
>> wrote:
>>
>>> On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
>>>> On 07/03/2022 11.06, Daniel P. Berrangé wrote:
>>>>> On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
>>>>>> On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
>>>>>>> The QMP commands have a trailing newline, but the response does
>>> not.
>>>>>>> This makes the qtest logs hard to follow as the next QMP command
>>>>>>> appears in the same line as the previous QMP response.
>>>>>>>
>>>>>>> Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
>>>>>>> ---
>>>>>>> tests/qtest/libqtest.c | 3 +++
>>>>>>> 1 file changed, 3 insertions(+)
>>>>>>>
>>>>>>> diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
>>>>>>> index a85f8a6d05..79c3edcf4b 100644
>>>>>>> --- a/tests/qtest/libqtest.c
>>>>>>> +++ b/tests/qtest/libqtest.c
>>>>>>> @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
>>>>>>> }
>>>>>>> json_message_parser_feed(&qmp.parser, &c, 1);
>>>>>>> }
>>>>>>> + if (log) {
>>>>>>> + g_assert(write(2, "\n", 1) == 1);
>>>>>>> + }
>>>>>>
>>>>>> Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
>>>>>
>>>>> You need to check the return value of write() otherwise you'll get a
>>>>> compile failure due to a warn_unused_result attribute annotation.
>>>>>
>>>>> I don't think G_DISABLE_ASSERT is a problem as we're not defining
>>>>> that in our code.
>>>>
>>>> You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
>>>
>>> I don't think we need to do that, per existing common practice:
>>>
>>> $ git grep '\bg_assert(' | wc -l
>>> 2912
>>>
>>> $ git grep '\bg_assert(' tests | wc -l
>>> 2296
I said, you *could* do that, not that you *must* do that ;-)
Since we require compiling without G_DISABLE_ASSERT in the QEMU code base,
it doesn't really matter for us. But if you're involved in other projects,
too, where GI_DISABLE_ASSERT might be allowed, it might be good habit to do
it "right", i.e. use g_assert_true() in case the expression has side effects
and must never be disabled.
>>>
>> On the topic of assert() usage, it would be nice to state clearly when to
>> assert() or g_assert().
>>
>> g_assert() behaviour is claimed to be more consistent than assert() across
>> platforms.
>>
>> Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or -DG_DISABLE_ASSERT.
>
> Personally I would make QEMU build error if NDEBUG or G_DISABLE_ASSERT
> was defined, as running with asserts disabled will lead to unsafe code.
> We rely on asserts firing to avoid compromise of QEMU when faced with
> malicious data.
We already do that, see osdep.h:
#ifdef NDEBUG
#error building with NDEBUG is not supported
#endif
#ifdef G_DISABLE_ASSERT
#error building with G_DISABLE_ASSERT is not supported
#endif
>> I would remove assert.h and prevent from using it back, but I might be
>> missing some reasons to still use it.
I guess it's just grown historically.
First step would be to add an entry to our coding styles, I think.
Then, if we don't care about the code churn, this would be a nice
BiteSizeTask for new contributors, I think (GSoC etc.), so we could create
some tickets in the Gitlab issue tracker for this (e.g. split up by subsystem).
Thomas
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 04/18] tests: print newline after QMP response in qtest logs
2022-03-10 11:35 ` Daniel P. Berrangé
` (2 preceding siblings ...)
2022-03-10 12:08 ` Thomas Huth
@ 2022-03-10 13:35 ` Dr. David Alan Gilbert
3 siblings, 0 replies; 48+ messages in thread
From: Dr. David Alan Gilbert @ 2022-03-10 13:35 UTC (permalink / raw)
To: Daniel P. Berrangé
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, QEMU, Peter Xu,
Marc-André Lureau, Paolo Bonzini
* Daniel P. Berrangé (berrange@redhat.com) wrote:
> On Thu, Mar 10, 2022 at 03:11:08PM +0400, Marc-André Lureau wrote:
> > Hi
> >
> > On Thu, Mar 10, 2022 at 2:56 PM Daniel P. Berrangé <berrange@redhat.com>
> > wrote:
> >
> > > On Mon, Mar 07, 2022 at 11:09:37AM +0100, Thomas Huth wrote:
> > > > On 07/03/2022 11.06, Daniel P. Berrangé wrote:
> > > > > On Mon, Mar 07, 2022 at 02:51:23PM +0800, Peter Xu wrote:
> > > > > > On Wed, Mar 02, 2022 at 05:49:18PM +0000, Daniel P. Berrangé wrote:
> > > > > > > The QMP commands have a trailing newline, but the response does
> > > not.
> > > > > > > This makes the qtest logs hard to follow as the next QMP command
> > > > > > > appears in the same line as the previous QMP response.
> > > > > > >
> > > > > > > Signed-off-by: Daniel P. Berrangé <berrange@redhat.com>
> > > > > > > ---
> > > > > > > tests/qtest/libqtest.c | 3 +++
> > > > > > > 1 file changed, 3 insertions(+)
> > > > > > >
> > > > > > > diff --git a/tests/qtest/libqtest.c b/tests/qtest/libqtest.c
> > > > > > > index a85f8a6d05..79c3edcf4b 100644
> > > > > > > --- a/tests/qtest/libqtest.c
> > > > > > > +++ b/tests/qtest/libqtest.c
> > > > > > > @@ -629,6 +629,9 @@ QDict *qmp_fd_receive(int fd)
> > > > > > > }
> > > > > > > json_message_parser_feed(&qmp.parser, &c, 1);
> > > > > > > }
> > > > > > > + if (log) {
> > > > > > > + g_assert(write(2, "\n", 1) == 1);
> > > > > > > + }
> > > > > >
> > > > > > Drop the g_assert() to remove side effect of G_DISABLE_ASSERT?
> > > > >
> > > > > You need to check the return value of write() otherwise you'll get a
> > > > > compile failure due to a warn_unused_result attribute annotation.
> > > > >
> > > > > I don't think G_DISABLE_ASSERT is a problem as we're not defining
> > > > > that in our code.
> > > >
> > > > You could use g_assert_true() - that's not affected by G_DISABLE_ASSERT.
> > >
> > > I don't think we need to do that, per existing common practice:
> > >
> > > $ git grep '\bg_assert(' | wc -l
> > > 2912
> > >
> > > $ git grep '\bg_assert(' tests | wc -l
> > > 2296
> > >
> > >
> > On the topic of assert() usage, it would be nice to state clearly when to
> > assert() or g_assert().
> >
> > g_assert() behaviour is claimed to be more consistent than assert() across
> > platforms.
> >
> > Also -DNDEBUG is less obvious than -DG_DISABLE_CHECKS or -DG_DISABLE_ASSERT.
>
> Personally I would make QEMU build error if NDEBUG or G_DISABLE_ASSERT
> was defined, as running with asserts disabled will lead to unsafe code.
> We rely on asserts firing to avoid compromise of QEMU when faced with
> malicious data.
That's not enough; glib allows many of the asserts to be made non-fatal
by a runtime flag, by calling g_test_set_nonfatal_assertions
(see glib commit a6a875068779) - I made checkpatch disallow the use of
these g_assert's in the main code in commit 6e93895
Dave
> > I would remove assert.h and prevent from using it back, but I might be
> > missing some reasons to still use it.
>
> As a metric we've got massive use of both families of asset
>
> $ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://' | sort | uniq -c
> 17 assert (
> 5196 assert(
> 2913 g_assert(
> 29 g_assert_cmpfloat(
> 662 g_assert_cmphex(
> 1745 g_assert_cmpint(
> 20 g_assert_cmpmem(
> 407 g_assert_cmpstr(
> 756 g_assert_cmpuint(
> 169 g_assert_false(
> 138 g_assert_nonnull(
> 22 g_assert_null(
> 167 g_assert_true(
>
> But for the tests/ subdir, g_assert family is a clear favourite
>
> $ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort | uniq -c
> 1 assert (
> 759 assert(
> 2297 g_assert(
> 29 g_assert_cmpfloat(
> 661 g_assert_cmphex(
> 1744 g_assert_cmpint(
> 20 g_assert_cmpmem(
> 406 g_assert_cmpstr(
> 754 g_assert_cmpuint(
> 169 g_assert_false(
> 138 g_assert_nonnull(
> 22 g_assert_null(
> 167 g_assert_true(
>
>
> This split doesn't appear to have changed all that much over time.
> Going back to v3.0.0 we see similar ballpark figures
>
> $ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' | sed -e 's/.*://' | sort | uniq -c
> 18 assert (
> 3766 assert(
> 2210 g_assert(
> 23 g_assert_cmpfloat(
> 310 g_assert_cmphex(
> 1352 g_assert_cmpint(
> 11 g_assert_cmpmem(
> 324 g_assert_cmpstr(
> 489 g_assert_cmpuint(
> 88 g_assert_false(
> 73 g_assert_nonnull(
> 8 g_assert_null(
> 46 g_assert_true(
> [berrange@catbus qemu]$ git grep -E -o '\b(assert|g_assert(_[a-z]+)?)\b\s?\(' tests | sed -e 's/.*://' | sort | uniq -c
> 566 assert(
> 1876 g_assert(
> 23 g_assert_cmpfloat(
> 309 g_assert_cmphex(
> 1351 g_assert_cmpint(
> 10 g_assert_cmpmem(
> 323 g_assert_cmpstr(
> 488 g_assert_cmpuint(
> 88 g_assert_false(
> 73 g_assert_nonnull(
> 8 g_assert_null(
> 46 g_assert_true(
>
>
> Removing either 'assert' or g_assert would be a massive amount of code
> churn, for no real functional benefit.
>
> I would personally encourage the more specific g_assert_* variants, to
> improve the error messages, but that's really minor.
>
> IMHO we can accept that all of the above are valid to use and worry
> about more important things.
>
> With regards,
> Daniel
> --
> |: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
> |: https://libvirt.org -o- https://fstop138.berrange.com :|
> |: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
>
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
^ permalink raw reply [flat|nested] 48+ messages in thread
* Re: [PATCH 11/18] tests: expand the migration precopy helper to support failures
2022-03-07 7:57 ` Peter Xu
@ 2022-03-10 16:18 ` Daniel P. Berrangé
0 siblings, 0 replies; 48+ messages in thread
From: Daniel P. Berrangé @ 2022-03-10 16:18 UTC (permalink / raw)
To: Peter Xu
Cc: Laurent Vivier, Thomas Huth, Juan Quintela, qemu-devel,
Dr. David Alan Gilbert, Paolo Bonzini
On Mon, Mar 07, 2022 at 03:57:16PM +0800, Peter Xu wrote:
> On Wed, Mar 02, 2022 at 05:49:25PM +0000, Daniel P. Berrangé wrote:
> > static void test_precopy_common(const char *listen_uri,
> > const char *connect_uri,
> > TestMigrateStartHook start_hook,
> > TestMigrateFinishHook finish_hook,
> > + bool expect_fail,
> > + bool dst_quit,
> > bool dirty_ring)
> > {
> > MigrateStart *args = migrate_start_new();
> > @@ -875,24 +890,32 @@ static void test_precopy_common(const char *listen_uri,
> >
> > migrate_qmp(from, connect_uri, "{}");
> >
> > - wait_for_migration_pass(from);
> > + if (expect_fail) {
> > + wait_for_migration_fail(from, !dst_quit);
>
> Two more thoughts..
>
> (1) Shall we move MigrateStart creation to be even upper? Then we avoid
> passing over these parameters but merge these new parameters into
> MigrateStart too. After all we used to have similar long lists of
> params and we merged them into MigrateStart.
I don't to use MigrateStart as these new parameters are not common
to all migration tests. I have come up with an equivalent approach
though.
> (2) Shall we leverage MigrateStart.hide_stderr? I saw a bunch of errors
> dumped even if all things run as expected.
Yes.
Regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
^ permalink raw reply [flat|nested] 48+ messages in thread
end of thread, other threads:[~2022-03-10 16:21 UTC | newest]
Thread overview: 48+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-02 17:49 [PATCH 00/18] tests: introduce testing coverage for TLS with migration Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 01/18] tests: fix encoding of IP addresses in x509 certs Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 02/18] tests: improve error message when saving TLS PSK file fails Daniel P. Berrangé
2022-03-07 6:52 ` Peter Xu
2022-03-02 17:49 ` [PATCH 03/18] tests: support QTEST_TRACE env variable Daniel P. Berrangé
2022-03-07 6:53 ` Peter Xu
2022-03-07 10:06 ` Thomas Huth
2022-03-02 17:49 ` [PATCH 04/18] tests: print newline after QMP response in qtest logs Daniel P. Berrangé
2022-03-07 6:51 ` Peter Xu
2022-03-07 10:06 ` Daniel P. Berrangé
2022-03-07 10:09 ` Thomas Huth
2022-03-07 10:20 ` Peter Xu
2022-03-10 10:55 ` Daniel P. Berrangé
2022-03-10 11:11 ` Marc-André Lureau
2022-03-10 11:35 ` Daniel P. Berrangé
2022-03-10 11:50 ` Marc-André Lureau
2022-03-10 12:02 ` Daniel P. Berrangé
2022-03-10 11:53 ` Marc-André Lureau
2022-03-10 12:08 ` Thomas Huth
2022-03-10 13:35 ` Dr. David Alan Gilbert
2022-03-02 17:49 ` [PATCH 05/18] tests: add more helper macros for creating TLS x509 certs Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 06/18] crypto: mandate a hostname when checking x509 creds on a client Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 07/18] migration: fix use of TLS PSK credentials with a UNIX socket Daniel P. Berrangé
2022-03-07 7:08 ` Peter Xu
2022-03-07 10:08 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 08/18] tests: merge code for UNIX and TCP migration pre-copy tests Daniel P. Berrangé
2022-03-07 7:16 ` Peter Xu
2022-03-07 10:11 ` Thomas Huth
2022-03-10 11:00 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 09/18] tests: introduce ability to provide hooks for migration precopy test Daniel P. Berrangé
2022-03-07 7:19 ` Peter Xu
2022-03-02 17:49 ` [PATCH 10/18] tests: switch migration FD passing test to use common precopy helper Daniel P. Berrangé
2022-03-07 7:22 ` Peter Xu
2022-03-02 17:49 ` [PATCH 11/18] tests: expand the migration precopy helper to support failures Daniel P. Berrangé
2022-03-07 7:39 ` Peter Xu
2022-03-07 10:10 ` Daniel P. Berrangé
2022-03-07 7:57 ` Peter Xu
2022-03-10 16:18 ` Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 12/18] tests: add migration tests of TLS with PSK credentials Daniel P. Berrangé
2022-03-07 10:12 ` Thomas Huth
2022-03-02 17:49 ` [PATCH 13/18] tests: add migration tests of TLS with x509 credentials Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 14/18] tests: convert XBZRLE migration test to use common helper Daniel P. Berrangé
2022-03-07 8:01 ` Peter Xu
2022-03-02 17:49 ` [PATCH 15/18] tests: convert multifd migration tests " Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 16/18] tests: add multifd migration tests of TLS with PSK credentials Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 17/18] tests: add multifd migration tests of TLS with x509 credentials Daniel P. Berrangé
2022-03-02 17:49 ` [PATCH 18/18] tests: ensure migration status isn't reported as failed Daniel P. Berrangé
2022-03-07 8:09 ` Peter Xu
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.