Re: More doc updates needed for new migrate argument @channels
On 06/05/24 9:10 pm, Peter Xu wrote: !---| CAUTION: External Email |---! On Mon, May 06, 2024 at 06:58:00AM +0200, Markus Armbruster wrote: Peter Xu writes: [...] I also copied qemu-devel starting from now. My bad, I messed that up! Het, do you want to send a patch? Thanks, Hi Fabiano, Peter. Sorry for joining late to this discussion. Was out of touch for quite sometime from the qemu upstream community. Will definitely try to send a patch for the 'channels' documentation changes soon in Qemu, as discussed above. Regards, Het Gala
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On 11/04/24 7:56 pm, Peter Xu wrote: !---| CAUTION: External Email |---! On Thu, Apr 11, 2024 at 07:45:21PM +0530, Het Gala wrote: On 10/04/24 8:23 pm, Peter Xu wrote: !---| CAUTION: External Email |---! On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote: Het Gala writes: This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb After addition of 'channels' as the starting argument of new QAPI syntax inside postcopy test, even if the user entered the old QAPI syntax, test used the new syntax. It was a temporary patch added to have some presence of the new syntax since the migration qtest framework lacked any logic for introducing 'channels' argument. That wasn't clear to me when we merged that. Was that really the case? Yeah these look all a bit confusing.. I'm wondering whether do we need the new interface to cover both precopy and postcopy, or one would suffice? Both should share the same interface. I think it means if we covered the channels interface in precopy, then perhaps we don't need to test anywhere else, as we got the code paths all covered. We actually do the same already for all kinds of channels for postcopy, where we stick with either tcp/unix but don't cover the rest. Do we want to add other transports too (vsock, exec, rdma) with the new interface ? I believe we have tests for fd based migration Het, What I meant is we used to do white box testing for migration, trying to cover all the code paths would suffice for us in that case. It means maybe we don't need the postcopy test to cover the channels interface as long as precopy has covered that and also if that covered all the "channels" abi then we should be safe. What I worry is we keep extending the test matrix but we're actually testing the same code paths. Then the test runs slower each time, we burn more cpus for each CI kick, but without a real beneift. Yeah, I understood your concern Peter. Yes, since precopy and postcopy tests cover the same code path, adding 'channels' postcopy qtests does not make much sense after already having introduced in precopy qtests. I just wanted to highlight couple of pointers: 1. though we are using 'channels' in the precopy tests for 'migrate' QAPI, we use the old uri for 'migrate-incoming' QAPI. 2. We do not cover other 'channels' abi, only have tcp path tested. So, the TO-DOs could be: 1. Omit the 4th patch here, which introduced postcopy qtests with 'channels' interface OR have 'channels' interface with other than tcp transport (file, exec, vsock, etc) so as to cover different code paths. 2. Extend channels interface to migrate-incoming QAPI for precopy qtests Regards, Het Gala
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On 10/04/24 8:23 pm, Peter Xu wrote: !---| CAUTION: External Email |---! On Wed, Apr 10, 2024 at 10:04:33AM -0300, Fabiano Rosas wrote: Het Gala writes: This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb After addition of 'channels' as the starting argument of new QAPI syntax inside postcopy test, even if the user entered the old QAPI syntax, test used the new syntax. It was a temporary patch added to have some presence of the new syntax since the migration qtest framework lacked any logic for introducing 'channels' argument. That wasn't clear to me when we merged that. Was that really the case? Yeah these look all a bit confusing.. I'm wondering whether do we need the new interface to cover both precopy and postcopy, or one would suffice? Both should share the same interface. I think it means if we covered the channels interface in precopy, then perhaps we don't need to test anywhere else, as we got the code paths all covered. We actually do the same already for all kinds of channels for postcopy, where we stick with either tcp/unix but don't cover the rest. Do we want to add other transports too (vsock, exec, rdma) with the new interface ? I believe we have tests for fd based migration Regards, Het Gala
Re: [PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri
On 10/04/24 6:45 pm, Fabiano Rosas wrote:
!---|
CAUTION: External Email
|---!
Het Gala writes:
Add qtests to perform postcopy live migration by having list of
'channels' argument as the starting point instead of uri string.
(Note: length of the list is restricted to 1 for now)
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 38 ++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index fa8a860811..599018baa0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState
**from_ptr,
migrate_ensure_non_converge(from);
migrate_prepare_for_dirty_mem(from);
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+if (args->connect_channels) {
+migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
+} else {
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+}
From an interface perspective it's a bit unexpected to need this
conditional when the migrate_qmp below doesn't need it.
I think, migrate_qmp has an unique advantage here. Please correct me If
my understanding
is not correct.
1. test_migrate_start starts the qemu cmd line with either --incoming
tcp:127.0.0.1:0
or "defer". If tcp uri is provided, then migrate_incoming_qmp is not
necessary
2. If "defer" is passed, then only migrate_incoming_qmp is required with
tcp uri string
3. migrate_qmp can get the uri anyway either from
test_migrate_start -> defer + migrate_incoming_qmp ->
tcp:127.0.0.1:0
test_migrate_start -> tcp:127.0.0.1:0
with the help of migrate_get_connect_uri() with the correct
migration port.
Even if we always pass NULL, we are okay, But this is just for tcp
transport, and
not unix
We can't have the unique situation for migrate_incoming_qmp, hence
avoiding conditional
earlier seemed to be necessary for me. But, with the hack suggested by
you in previous
patch, will prevent us from using conditional if else
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, NULL, "{}");
+migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1355,6 +1359,20 @@ static void test_postcopy(void)
test_postcopy_common();
}
+static void test_postcopy_channels(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+test_postcopy_common();
+}
+
static void test_postcopy_suspend(void)
{
MigrateCommon args = {
@@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
test_postcopy_recovery_common();
}
+static void test_postcopy_recovery_channels(void)
+{
+MigrateCommon args = {
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+test_postcopy_recovery_common();
+}
static void test_postcopy_recovery_compress(void)
{
MigrateCommon args = {
@@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
if (has_uffd) {
migration_test_add("/migration/postcopy/plain", test_postcopy);
+migration_test_add("/migration/postcopy/channels/plain",
+ test_postcopy_channels);
migration_test_add("/migration/postcopy/recovery/plain",
test_postcopy_recovery);
+migration_test_add("/migration/postcopy/recovery/channels/plain",
+ test_postcopy_recovery_channels);
migration_test_add("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
migration_test_add("/migration/postcopy/preempt/recovery/plain",
Regards,
Het Gala
Re: [PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp
On 10/04/24 6:44 pm, Fabiano Rosas wrote:
!---|
CAUTION: External Email
|---!
Het Gala writes:
Alter migrate_incoming_qmp() to allow both uri and channels
independently. For channels, convert string to a QDict.
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 13 +++--
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c | 12 ++--
tests/qtest/virtio-net-failover.c | 8
4 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3b72cad6c1..cbd91719ae 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -245,7 +245,8 @@ void migrate_set_capability(QTestState *who, const char
*capability,
capability, value);
}
-void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt, ...)
+void migrate_incoming_qmp(QTestState *to, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *rsp, *data;
@@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char *uri,
const char *fmt, ...)
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
Do you think it makes sense to have channels take precedence here? It
would make the next patch cleaner without having to check for channels
presence. I don't think we need a 'both' test for incoming.
Yes, this hack would silently solve, avoiding the above check.
IMO, it is good to have like to like QAPIs while running a qtest.
If the qtest uses the new syntax then, both 'migrate' and 'migrate-incoming'
QAPIs should use the new syntax. Even though implementation wise, it makes
no difference (qtests run successfully) but it would be good to ensure,
we have
grammatical correctness.
migrate_set_capability(to, "events", true);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 1339835698..9f74281aea 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
const char *channels, const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
+G_GNUC_PRINTF(4, 5)
void migrate_incoming_qmp(QTestState *who, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(4, 5)
void migrate_qmp_fail(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f2c27d611c..fa8a860811 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
migrate_ensure_non_converge(from);
migrate_prepare_for_dirty_mem(from);
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
@@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
* We need to wait for the source to finish before starting the
* destination.
*/
-migrate_incoming_qmp(to, args->connect_uri, "{}");
+migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
wait_for_migration_complete(to);
if (stop_src) {
@@ -2405,7 +2405,7 @@ static void *test_migrate_fd_start_hook(QTestState *from,
close(pair[0]);
/* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "fd:fd-mig", "{}");
+migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
/* Send the 2nd socket to the target */
qtest_qmp_fds_assert_success(from, [1], 1,
@@ -2715,7 +2715,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState
*from,
migrate_set_capability(to, "multifd", true);
/* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
return NULL;
}
@@ -3040,7 +3040,7 @@ static void test_multifd_tcp_cancel(v
Re: [PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
On 10/04/24 6:34 pm, Fabiano Rosas wrote: !---| CAUTION: External Email |---! Het Gala writes: This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb After addition of 'channels' as the starting argument of new QAPI syntax inside postcopy test, even if the user entered the old QAPI syntax, test used the new syntax. It was a temporary patch added to have some presence of the new syntax since the migration qtest framework lacked any logic for introducing 'channels' argument. That wasn't clear to me when we merged that. Was that really the case? Yes, I had little to no experience on how to introduce 'channels' as a new argument in the migration QAPIs back then. IIRC, while trying to merge the series (migration: Modify 'migrate' and 'migrate-incoming' QAPI commands for migration), I was adviced to just modify one of the qtest with the new QAPI syntax rather than writing a new qtest altogether. So, I just renamed the old syntax with the new one. Now that the qtest framework has logic to introduce uri and channel argument separately, we can remove this temporary change. Signed-off-by: Het Gala Anyway, I can see the point of this. Thanks for the support for building that framework. Today we can independently call channel or uri easily for 'migrate' QAPI Reviewed-by: Fabiano Rosas Regards, Het Gala
Re: [PATCH] tests/qtest: Standardize qtest function caller strings.
On 05/04/24 7:58 pm, Fabiano Rosas wrote: !---| CAUTION: External Email |---! Het Gala writes: On 27/03/24 2:37 am, Fabiano Rosas wrote: Het Gala writes: Some comments, mostly just thinking out loud... For --> migrate // //O:/... For --> validate ///O:/O:/ /O:/O:/... Do we need an optional 'capability' element? I'm not sure how practical is to leave that as 'others', because that puts it at the end of the string. We'd want the element that's more important/with more variants to be towards the start of the string so we can run all tests of the same kind with the -r option. While also looking at different functions for figuring out the transport and invocation, my observation was that, there might be many capabilities added to the same test, while it might not be important also. Ex: /migrate/multifd/tcp/plain 1. multifd is defined as a migration mode. 2. It is also a capability, and comes in 2 parts [multifd, multifd-channels] though one is a capability and another is parameter Similarly in other examples of compression, there are many capabilities and parameters added, but it might be not important to mention that ? Secondly, there are multiple migration capabilities IIRC (> 15). And a test requiring multiple capabilities, the overall string would be too long, and not that important also to mention all capabilities. Just thinking out of mind - Can we have selective list of capabilities ? 1. multifd 2. compress (again, there might be confusion with multifd compression methods like zstd, zlib and just 'compress') 3. zero-page (This will have sub capabilities ?) I was thinking of keeping that part more open-ended. So not specifying capabilities one by one, but more like "if you're testing a capability, it comes here". About multifd, it's a bit special since it cannot be seen as just a "feature" anymore. It's a core part of the migration code. I wouldn't classify it as capability for the purposes of the tests. Ack, got it. test-type:: migrate | validate We could alternatively drop migration|migrate|validate. They are kind of superfluous. I agree with the above comment. 'migrate' and 'validate' have a different set of variables required, some necessary, while other optional. IMO this will help is in streamlining the design further. migration-mode a. migrate --> :: precopy | postcopy | multifd b. validate -->:: (what to validate) methods :: preempt | recovery | reboot | suspend | simple I want some inputs here. 1. is there a better variable name rather than 'methods' Does this fall into the "mode" terminology that Steven introduced? Yes, as we decided that we don't want 'migration-mode' key-value pair, naming 'mode' would be a better term. In cases, where multiple modes are to be used ex: postcopy_preempt_recovery I feel it might be a good idea to separate multiple modes by '-' For example - .../preempty-recovery/... Similarly for other keys too if required 2. 'simple' does not fit perfect here IMO. Can we go without it? You mean omit the key itself in case of a no-op ? transport:: tcp | fd | unix | file invocation :: uri | channels | both CompressionType :: zlib | zstd | none s/none/nocomp/ ? We're already familiar with that. Ack. Will change that. encryptionType :: tls | plain s/plain/notls/ ? What if there is another encryption technique in future ? Or maybe we simply omit the noop options. It would make the string way shorter in most cases. This might be a better approach. Can have some keys/variables as optional while some necessary. For ex: for 'migrate' - transport and invocation might be necessary while it might not be necessary for 'validate' qtests Yep Ack, will do that! validate-test-result :: success | failure others :: other comments/capability that needs to be addressed. Can be multiple (more than one applicable, separated by using '-' in between) O: optional Signed-off-by: Het Gala Suggested-by: Fabiano Rosas --- tests/qtest/migration-test.c | 143 ++- 1 file changed, 72 insertions(+), 71 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index bd9f4b9dbb..bf4d000b76 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c Regards, Het Gala I'm wondering whether we should leave the existing tests untouched and require the new format only for new tests. Going through a git bisection with a change in the middle that alters test names would be infuriating. Hmm yup. I had this doubt on, how would we be enforcing the new design for any new qtests that gets added from now on ? Can we have this design started for validation tests maybe for now, the number
[PATCH 3/4] tests/qtest/migration: Add channels parameter in migrate_incoming_qmp
Alter migrate_incoming_qmp() to allow both uri and channels
independently. For channels, convert string to a QDict.
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 13 +++--
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c | 12 ++--
tests/qtest/virtio-net-failover.c | 8
4 files changed, 23 insertions(+), 14 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3b72cad6c1..cbd91719ae 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -245,7 +245,8 @@ void migrate_set_capability(QTestState *who, const char
*capability,
capability, value);
}
-void migrate_incoming_qmp(QTestState *to, const char *uri, const char *fmt,
...)
+void migrate_incoming_qmp(QTestState *to, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *rsp, *data;
@@ -255,7 +256,15 @@ void migrate_incoming_qmp(QTestState *to, const char *uri,
const char *fmt, ...)
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
migrate_set_capability(to, "events", true);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 1339835698..9f74281aea 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -29,9 +29,9 @@ G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
const char *channels, const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
+G_GNUC_PRINTF(4, 5)
void migrate_incoming_qmp(QTestState *who, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(4, 5)
void migrate_qmp_fail(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f2c27d611c..fa8a860811 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,7 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
migrate_ensure_non_converge(from);
migrate_prepare_for_dirty_mem(from);
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
@@ -1824,7 +1824,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
* We need to wait for the source to finish before starting the
* destination.
*/
-migrate_incoming_qmp(to, args->connect_uri, "{}");
+migrate_incoming_qmp(to, args->connect_uri, NULL, "{}");
wait_for_migration_complete(to);
if (stop_src) {
@@ -2405,7 +2405,7 @@ static void *test_migrate_fd_start_hook(QTestState *from,
close(pair[0]);
/* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "fd:fd-mig", "{}");
+migrate_incoming_qmp(to, "fd:fd-mig", NULL, "{}");
/* Send the 2nd socket to the target */
qtest_qmp_fds_assert_success(from, [1], 1,
@@ -2715,7 +2715,7 @@ test_migrate_precopy_tcp_multifd_start_common(QTestState
*from,
migrate_set_capability(to, "multifd", true);
/* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
return NULL;
}
@@ -3040,7 +3040,7 @@ static void test_multifd_tcp_cancel(void)
migrate_set_capability(to, "multifd", true);
/* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
@@ -3068,7 +3068,7 @@ static void test_multifd_tcp_cancel(void)
migrate_set_capability(to2, "multifd", true);
/* Start incoming migration from the 1st socket */
-migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", "{}");
+migrate_incoming_qmp(to2, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_status(from, "cancelled", NULL);
diff --git a/tests/qtest/virtio-net-failover.c
b/tests/qtest/virtio-net-fail
[PATCH 0/4] tests/qtest/migration: Add postcopy qtests for introducing 'channels' argument with new QAPI syntax
Add postcopy migration qtests with new QAPI syntax, having 'channels' as the starting argument. Also, introduce 'channels' to migrate_incoming_qmp function so as to call migration with the new QAPI syntax from src as well as dest. Patch 1: Revert back commit which temporarily introduced 'migrate-incoming' QAPI with the new 'channels' syntax. Patch 2-3: - Introduce channels arg to migrate_incoming_qmp Patch 4: --- Introduce postcopy qtests with new QAPI syntax Het Gala (4): Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax" tests/qtest/migration: Replace 'migrate-incoming' qtest_qmp_assert_success with migrate_incoming_qmp tests/qtest/migration: Add channels parameter in migrate_incoming_qmp tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri tests/qtest/migration-helpers.c | 13 ++-- tests/qtest/migration-helpers.h | 4 +-- tests/qtest/migration-test.c | 54 +++ tests/qtest/virtio-net-failover.c | 8 ++--- 4 files changed, 58 insertions(+), 21 deletions(-) -- 2.22.3
[PATCH 1/4] Revert "migration: modify test_multifd_tcp_none() to use new QAPI syntax"
This reverts commit 8e3766eefbb4036cbc280c1f1a0d28537929f7fb
After addition of 'channels' as the starting argument of new QAPI
syntax inside postcopy test, even if the user entered the old QAPI
syntax, test used the new syntax.
It was a temporary patch added to have some presence of the new syntax
since the migration qtest framework lacked any logic for introducing
'channels' argument.
Now that the qtest framework has logic to introduce uri and channel
argument separately, we can remove this temporary change.
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 7 +--
1 file changed, 1 insertion(+), 6 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 5d6d8cd634..27a1066ae4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1297,12 +1297,7 @@ static int migrate_postcopy_prepare(QTestState
**from_ptr,
migrate_prepare_for_dirty_mem(from);
qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
- " 'arguments': { "
- " 'channels': [ { 'channel-type': 'main',"
- " 'addr': { 'transport': 'socket',"
- "'type': 'inet',"
- "'host': '127.0.0.1',"
- "'port': '0' } } ] } }");
+ " 'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
--
2.22.3
[PATCH 2/4] tests/qtest/migration: Replace 'migrate-incoming' qtest_qmp_assert_success with migrate_incoming_qmp
Already have a migrate_incoming_qmp helper function to initiate
'migrate-incoming' QMP command with some additional checks.
Replace 'migrate-incoming' qtest_qmp_assert_success command with
calling migrate_incoming_qmp helper function for postcopy qtests.
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 27a1066ae4..f2c27d611c 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,8 +1296,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
migrate_ensure_non_converge(from);
migrate_prepare_for_dirty_mem(from);
-qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
- " 'arguments': { 'uri': 'tcp:127.0.0.1:0' }}");
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", "{}");
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
--
2.22.3
[PATCH 4/4] tests/qtest/migration: Add postcopy migration qtests to use 'channels' argument instead of uri
Add qtests to perform postcopy live migration by having list of
'channels' argument as the starting point instead of uri string.
(Note: length of the list is restricted to 1 for now)
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 38 ++--
1 file changed, 36 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index fa8a860811..599018baa0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1296,13 +1296,17 @@ static int migrate_postcopy_prepare(QTestState
**from_ptr,
migrate_ensure_non_converge(from);
migrate_prepare_for_dirty_mem(from);
-migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+if (args->connect_channels) {
+migrate_incoming_qmp(to, NULL, args->connect_channels, "{}");
+} else {
+migrate_incoming_qmp(to, "tcp:127.0.0.1:0", NULL, "{}");
+}
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, NULL, "{}");
+migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1355,6 +1359,20 @@ static void test_postcopy(void)
test_postcopy_common();
}
+static void test_postcopy_channels(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+test_postcopy_common();
+}
+
static void test_postcopy_suspend(void)
{
MigrateCommon args = {
@@ -1555,6 +1573,18 @@ static void test_postcopy_recovery(void)
test_postcopy_recovery_common();
}
+static void test_postcopy_recovery_channels(void)
+{
+MigrateCommon args = {
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+test_postcopy_recovery_common();
+}
static void test_postcopy_recovery_compress(void)
{
MigrateCommon args = {
@@ -3585,8 +3615,12 @@ int main(int argc, char **argv)
if (has_uffd) {
migration_test_add("/migration/postcopy/plain", test_postcopy);
+migration_test_add("/migration/postcopy/channels/plain",
+ test_postcopy_channels);
migration_test_add("/migration/postcopy/recovery/plain",
test_postcopy_recovery);
+migration_test_add("/migration/postcopy/recovery/channels/plain",
+ test_postcopy_recovery_channels);
migration_test_add("/migration/postcopy/preempt/plain",
test_postcopy_preempt);
migration_test_add("/migration/postcopy/preempt/recovery/plain",
--
2.22.3
[PATCH v2 3/3] fixup! tests/qtest/migration: Add multifd_tcp_plain test using list of channels instead of uri
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 584d7c496f..5d6d8cd634 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1728,7 +1728,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, to, args->connect_uri, NULL, "{}");
+migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
--
2.22.3
[PATCH v2 2/3] fixup! tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..3b72cad6c1 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -131,7 +131,7 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
{
QDict *addr;
QListEntry *entry;
-g_autofree const char *addr_port = NULL;
+const char *addr_port = NULL;
addr = migrate_get_connect_qdict(to);
@@ -143,7 +143,7 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
qdict_haskey(addr, "port") &&
(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
addr_port = qdict_get_str(addr, "port");
-qdict_put_str(addrdict, "port", addr_port);
+qdict_put_str(addrdict, "port", g_strdup(addr_port));
}
}
--
2.22.3
[PATCH v2 0/3] qtest/migration: Fixes around multifd_tcp_channels_none migration qtest
With the introduction of new patchset to have 'channels' as the start argument of migrate QAPIs instead of 'uri' (tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs), a few minor issues got went unnoticed, which were caught while trying to introduce similar qtests in migration-test.c Fix multifd_tcp_channels_none qtest to actually utilize 'channels' arg in migrate QAPIs, fix double freeing of addr Qdict and typos in that patchset. This patchset is built on top of (tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs) Can find the build pipeline at : https://gitlab.com/galahet/Qemu/-/pipelines/1245462266 v1 --> v2: - 1. Split the second patch into different patches - One to deal with double freeing of Qdict and other to add connect_channels inside multifd_tcp_channels_none to actually use 'channels' arg. 2. use 'git commit --fixup' to improve commit message as well as to inform on which commit is the fix meant to be. Het Gala (3): fixup! tests/qtest/migration: Add negative tests to validate migration QAPIs fixup! tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value fixup! tests/qtest/migration: Add multifd_tcp_plain test using list of channels instead of uri tests/qtest/migration-helpers.c | 4 ++-- tests/qtest/migration-test.c| 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) -- 2.22.3
[PATCH v2 1/3] fixup! tests/qtest/migration: Add negative tests to validate migration QAPIs
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d03a655f83..584d7c496f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels,
"{}");
goto finish;
}
--
2.22.3
Re: [PATCH 2/2] Call args->connect_channels to actually test multifd_tcp_channels_none qtest
On 08/04/24 9:10 pm, Peter Xu wrote:
!---|
CAUTION: External Email
|---!
On Sun, Apr 07, 2024 at 01:21:25PM +, Het Gala wrote:
Earlier, without args->connect_channels, multifd_tcp_channels_none would
call uri internally even though connect_channels was introduced in
function definition. To actually call 'migrate' QAPI with modified syntax,
args->connect_channels need to be passed.
Double free happens while setting correct migration ports. Fix that.
Fixes: (tests/qtest/migration: Add multifd_tcp_plain test using list of
channels instead of uri)
[1]
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 2 --
tests/qtest/migration-test.c| 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..b1d06187ab 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -146,8 +146,6 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
qdict_put_str(addrdict, "port", addr_port);
}
}
-
-qobject_unref(addr);
Firstly, this doesn't belong to the commit you were pointing at above [1].
Instead this line is part of:
tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update
migration port value
You may want to split them?
Ack
Side note: I didn't review carefully on the whole patchset, but I think
it's preferred to not include any dead code like what you did with
"tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update
migration port value". It'll be better to me if we introduce code that
will be used already otherwise reviewing such patch is a pain, same to when
we follow up stuff later like this.
Yes Peter. My intention was to have the code which could actually take the
benefit of using 'channels' for the new QAPI syntax. But somehow I missed
adding connect_channels in the code, despite that the test passed because
it generated connect_uri with the help of listen_uri inside migrate_qmp.
And it generated migrate QMP command using old syntax. Also because it never
entered migrate_set_ports, couldn't catch double free issue while manual
testing as well as while the CI/CD pipeline was run.
More importantly.. why free? I'll paste whole thing over, and raise my
questions.
static void migrate_set_ports(QTestState *to, QList *channel_list)
{
QDict *addr;
QListEntry *entry;
g_autofree const char *addr_port = NULL; <- this points to sub-field of
"addr", if we free "addr", why autofree here?
addr = migrate_get_connect_qdict(to);
QLIST_FOREACH_ENTRY(channel_list, entry) {
QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
QDict *addrdict = qdict_get_qdict(channel, "addr");
if (qdict_haskey(addrdict, "port") &&
qdict_haskey(addr, "port") &&
(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
addr_port = qdict_get_str(addr, "port");
qdict_put_str(addrdict, "port", addr_port); <-
shouldn't we g_strdup() instead of dropping the below unref()?
}
}
qobject_unref(addr);
}
Yes, I got your point Peter. Will update in the new patch.
}
bool migrate_watch_for_events(QTestState *who, const char *name,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 584d7c496f..5d6d8cd634 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1728,7 +1728,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, to, args->connect_uri, NULL, "{}");
+migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
--
2.22.3
Regards,
Het Gala
Re: [PATCH 1/2] Fix typo to allow migrate_qmp_fail command with 'channels' argument
On 08/04/24 9:05 pm, Peter Xu wrote: !---| CAUTION: External Email |---! Hey, Het, On Sun, Apr 07, 2024 at 01:21:24PM +, Het Gala wrote: Fixes: (tests/qtest/migration: Add negative tests to validate migration QAPIs) I think I get your intention to provide two fixup patches on top of migration-next, which indeed would be preferred so that I can squash them into the patches before the pull. However please next time use "git commit --fixup" so that a better subject will be generated, and that'll make my life (and Fabiano's I suppose in the future) easier because git rebase understand those subjects. Then you don't need Fixes with an empty commit ID. They'll start with "fixup: XXX" pointing to a commit with subject rather than commit IDs. I apologize for any inconvenience caused by not using "git commit --fixup" in my previous submission. Let me resend the patchset with correct message convention. Will take care of this in future patches too, thanks for bringing it to my notice. Regards, Het Gala
[PATCH 1/2] Fix typo to allow migrate_qmp_fail command with 'channels' argument
Fixes: (tests/qtest/migration: Add negative tests to validate migration QAPIs)
Signed-off-by: Het Gala
---
tests/qtest/migration-test.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d03a655f83..584d7c496f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels,
"{}");
goto finish;
}
--
2.22.3
[PATCH 0/2] Fix: qtest/migration: Improve multifd_tcp_channels_none test
With the introduction of new patchset to have 'channels' as the start argument of migrate QAPIs instead of 'uri' (tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs), a few minor typos got went unnoticed, which were caught while trying to introduce similar qtests in migration. Fix multifd_tcp_channels_none qtest to actually utilise 'channels' arg in migrate QAPIs This patchset is built on top of (tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs) Het Gala (2): Fix typo to allow migrate_qmp_fail command with 'channels' argument Call args->connect_channels to actually test multifd_tcp_channels_none qtest tests/qtest/migration-helpers.c | 2 -- tests/qtest/migration-test.c| 4 ++-- 2 files changed, 2 insertions(+), 4 deletions(-) -- 2.22.3
[PATCH 2/2] Call args->connect_channels to actually test multifd_tcp_channels_none qtest
Earlier, without args->connect_channels, multifd_tcp_channels_none would
call uri internally even though connect_channels was introduced in
function definition. To actually call 'migrate' QAPI with modified syntax,
args->connect_channels need to be passed.
Double free happens while setting correct migration ports. Fix that.
Fixes: (tests/qtest/migration: Add multifd_tcp_plain test using list of
channels instead of uri)
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 2 --
tests/qtest/migration-test.c| 2 +-
2 files changed, 1 insertion(+), 3 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..b1d06187ab 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -146,8 +146,6 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
qdict_put_str(addrdict, "port", addr_port);
}
}
-
-qobject_unref(addr);
}
bool migrate_watch_for_events(QTestState *who, const char *name,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 584d7c496f..5d6d8cd634 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1728,7 +1728,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, to, args->connect_uri, NULL, "{}");
+migrate_qmp(from, to, args->connect_uri, args->connect_channels, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
--
2.22.3
Re: [PATCH] tests/qtest: Standardize qtest function caller strings.
ping ! On 27/03/24 4:18 pm, Het Gala wrote: On 27/03/24 2:37 am, Fabiano Rosas wrote: Het Gala writes: Some comments, mostly just thinking out loud... For --> migrate // //O:/... For --> validate ///O:/O:/ /O:/O:/... Do we need an optional 'capability' element? I'm not sure how practical is to leave that as 'others', because that puts it at the end of the string. We'd want the element that's more important/with more variants to be towards the start of the string so we can run all tests of the same kind with the -r option. While also looking at different functions for figuring out the transport and invocation, my observation was that, there might be many capabilities added to the same test, while it might not be important also. Ex: /migrate/multifd/tcp/plain 1. multifd is defined as a migration mode. 2. It is also a capability, and comes in 2 parts [multifd, multifd-channels] though one is a capability and another is parameter Similarly in other examples of compression, there are many capabilities and parameters added, but it might be not important to mention that ? Secondly, there are multiple migration capabilities IIRC (> 15). And a test requiring multiple capabilities, the overall string would be too long, and not that important also to mention all capabilities. Just thinking out of mind - Can we have selective list of capabilities ? 1. multifd 2. compress (again, there might be confusion with multifd compression methods like zstd, zlib and just 'compress') 3. zero-page (This will have sub capabilities ?) test-type:: migrate | validate We could alternatively drop migration|migrate|validate. They are kind of superfluous. I agree with the above comment. 'migrate' and 'validate' have a different set of variables required, some necessary, while other optional. IMO this will help is in streamlining the design further. migration-mode a. migrate --> :: precopy | postcopy | multifd b. validate -->:: (what to validate) methods :: preempt | recovery | reboot | suspend | simple I want some inputs here. 1. is there a better variable name rather than 'methods' 2. 'simple' does not fit perfect here IMO. transport:: tcp | fd | unix | file invocation :: uri | channels | both CompressionType :: zlib | zstd | none s/none/nocomp/ ? We're already familiar with that. Ack. Will change that. encryptionType :: tls | plain s/plain/notls/ ? What if there is another encryption technique in future ? Or maybe we simply omit the noop options. It would make the string way shorter in most cases. This might be a better approach. Can have some keys/variables as optional while some necessary. For ex: for 'migrate' - transport and invocation might be necessary while it might not be necessary for 'validate' qtests validate-test-result :: success | failure others :: other comments/capability that needs to be addressed. Can be multiple (more than one applicable, separated by using '-' in between) O: optional Signed-off-by: Het Gala Suggested-by: Fabiano Rosas --- tests/qtest/migration-test.c | 143 ++- 1 file changed, 72 insertions(+), 71 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index bd9f4b9dbb..bf4d000b76 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c Regards, Het Gala Regards, Het Gala
Re: [PATCH] tests/qtest: Standardize qtest function caller strings.
On 27/03/24 2:37 am, Fabiano Rosas wrote: Het Gala writes: Some comments, mostly just thinking out loud... For --> migrate // //O:/... For --> validate ///O:/O:/ /O:/O:/... Do we need an optional 'capability' element? I'm not sure how practical is to leave that as 'others', because that puts it at the end of the string. We'd want the element that's more important/with more variants to be towards the start of the string so we can run all tests of the same kind with the -r option. While also looking at different functions for figuring out the transport and invocation, my observation was that, there might be many capabilities added to the same test, while it might not be important also. Ex: /migrate/multifd/tcp/plain 1. multifd is defined as a migration mode. 2. It is also a capability, and comes in 2 parts [multifd, multifd-channels] though one is a capability and another is parameter Similarly in other examples of compression, there are many capabilities and parameters added, but it might be not important to mention that ? Secondly, there are multiple migration capabilities IIRC (> 15). And a test requiring multiple capabilities, the overall string would be too long, and not that important also to mention all capabilities. Just thinking out of mind - Can we have selective list of capabilities ? 1. multifd 2. compress (again, there might be confusion with multifd compression methods like zstd, zlib and just 'compress') 3. zero-page (This will have sub capabilities ?) test-type:: migrate | validate We could alternatively drop migration|migrate|validate. They are kind of superfluous. I agree with the above comment. 'migrate' and 'validate' have a different set of variables required, some necessary, while other optional. IMO this will help is in streamlining the design further. migration-mode a. migrate --> :: precopy | postcopy | multifd b. validate -->:: (what to validate) methods :: preempt | recovery | reboot | suspend | simple I want some inputs here. 1. is there a better variable name rather than 'methods' 2. 'simple' does not fit perfect here IMO. transport:: tcp | fd | unix | file invocation :: uri | channels | both CompressionType :: zlib | zstd | none s/none/nocomp/ ? We're already familiar with that. Ack. Will change that. encryptionType :: tls | plain s/plain/notls/ ? What if there is another encryption technique in future ? Or maybe we simply omit the noop options. It would make the string way shorter in most cases. This might be a better approach. Can have some keys/variables as optional while some necessary. For ex: for 'migrate' - transport and invocation might be necessary while it might not be necessary for 'validate' qtests validate-test-result :: success | failure others :: other comments/capability that needs to be addressed. Can be multiple (more than one applicable, separated by using '-' in between) O: optional Signed-off-by: Het Gala Suggested-by: Fabiano Rosas --- tests/qtest/migration-test.c | 143 ++- 1 file changed, 72 insertions(+), 71 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index bd9f4b9dbb..bf4d000b76 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c Regards, Het Gala
Re: [PATCH] tests/qtest: Standardize qtest function caller strings.
Hi Maintainers, this is my first attempt trying to standardise the grammar around migration qtests. I am not sure at multiple places on whether a particular string is placed at the right place with the right grammar. Please comment on the patch, if anything you feel can be improvised in the existing design. Suggestions are most welcomed :) Thank you Fabiano for initiating the idea on having a strict convention to call migration qtests. Hope that this would help further down the lane for anyone to call qtests in particular manner. (For now, have not focused on the character width limit, will comply with that once, we are on a consensus with the design) On 27/03/24 1:08 am, Het Gala wrote: For --> migrate // //O:/... For --> validate ///O:/O:/ /O:/O:/... test-type:: migrate | validate migration-mode a. migrate --> :: precopy | postcopy | multifd b. validate -->:: (what to validate) methods :: preempt | recovery | reboot | suspend | simple transport:: tcp | fd | unix | file invocation :: uri | channels | both CompressionType :: zlib | zstd | none encryptionType :: tls | plain validate-test-result :: success | failure others :: other comments/capability that needs to be addressed. Can be multiple (more than one applicable, separated by using '-' in between) O: optional Signed-off-by: Het Gala Suggested-by: Fabiano Rosas --- tests/qtest/migration-test.c | 143 ++- 1 file changed, 72 insertions(+), 71 deletions(-) diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index bd9f4b9dbb..bf4d000b76 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -3620,62 +3620,63 @@ int main(int argc, char **argv) Regards, Het Gala
[PATCH] tests/qtest: Standardize qtest function caller strings.
For --> migrate
//
//O:/...
For --> validate
///O:/O:/
/O:/O:/...
test-type:: migrate | validate
migration-mode
a. migrate --> :: precopy | postcopy | multifd
b. validate -->:: (what to validate)
methods :: preempt | recovery | reboot | suspend | simple
transport:: tcp | fd | unix | file
invocation :: uri | channels | both
CompressionType :: zlib | zstd | none
encryptionType :: tls | plain
validate-test-result :: success | failure
others :: other comments/capability that needs to be
addressed. Can be multiple
(more than one applicable, separated by using '-' in between)
O: optional
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 143 ++-
1 file changed, 72 insertions(+), 71 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index bd9f4b9dbb..bf4d000b76 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -3620,62 +3620,63 @@ int main(int argc, char **argv)
module_call_init(MODULE_INIT_QOM);
if (is_x86) {
-migration_test_add("/migration/precopy/unix/suspend/live",
+migration_test_add("/migrate/precopy/suspend/unix/uri/none/plain/live",
test_precopy_unix_suspend_live);
-migration_test_add("/migration/precopy/unix/suspend/notlive",
+
migration_test_add("/migrate/precopy/suspend/unix/uri/none/plain/notlive",
test_precopy_unix_suspend_notlive);
}
if (has_uffd) {
-migration_test_add("/migration/postcopy/plain", test_postcopy);
-migration_test_add("/migration/postcopy/recovery/plain",
- test_postcopy_recovery);
-migration_test_add("/migration/postcopy/preempt/plain",
+migration_test_add("/migrate/postcopy/simple/tcp/uri/none/plain",
+ test_postcopy);
+migration_test_add("/migrate/postcopy/recovery/tcp/uri/none/plain",
+ test_postcopy_recovery);
+migration_test_add("/migrate/postcopy/preempt/tcp/uri/none/plain",
test_postcopy_preempt);
-migration_test_add("/migration/postcopy/preempt/recovery/plain",
+
migration_test_add("/migrate/postcopy/preempt-recovery/tcp/uri/none/plain",
test_postcopy_preempt_recovery);
if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-migration_test_add("/migration/postcopy/compress/plain",
+
migration_test_add("/migrate/postcopy/simple/tcp/uri/none/plain/compress",
test_postcopy_compress);
-migration_test_add("/migration/postcopy/recovery/compress/plain",
+
migration_test_add("/migrate/postcopy/recovery/tcp/uri/none/plain/compress",
test_postcopy_recovery_compress);
}
#ifndef _WIN32
-migration_test_add("/migration/postcopy/recovery/double-failures",
+
migration_test_add("/migrate/postcopy/recovery/tcp/uri/none/none/plain/double-failures",
test_postcopy_recovery_double_fail);
#endif /* _WIN32 */
if (is_x86) {
-migration_test_add("/migration/postcopy/suspend",
+migration_test_add("/migrate/postcopy/suspend/tcp/uri/none/plain",
test_postcopy_suspend);
}
}
-migration_test_add("/migration/bad_dest", test_baddest);
+migration_test_add("/migrate/precopy/simple/tcp/uri/none/plain/bad_dest",
test_baddest);
#ifndef _WIN32
if (!g_str_equal(arch, "s390x")) {
-migration_test_add("/migration/analyze-script", test_analyze_script);
+
migration_test_add("/migrate/precopy/simple/file/uri/none/plain/analyze-script",
test_analyze_script);
}
#endif
-migration_test_add("/migration/precopy/unix/plain",
+migration_test_add("/migrate/precopy/simple/unix/uri/none/plain/live",
test_precopy_unix_plain);
-migration_test_add("/migration/precopy/unix/xbzrle",
+
migration_test_add("/migrate/precopy/simple/unix/uri/none/plain/xbzrle-live",
test_precopy_unix_xbzrle);
/*
* Compression fails from time to time.
* Put test here but don't enable it until everything is fixed.
*/
if (getenv("QEMU_TEST_FLAKY_TESTS")) {
-migration_test_add("/migration/precopy/unix/compress/wait",
+
migration_test_add("/mig
Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
On 20/03/24 6:49 pm, Fabiano Rosas wrote:
Het Gala writes:
On 20/03/24 3:27 am, Peter Xu wrote:
On Tue, Mar 19, 2024 at 08:48:39PM +, Het Gala wrote:
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Hmm, this patch isn't as clear to me. Even if this can return NULL now,
it'll soon crash at some later point, no?
It won't crash IMO, the next function SocketAddress_to_str for a non-socket
type would return an proper error ?
IMHO such patch is more suitable to be included in the same patch where
such new tests will be introduced, then we're addressing some real test
code changes that will work, rather than worrying on what will happen in
the future (and as I mentioned, i don't think it fully resolved that either..)
Thanks,
Maybe, Fabiano can pick this patch, and add along file migration qtests
patch ?
Yep.
Thanks
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
--
2.22.3
Regards,
Het Gala
Regards,
Het Gala
Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
On 20/03/24 3:27 am, Peter Xu wrote:
On Tue, Mar 19, 2024 at 08:48:39PM +, Het Gala wrote:
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Hmm, this patch isn't as clear to me. Even if this can return NULL now,
it'll soon crash at some later point, no?
It won't crash IMO, the next function SocketAddress_to_str for a non-socket
type would return an proper error ?
IMHO such patch is more suitable to be included in the same patch where
such new tests will be introduced, then we're addressing some real test
code changes that will work, rather than worrying on what will happen in
the future (and as I mentioned, i don't think it fully resolved that either..)
Thanks,
Maybe, Fabiano can pick this patch, and add along file migration qtests
patch ?
Suggested-by: Fabiano Rosas
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
--
2.22.3
Regards,
Het Gala
Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 20/03/24 12:33 am, Fabiano Rosas wrote:
Het Gala writes:
On 18/03/24 7:46 pm, Fabiano Rosas wrote:
Het Gala writes:
On 15/03/24 6:28 pm, Fabiano Rosas wrote:
Het Galawrites:
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
Just a heads up, none of what I'm about to say applies to current
master.
This can return NULL if there is no socket-address, such as with a file
migration. Then the visitor code below just barfs. It would be nice if
we touched this up eventually.
Yes. I agree this is not full proof solution and covers for all the cases.
It would only for 'socket-address'. Could you elaborate on what other than
socket-address the QObject can have ?
I can just not have the socket-address, AFAICS. We'd just need to not
crash if that's the case.
value: {
"status": "setup",
"socket-address": [
{
"port": "46213",
"ipv4": true,
"host": "127.0.0.1",
"type": "inet"
}
]
}
Okay, I understood your ask here. This is what gets printed from the QDict.
Let me raise a patch to return with a message if the QDict does not have key
with 'socket-address'. This would prevent the crash later on.
I wanted to know what other than "socket-address" key can he QDict give us
because, if that's the case, for other than socket migration, then we can
make this function more generic rather than having it as
'migrate_get_socket_address'
For now, there's nothing else. Let's just ignore when socket-address is
missing in the reply so we don't break future tests that use a
non-socket type.
Okay, Done. Can find the build here:
https://gitlab.com/galahet/Qemu/-/pipelines/1219841944
I only noticed this because I was fiddling with the file migration API
and this series helped me a lot to test my changes. So thanks for that,
Het.
Another point is: we really need to encourage people to write tests
using the new channels API. I added the FileMigrationArgs with the
'offset' as a required parameter, not even knowing optional parameters
were a thing. So it's obviously not enough to write support for the new
API if no tests ever touch it.
Yes, definitely we need more tests with the new channels API to test other
than just tcp connection. I could give a try for vsock and unix with the
new QAPI syntax, and add some tests.
I also wanted to bring in attention that, this solution I what i feel is
also
not complete. If we are using new channel syntax for migrate_qmp, then the
same syntax should also be used for migrate_qmp_incoming. But we haven't
touch that, and it still prints the old syntax. We might need a lot changes
in design maybe to incorporate that too in new tests totally with the new
syntax.
Adding migrate_qmp_incoming support should be relatively simple. You had
patches for that in another version, no?
No Fabiano, what I meant was, in migration-test.c, change in
migrate_incoming_qmp
would need to change the callback function and ultimately change all the
callback
handlers ? In that sense, it would require a big change ?
Inside the migrate_incoming_qmp function, adding implementation for
channels is
same as other migrate_* function.
You could add the parameter to migrate_incoming_qmp and use NULL when
calling. The callbacks don't need to be changed. When we add more tests
then we'd alter the callbacks accordingly.
I might convert the file tests soon, you can leave that part to me if
you want.
Okay, sure. Will leave those changes to you.
Another thing that you also noted down while discussing on the patches that
we should have a standard pattern on how to define the migration tests. Even
that would be helpful for the users, on how to add new tests, where to add
new tests in the file, and which test is needed to run if a specific change
needs to be tested.
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+ addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
Regards,
Het Gala
Re: [PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
FYI: This 2 patches are rebased on top of another
(tests/qtest/migration: Add tests for introducing 'channels' argument in
migrate QAPIs) series.
Can find the build for both the patches here:
https://gitlab.com/galahet/Qemu/-/pipelines/1219841944
On 20/03/24 2:18 am, Het Gala wrote:
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Suggested-by: Fabiano Rosas
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
Regards,
Het Gala
[PATCH 2/2] tests/qtest/migration: Fix typo for vsock in SocketAddress_to_str
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index fb7156f09a..651c6c555a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -42,7 +42,7 @@ static char *SocketAddress_to_str(SocketAddress *addr)
case SOCKET_ADDRESS_TYPE_FD:
return g_strdup_printf("fd:%s", addr->u.fd.str);
case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
+return g_strdup_printf("vsock:%s:%s",
addr->u.vsock.cid,
addr->u.vsock.port);
default:
--
2.22.3
[PATCH 1/2] tests/qtest/migration: Ignore if socket-address is missing to avoid crash below
'object' can return NULL if there is no socket-address, such as with a
file migration. Then the visitor code below fails and the test crashes.
Ignore and return NULL when socket-address is missing in the reply so
we don't break future tests that use a non-socket type.
Suggested-by: Fabiano Rosas
Signed-off-by: Het Gala
---
tests/qtest/migration-helpers.c | 4
1 file changed, 4 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b2a90469fb..fb7156f09a 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -90,6 +90,10 @@ static SocketAddress *migrate_get_socket_address(QTestState
*who)
QObject *object;
rsp = migrate_query(who);
+
+if (!qdict_haskey(rsp, "socket-address")) {
+return NULL;
+}
object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
--
2.22.3
Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 18/03/24 7:46 pm, Fabiano Rosas wrote:
Het Gala writes:
On 15/03/24 6:28 pm, Fabiano Rosas wrote:
Het Gala writes:
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
Just a heads up, none of what I'm about to say applies to current
master.
This can return NULL if there is no socket-address, such as with a file
migration. Then the visitor code below just barfs. It would be nice if
we touched this up eventually.
Yes. I agree this is not full proof solution and covers for all the cases.
It would only for 'socket-address'. Could you elaborate on what other than
socket-address the QObject can have ?
I can just not have the socket-address, AFAICS. We'd just need to not
crash if that's the case.
value: {
"status": "setup",
"socket-address": [
{
"port": "46213",
"ipv4": true,
"host": "127.0.0.1",
"type": "inet"
}
]
}
Okay, I understood your ask here. This is what gets printed from the QDict.
Let me raise a patch to return with a message if the QDict does not have key
with 'socket-address'. This would prevent the crash later on.
I wanted to know what other than "socket-address" key can he QDict give us
because, if that's the case, for other than socket migration, then we can
make this function more generic rather than having it as
'migrate_get_socket_address'
I only noticed this because I was fiddling with the file migration API
and this series helped me a lot to test my changes. So thanks for that,
Het.
Another point is: we really need to encourage people to write tests
using the new channels API. I added the FileMigrationArgs with the
'offset' as a required parameter, not even knowing optional parameters
were a thing. So it's obviously not enough to write support for the new
API if no tests ever touch it.
Yes, definitely we need more tests with the new channels API to test other
than just tcp connection. I could give a try for vsock and unix with the
new QAPI syntax, and add some tests.
I also wanted to bring in attention that, this solution I what i feel is
also
not complete. If we are using new channel syntax for migrate_qmp, then the
same syntax should also be used for migrate_qmp_incoming. But we haven't
touch that, and it still prints the old syntax. We might need a lot changes
in design maybe to incorporate that too in new tests totally with the new
syntax.
Adding migrate_qmp_incoming support should be relatively simple. You had
patches for that in another version, no?
No Fabiano, what I meant was, in migration-test.c, change in
migrate_incoming_qmp
would need to change the callback function and ultimately change all the
callback
handlers ? In that sense, it would require a big change ?
Inside the migrate_incoming_qmp function, adding implementation for
channels is
same as other migrate_* function.
Another thing that you also noted down while discussing on the patches that
we should have a standard pattern on how to define the migration tests. Even
that would be helpful for the users, on how to add new tests, where to add
new tests in the file, and which test is needed to run if a specific change
needs to be tested.
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migra
Re: [PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 15/03/24 6:28 pm, Fabiano Rosas wrote:
Het Gala writes:
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
Just a heads up, none of what I'm about to say applies to current
master.
This can return NULL if there is no socket-address, such as with a file
migration. Then the visitor code below just barfs. It would be nice if
we touched this up eventually.
Yes. I agree this is not full proof solution and covers for all the cases.
It would only for 'socket-address'. Could you elaborate on what other than
socket-address the QObject can have ?
I only noticed this because I was fiddling with the file migration API
and this series helped me a lot to test my changes. So thanks for that,
Het.
Another point is: we really need to encourage people to write tests
using the new channels API. I added the FileMigrationArgs with the
'offset' as a required parameter, not even knowing optional parameters
were a thing. So it's obviously not enough to write support for the new
API if no tests ever touch it.
Yes, definitely we need more tests with the new channels API to test other
than just tcp connection. I could give a try for vsock and unix with the
new QAPI syntax, and add some tests.
I also wanted to bring in attention that, this solution I what i feel is
also
not complete. If we are using new channel syntax for migrate_qmp, then the
same syntax should also be used for migrate_qmp_incoming. But we haven't
touch that, and it still prints the old syntax. We might need a lot changes
in design maybe to incorporate that too in new tests totally with the new
syntax.
Another thing that you also noted down while discussing on the patches that
we should have a standard pattern on how to define the migration tests. Even
that would be helpful for the users, on how to add new tests, where to add
new tests in the file, and which test is needed to run if a specific change
needs to be tested.
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migrate_get_socket_address(who);
+connect_uri = SocketAddress_to_str(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_uri;
}
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to);
}
qdict_put_str(args, "uri", uri ? uri : connect_uri);
Regards,
Het Gala
Re: [PATCH v7 0/8] tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs
Can find the passed build here: https://gitlab.com/galahet/Qemu/-/pipelines/1210931136 On 13/03/24 1:56 am, Het Gala wrote: With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. [...] Het Gala (8): tests/qtest/migration: Add 'to' object into migrate_qmp() tests/qtest/migration: Replace connect_uri and move migrate_get_socket_address inside migrate_qmp tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address tests/qtest/migration: Add channels parameter in migrate_qmp_fail tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value tests/qtest/migration: Add channels parameter in migrate_qmp tests/qtest/migration: Add multifd_tcp_plain test using list of channels instead of uri tests/qtest/migration: Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 180 +--- 3 files changed, 257 insertions(+), 91 deletions(-) Regards, Het Gala
[PATCH v7 0/8] tests/qtest/migration: Add tests for introducing 'channels' argument in migrate QAPIs
With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. Patch Summary: - Patch 1-2: - Introduce 'to' object inside migrate_qmp() so and move the calls to migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri with args->connect_uri everywhere. Patch 3-6: - Add channels argument to allow both migration QAPI arguments independently into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to be changed from 0 to port value coming from migrate_get_socket_address. Add migrate_set_ports to address this change of port value. Patch 7-8: - Add 2 negative tests to validate mutually exhaustive behaviour of migration QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the initial entry point for migration QAPIs. v6->v7 Changelog: 1. Rebased the series withthe latest master 2. Added "tests/qtest/migration" prefix in all patches. v5->v6 Changelog: 1. Pass args->connect_channels variable instead of NULL while calling migrate_qmp for .../multifd/channels/plain/none test. Passing NULL beats the whole purpose of the test. v4->v5 Changelog: 1. Remove redundant imports from migration-test.c after moving helper functions to migration-helpers.c 2. call migrate_get_socket_address(to) and internally let qdict_get() call ???socket-address??? parameter to make more sense to the reader. 3. qdict needs to be freed, other small fixups. v3->v4 Changelog: 1. introduced migrate_get_connect_uri and migrate_get_connect_qdict to both used migrate_get_socket_address to get dest uri in socket- address, and then use SokcketAddress_to_qdict to convert it into qdict. 2. Misc code changes. v2->v3 Changelog: - 1. 'channels' introduction is not required now for migrate_qmp_incoming 2. Refactor the code into 7 different patches 3. 'channels' introduction is not required now for migrate_qmp_incoming 4. Remove custom function for converting string to MigrationChannelList 5. move calls for migrate_get_socket_address inside migrate_qmp so that migrate_set_ports can replace the QAPI's port with correct value. Het Gala (8): tests/qtest/migration: Add 'to' object into migrate_qmp() tests/qtest/migration: Replace connect_uri and move migrate_get_socket_address inside migrate_qmp tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address tests/qtest/migration: Add channels parameter in migrate_qmp_fail tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value tests/qtest/migration: Add channels parameter in migrate_qmp tests/qtest/migration: Add multifd_tcp_plain test using list of channels instead of uri tests/qtest/migration: Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 180 +--- 3 files changed, 257 insertions(+), 91 deletions(-) -- 2.22.3
[PATCH v7 7/8] tests/qtest/migration: Add multifd_tcp_plain test using list of channels instead of uri
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 62e52a29cc..7ea13c8e5b 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -655,6 +655,13 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: JSON-formatted list of src QEMU URIs. If a port is
+ * defined as '0' in any QDict key a value of '0' will be
+ * automatically converted to the correct destination port.
+ */
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2758,7 +2765,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer",
@@ -2803,6 +2810,21 @@ static void test_multifd_tcp_no_zero_page(void)
test_precopy_common();
}
+static void test_multifd_tcp_channels_none(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3712,8 +3734,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
migration_test_add("/migration/multifd/tcp/plain/zero-page/legacy",
test_multifd_tcp_zero_page_legacy);
migration_test_add("/migration/multifd/tcp/plain/zero-page/none",
--
2.22.3
[PATCH v7 3/8] tests/qtest/migration: Replace migrate_get_connect_uri inplace of migrate_get_socket_address
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migrate_get_socket_address(who);
+connect_uri = SocketAddress_to_str(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_uri;
}
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to);
}
qdict_put_str(args, "uri", uri ? uri : connect_uri);
--
2.22.3
[PATCH v7 2/8] tests/qtest/migration: Replace connect_uri and move migrate_get_socket_address inside migrate_qmp
Move the calls to migrate_get_socket_address() into migrate_qmp().
Get rid of connect_uri and replace it with args->connect_uri only
because 'to' object will help to generate connect_uri with the
correct port number.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 54 -
tests/qtest/migration-test.c| 83 -
2 files changed, 63 insertions(+), 74 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..3e8c19c4de 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,9 @@
#include "qemu/osdep.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
#include "migration-helpers.h"
@@ -24,6 +27,51 @@
*/
#define MIGRATION_STATUS_WAIT_TIMEOUT 120
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.inet.host,
+ addr->u.inet.port);
+case SOCKET_ADDRESS_TYPE_UNIX:
+return g_strdup_printf("unix:%s",
+ addr->u.q_unix.path);
+case SOCKET_ADDRESS_TYPE_FD:
+return g_strdup_printf("fd:%s", addr->u.fd.str);
+case SOCKET_ADDRESS_TYPE_VSOCK:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.vsock.cid,
+ addr->u.vsock.port);
+default:
+return g_strdup("unknown address type");
+}
+}
+
+static char *
+migrate_get_socket_address(QTestState *who, const char *parameter)
+{
+QDict *rsp;
+char *result;
+SocketAddressList *addrs;
+Visitor *iv = NULL;
+QObject *object;
+
+rsp = migrate_query(who);
+object = qdict_get(rsp, parameter);
+
+iv = qobject_input_visitor_new(object);
+visit_type_SocketAddressList(iv, NULL, , _abort);
+visit_free(iv);
+
+/* we are only using a single address */
+result = SocketAddress_to_str(addrs->value);
+
+qapi_free_SocketAddressList(addrs);
+qobject_unref(rsp);
+return result;
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -73,13 +121,17 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (!uri) {
+connect_uri = migrate_get_socket_address(to, "socket-address");
+}
+qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index dc1fc002f5..7f6a14b19a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,16 +13,12 @@
#include "qemu/osdep.h"
#include "libqtest.h"
-#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/range.h"
#include "qemu/sockets.h"
#include "chardev/char.h"
-#include "qapi/qapi-visit-sockets.h"
-#include "qapi/qobject-input-visitor.h"
-#include "qapi/qobject-output-visitor.h"
#include "crypto/tlscredspsk.h"
#include "qapi/qmp/qlist.h"
@@ -369,50 +365,6 @@ static void cleanup(const char *filename)
unlink(path);
}
-static char *SocketAddress_to_str(SocketAddress *addr)
-{
-switch (addr->type) {
-case SOCKET_ADDRESS_TYPE_INET:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.inet.host,
- addr->u.inet.port);
-case SOCKET_ADDRESS_TYPE_UNIX:
-return g_strdup_printf("unix:%s",
- addr->u.q_unix.path);
-case SOCKET_ADDRESS_TYPE_FD:
-return g_strdup_printf("fd:%s", addr->u.fd.str);
-case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.vsock.cid,
- addr->u.vsock.port);
-default:
-return g_strdup("unknown address type");
-}
-}
-
-static char *migrate_get_socket_address(QTest
[PATCH v7 1/8] tests/qtest/migration: Add 'to' object into migrate_qmp()
Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() inside migrate_qmp() to get
the port value. This is not applied to other migrate_qmp*
because they don't need the port.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 3 ++-
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..b6206a04fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -68,7 +68,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const
char *fmt, ...)
* Arguments are built from @fmt... (formatted like
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...)
{
va_list ap;
QDict *args;
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..e16a34c796 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,8 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 71895abb7f..dc1fc002f5 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1350,7 +1350,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_suspend(from, _state);
g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1500,7 +1500,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1588,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1669,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1708,7 +1708,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
wait_for_migration_complete(from);
pid = fork();
@@ -1777,7 +1777,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1873,7 +1873,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
wait_for_migration_complete(from);
/*
@@ -2029,7 +2029,7 @@ static void test_ignore_shared(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2605,7 +2605,7 @@ static void do_test_validate_uuid(MigrateStart *args,
bool should_fail)
/* Wait for the first serial output from the source */
wait_for
[PATCH v7 6/8] tests/qtest/migration: Add channels parameter in migrate_qmp
Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 22 +-
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index ed8d812e9d..b2a90469fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -133,10 +133,6 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
QListEntry *entry;
g_autofree const char *addr_port = NULL;
-if (channel_list == NULL) {
-return;
-}
-
addr = migrate_get_connect_qdict(to);
QLIST_FOREACH_ENTRY(channel_list, entry) {
@@ -208,11 +204,10 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...)
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
-QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -220,11 +215,20 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-if (!uri) {
+if (uri) {
+qdict_put_str(args, "uri", uri);
+} else if (!channels) {
connect_uri = migrate_get_connect_uri(to);
+qdict_put_str(args, "uri", connect_uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+QList *channel_list = qobject_to(QList, channels_obj);
+migrate_set_ports(to, channel_list);
+qdict_put_obj(args, "channels", channels_obj);
}
-migrate_set_ports(to, channel_list);
-qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 4e664148a5..1339835698 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,9 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(4, 5)
+G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index cda07f8f97..62e52a29cc 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1301,7 +1301,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, "{}");
+migrate_qmp(from, to, NULL, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1451,7 +1451,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", NULL, "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1539,7 +1539,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, to, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, NULL, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1620,7 +1620,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1659,7 +1659,7 @@ static void test_analyze_script(void)
uri = g_strdup_pr
[PATCH v7 5/8] tests/qtest/migration: Add migrate_set_ports into migrate_qmp to update migration port value
migrate_get_connect_qdict gets qdict with the dst QEMU parameters.
migrate_set_ports() from list of channels reads each QDict for port,
and fills the port with correct value in case it was 0 in the test.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 75 +
1 file changed, 75 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index f215f44467..ed8d812e9d 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -16,6 +16,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
#include "migration-helpers.h"
@@ -48,6 +50,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
+static QDict *SocketAddress_to_qdict(SocketAddress *addr)
+{
+QDict *dict = qdict_new();
+
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+qdict_put_str(dict, "type", "inet");
+qdict_put_str(dict, "host", addr->u.inet.host);
+qdict_put_str(dict, "port", addr->u.inet.port);
+break;
+case SOCKET_ADDRESS_TYPE_UNIX:
+qdict_put_str(dict, "type", "unix");
+qdict_put_str(dict, "path", addr->u.q_unix.path);
+break;
+case SOCKET_ADDRESS_TYPE_FD:
+qdict_put_str(dict, "type", "fd");
+qdict_put_str(dict, "str", addr->u.fd.str);
+break;
+case SOCKET_ADDRESS_TYPE_VSOCK:
+qdict_put_str(dict, "type", "vsock");
+qdict_put_str(dict, "cid", addr->u.vsock.cid);
+qdict_put_str(dict, "port", addr->u.vsock.port);
+break;
+default:
+g_assert_not_reached();
+break;
+}
+
+return dict;
+}
+
static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
@@ -81,6 +114,46 @@ migrate_get_connect_uri(QTestState *who)
return connect_uri;
}
+static QDict *
+migrate_get_connect_qdict(QTestState *who)
+{
+SocketAddress *addrs;
+QDict *connect_qdict;
+
+addrs = migrate_get_socket_address(who);
+connect_qdict = SocketAddress_to_qdict(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_qdict;
+}
+
+static void migrate_set_ports(QTestState *to, QList *channel_list)
+{
+QDict *addr;
+QListEntry *entry;
+g_autofree const char *addr_port = NULL;
+
+if (channel_list == NULL) {
+return;
+}
+
+addr = migrate_get_connect_qdict(to);
+
+QLIST_FOREACH_ENTRY(channel_list, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QDict *addrdict = qdict_get_qdict(channel, "addr");
+
+if (qdict_haskey(addrdict, "port") &&
+qdict_haskey(addr, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+addr_port = qdict_get_str(addr, "port");
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+
+qobject_unref(addr);
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -139,6 +212,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -149,6 +223,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_connect_uri(to);
}
+migrate_set_ports(to, channel_list);
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
--
2.22.3
[PATCH v7 4/8] tests/qtest/migration: Add channels parameter in migrate_qmp_fail
Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 13 +++--
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 4 ++--
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 8806dc841e..f215f44467 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -100,7 +100,8 @@ bool migrate_watch_for_events(QTestState *who, const char
*name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
@@ -110,7 +111,15 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
const char *fmt, ...)
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
err = qtest_qmp_assert_failure_ref(
who, "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e16a34c796..4e664148a5 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,8 +33,9 @@ G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...);
void migrate_set_capability(QTestState *who, const char *capability,
bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7f6a14b19a..cda07f8f97 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1717,7 +1717,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
@@ -1812,7 +1812,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
--
2.22.3
[PATCH v7 8/8] tests/qtest/migration: Add negative tests to validate migration QAPIs
Migration QAPI arguments - uri and channels are mutually exhaustive.
Add negative validation tests, one with both arguments present and
one with none present.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 55 +++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 7ea13c8e5b..bd9f4b9dbb 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
+migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
goto finish;
}
@@ -2608,6 +2608,55 @@ static void test_validate_uuid_dst_not_set(void)
do_test_validate_uuid(, false);
}
+static void do_test_validate_uri_channel(MigrateCommon *args)
+{
+QTestState *from, *to;
+
+if (test_migrate_start(, , args->listen_uri, >start)) {
+return;
+}
+
+/* Wait for the first serial output from the source */
+wait_for_serial("src_serial");
+
+/*
+ * 'uri' and 'channels' validation is checked even before the migration
+ * starts.
+ */
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
+test_migrate_end(from, to, false);
+}
+
+static void test_validate_uri_channels_both_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+.connect_uri = "tcp:127.0.0.1:0",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+do_test_validate_uri_channel();
+}
+
+static void test_validate_uri_channels_none_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+};
+
+do_test_validate_uri_channel();
+}
+
/*
* The way auto_converge works, we need to do too many passes to
* run this test. Auto_converge logic is only run once every
@@ -3722,6 +3771,10 @@ int main(int argc, char **argv)
test_validate_uuid_src_not_set);
migration_test_add("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
+migration_test_add("/migration/validate_uri/channels/both_set",
+ test_validate_uri_channels_both_set);
+migration_test_add("/migration/validate_uri/channels/none_set",
+ test_validate_uri_channels_none_set);
/*
* See explanation why this test is slow on function definition
*/
--
2.22.3
Re: [PATCH v6 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
On 13/03/24 12:44 am, Peter Xu wrote: Het, Could you rebase this to latest master? Meanwhile, please provide a prefix in each of the subjects, it can be any of "tests/migration-test:", "tests/qtest/migration", "tests/migration", etc. I don't think we have a rule yet.. but any such prefix is still helpful. Thanks, Yes, sure Peter. Let me rebase it to latest master and also share the latest passed build link. Will add "tests/qtest/migration" prefix to all the commit message headers. Regards, Het Gala
[PATCH v6 4/8] Add channels parameter in migrate_qmp_fail
Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 13 +++--
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 4 ++--
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 8806dc841e..f215f44467 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -100,7 +100,8 @@ bool migrate_watch_for_events(QTestState *who, const char
*name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
@@ -110,7 +111,15 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
const char *fmt, ...)
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
err = qtest_qmp_assert_failure_ref(
who, "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e16a34c796..4e664148a5 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,8 +33,9 @@ G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...);
void migrate_set_capability(QTestState *who, const char *capability,
bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9bb24fd7c5..da4b0006c7 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1717,7 +1717,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
@@ -1812,7 +1812,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
--
2.22.3
[PATCH v6 6/8] Add channels parameter in migrate_qmp
Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 22 +-
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index ed8d812e9d..b2a90469fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -133,10 +133,6 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
QListEntry *entry;
g_autofree const char *addr_port = NULL;
-if (channel_list == NULL) {
-return;
-}
-
addr = migrate_get_connect_qdict(to);
QLIST_FOREACH_ENTRY(channel_list, entry) {
@@ -208,11 +204,10 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...)
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
-QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -220,11 +215,20 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-if (!uri) {
+if (uri) {
+qdict_put_str(args, "uri", uri);
+} else if (!channels) {
connect_uri = migrate_get_connect_uri(to);
+qdict_put_str(args, "uri", connect_uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+QList *channel_list = qobject_to(QList, channels_obj);
+migrate_set_ports(to, channel_list);
+qdict_put_obj(args, "channels", channels_obj);
}
-migrate_set_ports(to, channel_list);
-qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 4e664148a5..1339835698 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,9 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(4, 5)
+G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index da4b0006c7..bf27766eb0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1301,7 +1301,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, "{}");
+migrate_qmp(from, to, NULL, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1451,7 +1451,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", NULL, "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1539,7 +1539,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, to, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, NULL, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1620,7 +1620,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1659,7 +1659,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat >
[PATCH v6 3/8] Replace migrate_get_connect_uri inplace of migrate_get_socket_address
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migrate_get_socket_address(who);
+connect_uri = SocketAddress_to_str(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_uri;
}
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to);
}
qdict_put_str(args, "uri", uri ? uri : connect_uri);
--
2.22.3
[PATCH v6 2/8] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp
Move the calls to migrate_get_socket_address() into migrate_qmp().
Get rid of connect_uri and replace it with args->connect_uri only
because 'to' object will help to generate connect_uri with the
correct port number.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 54 -
tests/qtest/migration-test.c| 83 -
2 files changed, 63 insertions(+), 74 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..3e8c19c4de 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,9 @@
#include "qemu/osdep.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
#include "migration-helpers.h"
@@ -24,6 +27,51 @@
*/
#define MIGRATION_STATUS_WAIT_TIMEOUT 120
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.inet.host,
+ addr->u.inet.port);
+case SOCKET_ADDRESS_TYPE_UNIX:
+return g_strdup_printf("unix:%s",
+ addr->u.q_unix.path);
+case SOCKET_ADDRESS_TYPE_FD:
+return g_strdup_printf("fd:%s", addr->u.fd.str);
+case SOCKET_ADDRESS_TYPE_VSOCK:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.vsock.cid,
+ addr->u.vsock.port);
+default:
+return g_strdup("unknown address type");
+}
+}
+
+static char *
+migrate_get_socket_address(QTestState *who, const char *parameter)
+{
+QDict *rsp;
+char *result;
+SocketAddressList *addrs;
+Visitor *iv = NULL;
+QObject *object;
+
+rsp = migrate_query(who);
+object = qdict_get(rsp, parameter);
+
+iv = qobject_input_visitor_new(object);
+visit_type_SocketAddressList(iv, NULL, , _abort);
+visit_free(iv);
+
+/* we are only using a single address */
+result = SocketAddress_to_str(addrs->value);
+
+qapi_free_SocketAddressList(addrs);
+qobject_unref(rsp);
+return result;
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -73,13 +121,17 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (!uri) {
+connect_uri = migrate_get_socket_address(to, "socket-address");
+}
+qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d9b4e28c12..9bb24fd7c5 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,16 +13,12 @@
#include "qemu/osdep.h"
#include "libqtest.h"
-#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/range.h"
#include "qemu/sockets.h"
#include "chardev/char.h"
-#include "qapi/qapi-visit-sockets.h"
-#include "qapi/qobject-input-visitor.h"
-#include "qapi/qobject-output-visitor.h"
#include "crypto/tlscredspsk.h"
#include "qapi/qmp/qlist.h"
@@ -369,50 +365,6 @@ static void cleanup(const char *filename)
unlink(path);
}
-static char *SocketAddress_to_str(SocketAddress *addr)
-{
-switch (addr->type) {
-case SOCKET_ADDRESS_TYPE_INET:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.inet.host,
- addr->u.inet.port);
-case SOCKET_ADDRESS_TYPE_UNIX:
-return g_strdup_printf("unix:%s",
- addr->u.q_unix.path);
-case SOCKET_ADDRESS_TYPE_FD:
-return g_strdup_printf("fd:%s", addr->u.fd.str);
-case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.vsock.cid,
- addr->u.vsock.port);
-default:
-return g_strdup("unknown address type");
-}
-}
-
-static char *migrate_get_socket_address(QTestState *who, const char *param
[PATCH v6 1/8] Add 'to' object into migrate_qmp()
Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() inside migrate_qmp() to get
the port value. This is not applied to other migrate_qmp*
because they don't need the port.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 3 ++-
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..b6206a04fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -68,7 +68,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const
char *fmt, ...)
* Arguments are built from @fmt... (formatted like
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...)
{
va_list ap;
QDict *args;
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..e16a34c796 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,8 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4023d808f9..d9b4e28c12 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1350,7 +1350,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_suspend(from, _state);
g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1500,7 +1500,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1588,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1669,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1708,7 +1708,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
wait_for_migration_complete(from);
pid = fork();
@@ -1777,7 +1777,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1873,7 +1873,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
wait_for_migration_complete(from);
/*
@@ -2029,7 +2029,7 @@ static void test_ignore_shared(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2605,7 +2605,7 @@ static void do_test_validate_uuid(MigrateStart *args,
bool should_fail)
/* Wait for the first serial output from the source */
wait_for
[PATCH v6 5/8] Add migrate_set_ports into migrate_qmp to update migration port value
migrate_get_connect_qdict gets qdict with the dst QEMU parameters.
migrate_set_ports() from list of channels reads each QDict for port,
and fills the port with correct value in case it was 0 in the test.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 75 +
1 file changed, 75 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index f215f44467..ed8d812e9d 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -16,6 +16,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
#include "migration-helpers.h"
@@ -48,6 +50,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
+static QDict *SocketAddress_to_qdict(SocketAddress *addr)
+{
+QDict *dict = qdict_new();
+
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+qdict_put_str(dict, "type", "inet");
+qdict_put_str(dict, "host", addr->u.inet.host);
+qdict_put_str(dict, "port", addr->u.inet.port);
+break;
+case SOCKET_ADDRESS_TYPE_UNIX:
+qdict_put_str(dict, "type", "unix");
+qdict_put_str(dict, "path", addr->u.q_unix.path);
+break;
+case SOCKET_ADDRESS_TYPE_FD:
+qdict_put_str(dict, "type", "fd");
+qdict_put_str(dict, "str", addr->u.fd.str);
+break;
+case SOCKET_ADDRESS_TYPE_VSOCK:
+qdict_put_str(dict, "type", "vsock");
+qdict_put_str(dict, "cid", addr->u.vsock.cid);
+qdict_put_str(dict, "port", addr->u.vsock.port);
+break;
+default:
+g_assert_not_reached();
+break;
+}
+
+return dict;
+}
+
static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
@@ -81,6 +114,46 @@ migrate_get_connect_uri(QTestState *who)
return connect_uri;
}
+static QDict *
+migrate_get_connect_qdict(QTestState *who)
+{
+SocketAddress *addrs;
+QDict *connect_qdict;
+
+addrs = migrate_get_socket_address(who);
+connect_qdict = SocketAddress_to_qdict(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_qdict;
+}
+
+static void migrate_set_ports(QTestState *to, QList *channel_list)
+{
+QDict *addr;
+QListEntry *entry;
+g_autofree const char *addr_port = NULL;
+
+if (channel_list == NULL) {
+return;
+}
+
+addr = migrate_get_connect_qdict(to);
+
+QLIST_FOREACH_ENTRY(channel_list, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QDict *addrdict = qdict_get_qdict(channel, "addr");
+
+if (qdict_haskey(addrdict, "port") &&
+qdict_haskey(addr, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+addr_port = qdict_get_str(addr, "port");
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+
+qobject_unref(addr);
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -139,6 +212,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -149,6 +223,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_connect_uri(to);
}
+migrate_set_ports(to, channel_list);
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
--
2.22.3
[PATCH v6 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. Patch Summary: - Patch 1-2: - Introduce 'to' object inside migrate_qmp() so and move the calls to migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri with args->connect_uri everywhere. Patch 3-6: - Add channels argument to allow both migration QAPI arguments independently into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to be changed from 0 to port value coming from migrate_get_socket_address. Add migrate_set_ports to address this change of port value. Patch 7-8: - Add 2 negative tests to validate mutually exhaustive behaviour of migration QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the initial entry point for migration QAPIs. v5->v6 Changelog: 1. Pass args->connect_channels variable instead of NULL while calling migrate_qmp for .../multifd/channels/plain/none test. Passing NULL beats the whole purpose of the test. v4->v5 Changelog: 1. Remove redundant imports from migration-test.c after moving helper functions to migration-helpers.c 2. call migrate_get_socket_address(to) and internally let qdict_get() call ???socket-address??? parameter to make more sense to the reader. 3. qdict needs to be freed, other small fixups. v3->v4 Changelog: 1. introduced migrate_get_connect_uri and migrate_get_connect_qdict to both used migrate_get_socket_address to get dest uri in socket- address, and then use SokcketAddress_to_qdict to convert it into qdict. 2. Misc code changes. v2->v3 Changelog: - 1. 'channels' introduction is not required now for migrate_qmp_incoming 2. Refactor the code into 7 different patches 3. 'channels' introduction is not required now for migrate_qmp_incoming 4. Remove custom function for converting string to MigrationChannelList 5. move calls for migrate_get_socket_address inside migrate_qmp so that migrate_set_ports can replace the QAPI's port with correct value. Het Gala (8): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Replace migrate_get_connect_uri inplace of migrate_get_socket_address Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to update migration port value Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 180 +--- 3 files changed, 257 insertions(+), 91 deletions(-) -- 2.22.3
[PATCH v6 7/8] Add multifd_tcp_plain test using list of channels instead of uri
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index bf27766eb0..392d5d0b62 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -655,6 +655,13 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: JSON-formatted list of src QEMU URIs. If a port is
+ * defined as '0' in any QDict key a value of '0' will be
+ * automatically converted to the correct destination port.
+ */
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2740,7 +2747,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer",
@@ -2755,6 +2762,21 @@ static void test_multifd_tcp_none(void)
test_precopy_common();
}
+static void test_multifd_tcp_channels_none(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3664,8 +3686,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
migration_test_add("/migration/multifd/tcp/plain/cancel",
test_multifd_tcp_cancel);
migration_test_add("/migration/multifd/tcp/plain/zlib",
--
2.22.3
[PATCH v6 8/8] Add negative tests to validate migration QAPIs
Migration QAPI arguments - uri and channels are mutually exhaustive.
Add negative validation tests, one with both arguments present and
one with none present.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 55 +++-
1 file changed, 54 insertions(+), 1 deletion(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 392d5d0b62..e9801da701 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1724,7 +1724,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
+migrate_qmp_fail(from, args->connect_uri, args->connect_uri, "{}");
goto finish;
}
@@ -2608,6 +2608,55 @@ static void test_validate_uuid_dst_not_set(void)
do_test_validate_uuid(, false);
}
+static void do_test_validate_uri_channel(MigrateCommon *args)
+{
+QTestState *from, *to;
+
+if (test_migrate_start(, , args->listen_uri, >start)) {
+return;
+}
+
+/* Wait for the first serial output from the source */
+wait_for_serial("src_serial");
+
+/*
+ * 'uri' and 'channels' validation is checked even before the migration
+ * starts.
+ */
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
+test_migrate_end(from, to, false);
+}
+
+static void test_validate_uri_channels_both_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+.connect_uri = "tcp:127.0.0.1:0",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+do_test_validate_uri_channel();
+}
+
+static void test_validate_uri_channels_none_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+};
+
+do_test_validate_uri_channel();
+}
+
/*
* The way auto_converge works, we need to do too many passes to
* run this test. Auto_converge logic is only run once every
@@ -3674,6 +3723,10 @@ int main(int argc, char **argv)
test_validate_uuid_src_not_set);
migration_test_add("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
+migration_test_add("/migration/validate_uri/channels/both_set",
+ test_validate_uri_channels_both_set);
+migration_test_add("/migration/validate_uri/channels/none_set",
+ test_validate_uri_channels_none_set);
/*
* See explanation why this test is slow on function definition
*/
--
2.22.3
Re: [PATCH v5 7/8] Add multifd_tcp_plain test using list of channels instead of uri
Hi Fabiano and Peter, do not give Reviewed-by tag for this patch.
There is a small mistake here, forgot to replace NULL with
args->connect_channels while calling migrate_qmp inside
test_precopy_common.
This was not caught earlier because, in the case where channels
and uri both are absent, migrate_qmp still makes live migration
succeed.
Sending out a new patchset series - v6. Can directly review v6
patchset series.
On 12/03/24 3:23 am, Het Gala wrote:
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index bf27766eb0..392d5d0b62 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -655,6 +655,13 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: JSON-formatted list of src QEMU URIs. If a port is
+ * defined as '0' in any QDict key a value of '0' will be
+ * automatically converted to the correct destination port.
+ */
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2740,7 +2747,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer", @@ -2755,6 +2762,21 @@ static void test_multifd_tcp_none(void)
test_precopy_common(); } +static void
test_multifd_tcp_channels_none(void) +{ + MigrateCommon args = { +
.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3664,8 +3686,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
migration_test_add("/migration/multifd/tcp/plain/cancel",
test_multifd_tcp_cancel);
migration_test_add("/migration/multifd/tcp/plain/zlib",
Regards,
Het Gala
Re: [PATCH v5 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
Can also check the passed build at https://gitlab.com/galahet/Qemu/-/pipelines/1209497470 On 12/03/24 3:23 am, Het Gala wrote: With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. v4->v5 Changelog: 1. Remove redundant imports from migration-test.c after moving helper functions to migration-helpers.c 2. call migrate_get_socket_address(to) and internally let qdict_get() call “socket-address” parameter to make more sense to the reader. 3. qdict needs to be freed, other small fixups. [...] Het Gala (8): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Replace migrate_get_connect_uri inplace of migrate_get_socket_address Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to update migration port value Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 180 +--- 3 files changed, 257 insertions(+), 91 deletions(-) Regards, Het Gala
Re: [PATCH v4 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
On 12/03/24 3:08 am, Peter Xu wrote: On Tue, Mar 12, 2024 at 03:01:51AM +0530, Het Gala wrote: On 12/03/24 2:55 am, Peter Xu wrote: On Sat, Mar 09, 2024 at 01:11:45PM +0530, Het Gala wrote: Can find the reference to the githab pipeline (before patchset) : https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_galahet_Qemu_-2D_pipelines_1207185095=DwIBaQ=s883GpUCOChKOHiocYtGcg=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg=y2xUaOwvRVC5eTpFNEdxb37JYDdxN61W406HlCyx3CWIVyBRgLwjJhAYALZLinoi=vZRNX33_DuLO1TsfTpYR_s9bf_EMFm3oHHH_eg57zE0= Can find the reference to the githab pipeline (after patchset) : https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_galahet_Qemu_-2D_pipelines_1207183673=DwIBaQ=s883GpUCOChKOHiocYtGcg=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg=y2xUaOwvRVC5eTpFNEdxb37JYDdxN61W406HlCyx3CWIVyBRgLwjJhAYALZLinoi=C73ka3k3ouAuRJYNVLPIBQiWx3jDFDDvVYDiEYqfE04= Het, Please still copy me for any migration patches. In this case Fabiano is looking it'll be all fine, but it will still help me on marking the emails. Thanks, So sorry about that Peter. I am aware that you and Fabiano are the go to migration maintainers. I thought I emailed or cc'd all the stakeholders that should be involved for this patchset series. Even in earlier series of this patchset, you were cc'ed, but somehow I just forgot to cc you for this patchset. Sure, will take care from next time. Again apologies for the mixup :) No problem at all. As long as you have at least 1 maintainers copied, logically nothing will get lost. It's just that it helps me in the routines. Are you managing cc list manually for each version? In that case I suggest you have a look at Stefan's tool: I used to earlier. But lately markus introduced me to scripts/get_maintainers.pl -f It gives list of all the maintainers handling that particular file. So that helped me for this patchset. https://urldefense.proofpoint.com/v2/url?u=https-3A__github.com_stefanha_git-2Dpublish=DwIBaQ=s883GpUCOChKOHiocYtGcg=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg=ydJfb02Wuk_NnlYl8-RkRkYXzWNpzlEht7yj5kakeAlz_WPoD6yvC7b-fVCeLzom=8KSe9MiMzmHda3uZ_uaGCIEjub4tSzpeDTpZZwq5knc= Thanks a lot Peter, looks cool. Will try to explore and use git-publish and its different methods for next patchset. It might help a great deal in patch managements at least to me, and it definitely covers more than maintaining the cc list for a patchset. Yes, it looks like there are a lot of useful methods that I can leverage in future :) Regards, Het Gala
[PATCH v5 1/8] Add 'to' object into migrate_qmp()
Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() inside migrate_qmp() to get
the port value. This is not applied to other migrate_qmp*
because they don't need the port.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 3 ++-
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..b6206a04fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -68,7 +68,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const
char *fmt, ...)
* Arguments are built from @fmt... (formatted like
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...)
{
va_list ap;
QDict *args;
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..e16a34c796 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,8 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4023d808f9..d9b4e28c12 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1350,7 +1350,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_suspend(from, _state);
g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1500,7 +1500,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1588,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1669,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1708,7 +1708,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
wait_for_migration_complete(from);
pid = fork();
@@ -1777,7 +1777,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1873,7 +1873,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
wait_for_migration_complete(from);
/*
@@ -2029,7 +2029,7 @@ static void test_ignore_shared(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2605,7 +2605,7 @@ static void do_test_validate_uuid(MigrateStart *args,
bool should_fail)
/* Wait for the first serial output from the source */
wait_for
[PATCH v5 5/8] Add migrate_set_ports into migrate_qmp to update migration port value
migrate_get_connect_qdict gets qdict with the dst QEMU parameters.
migrate_set_ports() from list of channels reads each QDict for port,
and fills the port with correct value in case it was 0 in the test.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 75 +
1 file changed, 75 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index f215f44467..ed8d812e9d 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -16,6 +16,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+#include "qemu/cutils.h"
#include "migration-helpers.h"
@@ -48,6 +50,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
+static QDict *SocketAddress_to_qdict(SocketAddress *addr)
+{
+QDict *dict = qdict_new();
+
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+qdict_put_str(dict, "type", "inet");
+qdict_put_str(dict, "host", addr->u.inet.host);
+qdict_put_str(dict, "port", addr->u.inet.port);
+break;
+case SOCKET_ADDRESS_TYPE_UNIX:
+qdict_put_str(dict, "type", "unix");
+qdict_put_str(dict, "path", addr->u.q_unix.path);
+break;
+case SOCKET_ADDRESS_TYPE_FD:
+qdict_put_str(dict, "type", "fd");
+qdict_put_str(dict, "str", addr->u.fd.str);
+break;
+case SOCKET_ADDRESS_TYPE_VSOCK:
+qdict_put_str(dict, "type", "vsock");
+qdict_put_str(dict, "cid", addr->u.vsock.cid);
+qdict_put_str(dict, "port", addr->u.vsock.port);
+break;
+default:
+g_assert_not_reached();
+break;
+}
+
+return dict;
+}
+
static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
@@ -81,6 +114,46 @@ migrate_get_connect_uri(QTestState *who)
return connect_uri;
}
+static QDict *
+migrate_get_connect_qdict(QTestState *who)
+{
+SocketAddress *addrs;
+QDict *connect_qdict;
+
+addrs = migrate_get_socket_address(who);
+connect_qdict = SocketAddress_to_qdict(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_qdict;
+}
+
+static void migrate_set_ports(QTestState *to, QList *channel_list)
+{
+QDict *addr;
+QListEntry *entry;
+g_autofree const char *addr_port = NULL;
+
+if (channel_list == NULL) {
+return;
+}
+
+addr = migrate_get_connect_qdict(to);
+
+QLIST_FOREACH_ENTRY(channel_list, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QDict *addrdict = qdict_get_qdict(channel, "addr");
+
+if (qdict_haskey(addrdict, "port") &&
+qdict_haskey(addr, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+addr_port = qdict_get_str(addr, "port");
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+
+qobject_unref(addr);
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -139,6 +212,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -149,6 +223,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_connect_uri(to);
}
+migrate_set_ports(to, channel_list);
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
--
2.22.3
[PATCH v5 7/8] Add multifd_tcp_plain test using list of channels instead of uri
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index bf27766eb0..392d5d0b62 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -655,6 +655,13 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: JSON-formatted list of src QEMU URIs. If a port is
+ * defined as '0' in any QDict key a value of '0' will be
+ * automatically converted to the correct destination port.
+ */
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2740,7 +2747,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer",
@@ -2755,6 +2762,21 @@ static void test_multifd_tcp_none(void)
test_precopy_common();
}
+static void test_multifd_tcp_channels_none(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3664,8 +3686,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
migration_test_add("/migration/multifd/tcp/plain/cancel",
test_multifd_tcp_cancel);
migration_test_add("/migration/multifd/tcp/plain/zlib",
--
2.22.3
[PATCH v5 8/8] Add negative tests to validate migration QAPIs
Migration QAPI arguments - uri and channels are mutually exhaustive.
Add negative validation tests, one with both arguments present and
one with none present.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 53
1 file changed, 53 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 392d5d0b62..9e3146d23f 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2608,6 +2608,55 @@ static void test_validate_uuid_dst_not_set(void)
do_test_validate_uuid(, false);
}
+static void do_test_validate_uri_channel(MigrateCommon *args)
+{
+QTestState *from, *to;
+
+if (test_migrate_start(, , args->listen_uri, >start)) {
+return;
+}
+
+/* Wait for the first serial output from the source */
+wait_for_serial("src_serial");
+
+/*
+ * 'uri' and 'channels' validation is checked even before the migration
+ * starts.
+ */
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
+test_migrate_end(from, to, false);
+}
+
+static void test_validate_uri_channels_both_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+.connect_uri = "tcp:127.0.0.1:0",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+do_test_validate_uri_channel();
+}
+
+static void test_validate_uri_channels_none_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+};
+
+do_test_validate_uri_channel();
+}
+
/*
* The way auto_converge works, we need to do too many passes to
* run this test. Auto_converge logic is only run once every
@@ -3674,6 +3723,10 @@ int main(int argc, char **argv)
test_validate_uuid_src_not_set);
migration_test_add("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
+migration_test_add("/migration/validate_uri/channels/both_set",
+ test_validate_uri_channels_both_set);
+migration_test_add("/migration/validate_uri/channels/none_set",
+ test_validate_uri_channels_none_set);
/*
* See explanation why this test is slow on function definition
*/
--
2.22.3
[PATCH v5 2/8] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp
Move the calls to migrate_get_socket_address() into migrate_qmp().
Get rid of connect_uri and replace it with args->connect_uri only
because 'to' object will help to generate connect_uri with the
correct port number.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 54 -
tests/qtest/migration-test.c| 83 -
2 files changed, 63 insertions(+), 74 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..3e8c19c4de 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,9 @@
#include "qemu/osdep.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
#include "migration-helpers.h"
@@ -24,6 +27,51 @@
*/
#define MIGRATION_STATUS_WAIT_TIMEOUT 120
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.inet.host,
+ addr->u.inet.port);
+case SOCKET_ADDRESS_TYPE_UNIX:
+return g_strdup_printf("unix:%s",
+ addr->u.q_unix.path);
+case SOCKET_ADDRESS_TYPE_FD:
+return g_strdup_printf("fd:%s", addr->u.fd.str);
+case SOCKET_ADDRESS_TYPE_VSOCK:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.vsock.cid,
+ addr->u.vsock.port);
+default:
+return g_strdup("unknown address type");
+}
+}
+
+static char *
+migrate_get_socket_address(QTestState *who, const char *parameter)
+{
+QDict *rsp;
+char *result;
+SocketAddressList *addrs;
+Visitor *iv = NULL;
+QObject *object;
+
+rsp = migrate_query(who);
+object = qdict_get(rsp, parameter);
+
+iv = qobject_input_visitor_new(object);
+visit_type_SocketAddressList(iv, NULL, , _abort);
+visit_free(iv);
+
+/* we are only using a single address */
+result = SocketAddress_to_str(addrs->value);
+
+qapi_free_SocketAddressList(addrs);
+qobject_unref(rsp);
+return result;
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -73,13 +121,17 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (!uri) {
+connect_uri = migrate_get_socket_address(to, "socket-address");
+}
+qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d9b4e28c12..9bb24fd7c5 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -13,16 +13,12 @@
#include "qemu/osdep.h"
#include "libqtest.h"
-#include "qapi/error.h"
#include "qapi/qmp/qdict.h"
#include "qemu/module.h"
#include "qemu/option.h"
#include "qemu/range.h"
#include "qemu/sockets.h"
#include "chardev/char.h"
-#include "qapi/qapi-visit-sockets.h"
-#include "qapi/qobject-input-visitor.h"
-#include "qapi/qobject-output-visitor.h"
#include "crypto/tlscredspsk.h"
#include "qapi/qmp/qlist.h"
@@ -369,50 +365,6 @@ static void cleanup(const char *filename)
unlink(path);
}
-static char *SocketAddress_to_str(SocketAddress *addr)
-{
-switch (addr->type) {
-case SOCKET_ADDRESS_TYPE_INET:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.inet.host,
- addr->u.inet.port);
-case SOCKET_ADDRESS_TYPE_UNIX:
-return g_strdup_printf("unix:%s",
- addr->u.q_unix.path);
-case SOCKET_ADDRESS_TYPE_FD:
-return g_strdup_printf("fd:%s", addr->u.fd.str);
-case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.vsock.cid,
- addr->u.vsock.port);
-default:
-return g_strdup("unknown address type");
-}
-}
-
-static char *migrate_get_socket_address(QTestState *who, const char *param
[PATCH v5 6/8] Add channels parameter in migrate_qmp
Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 22 +-
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index ed8d812e9d..b2a90469fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -133,10 +133,6 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
QListEntry *entry;
g_autofree const char *addr_port = NULL;
-if (channel_list == NULL) {
-return;
-}
-
addr = migrate_get_connect_qdict(to);
QLIST_FOREACH_ENTRY(channel_list, entry) {
@@ -208,11 +204,10 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...)
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
-QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -220,11 +215,20 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-if (!uri) {
+if (uri) {
+qdict_put_str(args, "uri", uri);
+} else if (!channels) {
connect_uri = migrate_get_connect_uri(to);
+qdict_put_str(args, "uri", connect_uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+QList *channel_list = qobject_to(QList, channels_obj);
+migrate_set_ports(to, channel_list);
+qdict_put_obj(args, "channels", channels_obj);
}
-migrate_set_ports(to, channel_list);
-qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 4e664148a5..1339835698 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,9 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(4, 5)
+G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index da4b0006c7..bf27766eb0 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1301,7 +1301,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, "{}");
+migrate_qmp(from, to, NULL, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1451,7 +1451,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", NULL, "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1539,7 +1539,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, to, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, NULL, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1620,7 +1620,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1659,7 +1659,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat >
[PATCH v5 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. Patch Summary: - Patch 1-2: - Introduce 'to' object inside migrate_qmp() so and move the calls to migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri with args->connect_uri everywhere. Patch 3-6: - Add channels argument to allow both migration QAPI arguments independently into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to be changed from 0 to port value coming from migrate_get_socket_address. Add migrate_set_ports to address this change of port value. Patch 7-8: - Add 2 negative tests to validate mutually exhaustive behaviour of migration QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the initial entry point for migration QAPIs. v4->v5 Changelog: 1. Remove redundant imports from migration-test.c after moving helper functions to migration-helpers.c 2. call migrate_get_socket_address(to) and internally let qdict_get() call ???socket-address??? parameter to make more sense to the reader. 3. qdict needs to be freed, other small fixups. v3->v4 Changelog: 1. introduced migrate_get_connect_uri and migrate_get_connect_qdict to both used migrate_get_socket_address to get dest uri in socket- address, and then use SokcketAddress_to_qdict to convert it into qdict. 2. Misc code changes. v2->v3 Changelog: - 1. 'channels' introduction is not required now for migrate_qmp_incoming 2. Refactor the code into 7 different patches 3. 'channels' introduction is not required now for migrate_qmp_incoming 4. Remove custom function for converting string to MigrationChannelList 5. move calls for migrate_get_socket_address inside migrate_qmp so that migrate_set_ports can replace the QAPI's port with correct value. Het Gala (8): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Replace migrate_get_connect_uri inplace of migrate_get_socket_address Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to update migration port value Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 180 +--- 3 files changed, 257 insertions(+), 91 deletions(-) -- 2.22.3
[PATCH v5 4/8] Add channels parameter in migrate_qmp_fail
Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 13 +++--
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 4 ++--
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 8806dc841e..f215f44467 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -100,7 +100,8 @@ bool migrate_watch_for_events(QTestState *who, const char
*name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
@@ -110,7 +111,15 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
const char *fmt, ...)
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
err = qtest_qmp_assert_failure_ref(
who, "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e16a34c796..4e664148a5 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,8 +33,9 @@ G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...);
void migrate_set_capability(QTestState *who, const char *capability,
bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 9bb24fd7c5..da4b0006c7 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1717,7 +1717,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
@@ -1812,7 +1812,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
--
2.22.3
[PATCH v5 3/8] Replace migrate_get_connect_uri inplace of migrate_get_socket_address
Refactor migrate_get_socket_address to internally utilize 'socket-address'
parameter, reducing redundancy in the function definition.
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 29 +++--
1 file changed, 19 insertions(+), 10 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3e8c19c4de..8806dc841e 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -48,28 +48,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-static char *
-migrate_get_socket_address(QTestState *who, const char *parameter)
+static SocketAddress *migrate_get_socket_address(QTestState *who)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
+object = qdict_get(rsp, "socket-address");
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migrate_get_socket_address(who);
+connect_uri = SocketAddress_to_str(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_uri;
}
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -129,7 +138,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to);
}
qdict_put_str(args, "uri", uri ? uri : connect_uri);
--
2.22.3
Re: [PATCH v4 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
On 12/03/24 2:55 am, Peter Xu wrote: On Sat, Mar 09, 2024 at 01:11:45PM +0530, Het Gala wrote: Can find the reference to the githab pipeline (before patchset) : https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_galahet_Qemu_-2D_pipelines_1207185095=DwIBaQ=s883GpUCOChKOHiocYtGcg=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg=y2xUaOwvRVC5eTpFNEdxb37JYDdxN61W406HlCyx3CWIVyBRgLwjJhAYALZLinoi=vZRNX33_DuLO1TsfTpYR_s9bf_EMFm3oHHH_eg57zE0= Can find the reference to the githab pipeline (after patchset) : https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_galahet_Qemu_-2D_pipelines_1207183673=DwIBaQ=s883GpUCOChKOHiocYtGcg=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg=y2xUaOwvRVC5eTpFNEdxb37JYDdxN61W406HlCyx3CWIVyBRgLwjJhAYALZLinoi=C73ka3k3ouAuRJYNVLPIBQiWx3jDFDDvVYDiEYqfE04= Het, Please still copy me for any migration patches. In this case Fabiano is looking it'll be all fine, but it will still help me on marking the emails. Thanks, So sorry about that Peter. I am aware that you and Fabiano are the go to migration maintainers. I thought I emailed or cc'd all the stakeholders that should be involved for this patchset series. Even in earlier series of this patchset, you were cc'ed, but somehow I just forgot to cc you for this patchset. Sure, will take care from next time. Again apologies for the mixup :) Regards, Het Gala
Re: [PATCH v4 5/8] Add migrate_set_ports into migrate_qmp to update migration port value
On 12/03/24 12:12 am, Fabiano Rosas wrote:
Het Gala writes:
migrate_set_get_qdict gets qdict with the dst QEMU parameters
s/set_//
Ack
migrate_set_ports() from list of channels reads each QDict for port,
and fills the port with correct value in case it was 0 in the test.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 73 +
1 file changed, 73 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 91c8a817d2..7c17d78d6b 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+#include "include/qemu/cutils.h"
Extra "include/" here?
Ack
#include "migration-helpers.h"
@@ -49,6 +51,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
+static QDict *SocketAddress_to_qdict(SocketAddress *addr)
+{
+QDict *dict = qdict_new();
+
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+qdict_put_str(dict, "type", "inet");
+qdict_put_str(dict, "host", addr->u.inet.host);
+qdict_put_str(dict, "port", addr->u.inet.port);
+break;
+case SOCKET_ADDRESS_TYPE_UNIX:
+qdict_put_str(dict, "type", "unix");
+qdict_put_str(dict, "path", addr->u.q_unix.path);
+break;
+case SOCKET_ADDRESS_TYPE_FD:
+qdict_put_str(dict, "type", "fd");
+qdict_put_str(dict, "str", addr->u.fd.str);
+break;
+case SOCKET_ADDRESS_TYPE_VSOCK:
+qdict_put_str(dict, "type", "vsock");
+qdict_put_str(dict, "cid", addr->u.vsock.cid);
+qdict_put_str(dict, "port", addr->u.vsock.port);
+break;
+default:
+g_assert_not_reached();
+break;
+}
+
+return dict;
+}
+
static SocketAddress *
migrate_get_socket_address(QTestState *who, const char *parameter)
{
@@ -83,6 +116,44 @@ migrate_get_connect_uri(QTestState *who, const char
*parameter)
return connect_uri;
}
+static QDict *
+migrate_get_connect_qdict(QTestState *who, const char *parameter)
+{
+SocketAddress *addrs;
+QDict *connect_qdict;
+
+addrs = migrate_get_socket_address(who, parameter);
+connect_qdict = SocketAddress_to_qdict(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_qdict;
+}
+
+static void migrate_set_ports(QTestState *to, QList *channel_list)
+{
+QDict *addr;
+QListEntry *entry;
+g_autofree const char *addr_port = NULL;
+
+if (channel_list == NULL) {
+return;
+}
+
+addr = migrate_get_connect_qdict(to, "socket-address");
addr needs to be freed.
Ack. Thanks for pointing this out
+
+QLIST_FOREACH_ENTRY(channel_list, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QDict *addrdict = qdict_get_qdict(channel, "addr");
+
+if (qdict_haskey(addrdict, "port") &&
+qdict_haskey(addr, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+addr_port = qdict_get_str(addr, "port");
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -141,6 +212,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -151,6 +223,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_connect_uri(to, "socket-address");
}
+migrate_set_ports(to, channel_list);
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
Regards,
Het Gala
Re: [PATCH v4 3/8] Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 12/03/24 2:21 am, Fabiano Rosas wrote:
Het Gala writes:
On 11/03/24 11:49 pm, Fabiano Rosas wrote:
Het Gala writes:
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -130,7 +140,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to, "socket-address");
What's the point of the "socket-address" argument here? Seems a bit
nonsensical to me to call: migrate_get_socket_address(..., "socket-address").
What about we just suppress this throughout the stack and directly call:
object = qdict_get(rsp, "socket-address");
Fabiano, I didn't get clearly understand your point here. From what I
understand,
you want to call just
1. migrate_get_connect_uri(to) and migrate_get_connect_qdict(to)
Yes.
Ack
2. delete migrate_get_socket_address(..., "socket-address") altogether
No, just the string argument, not the whole function:
static char *migrate_get_socket_address(QTestState *who) <
{
QDict *rsp;
char *result;
SocketAddressList *addrs;
Visitor *iv = NULL;
QObject *object;
rsp = migrate_query(who);
object = qdict_get(rsp, "socket-address"); <-
...
}
If the thing is called migrate_get_SOCKET_ADDRESS(), it's obvious that
the "socket-address" is the parameter we want. We even call
SocketAddress_to_str, so there's no point in having that argument
there. We will never call the function with something else in
'parameter'.
Ahh, okay. I got your point, and yes, it makes sense. Will just call
migrate_get_socket_address(to) and let the qdict_get() call
"socket-address" internally.
Regards,
Het Gala
Re: [PATCH v4 3/8] Replace migrate_get_connect_uri inplace of migrate_get_socket_address
On 11/03/24 11:49 pm, Fabiano Rosas wrote:
Het Gala writes:
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -130,7 +140,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to, "socket-address");
What's the point of the "socket-address" argument here? Seems a bit
nonsensical to me to call: migrate_get_socket_address(..., "socket-address").
What about we just suppress this throughout the stack and directly call:
object = qdict_get(rsp, "socket-address");
Fabiano, I didn't get clearly understand your point here. From what I
understand,
you want to call just
1. migrate_get_connect_uri(to) and migrate_get_connect_qdict(to)
2. delete migrate_get_socket_address(..., "socket-address") altogether
3. Just call qdict_get(rsp, "socket-address") which will return an
object 4. Then convert this object into qdict and uri string respectively ?
Hmm, If that's the case, converting to qdict shouldn't be a problem. But
for uri string is there a simpler method or writing a parsing function
would be needed ?
Regards,
Het Gala
Re: [PATCH v4 2/8] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp
On 11/03/24 11:46 pm, Fabiano Rosas wrote: Het Gala writes: Move the calls to migrate_get_socket_address() into migrate_qmp(). Get rid of connect_uri and replace it with args->connect_uri only because 'to' object will help to generate connect_uri with the correct port number. Signed-off-by: Het Gala Suggested-by: Fabiano Rosas Reviewed-by: Fabiano Rosas --- tests/qtest/migration-helpers.c | 55 ++- tests/qtest/migration-test.c| 79 + 2 files changed, 64 insertions(+), 70 deletions(-) diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c index b6206a04fb..9af3c7d4d5 100644 --- a/tests/qtest/migration-helpers.c +++ b/tests/qtest/migration-helpers.c @@ -13,6 +13,10 @@ #include "qemu/osdep.h" #include "qemu/ctype.h" #include "qapi/qmp/qjson.h" +#include "qemu/sockets.h" +#include "qapi/qapi-visit-sockets.h" +#include "qapi/qobject-input-visitor.h" +#include "qapi/error.h" Are any of these now superfluous at migration-test.c? Yess, actually all of them are now redundant or not required from the migration-test.c. Will remove all imports from there Regards, Het Gala
Re: Problem with migration/rdma
On 11/03/24 8:16 pm, Peter Xu wrote: On Mon, Mar 11, 2024 at 08:00:06PM +0530, Het Gala wrote: Let me send a proper patch to qemu devel mailing list and cc all the people involved. I have reviewed and tested the change. Have tweaked the commit message accordingly. I hope that's okay with you Yu Zhang :) Het - don't worry, I've had it in my queue. Thanks, Okay. Thanks = From 694451b89b21b3b67c404cbcfa2b84e3afae0c5d Mon Sep 17 00:00:00 2001 From: Yu Zhang Date: Wed, 6 Mar 2024 09:06:54 +0100 Subject: [PATCH] migration/rdma: Fix a memory issue for migration In commit 3fa9642ff7 change was made to convert the RDMA backend to accept MigrateAddress struct. However, the assignment of "host" leads to data corruption on the target host and the failure of migration. isock->host = rdma->host; Regards, Het Gala
Re: Problem with migration/rdma
Let me send a proper patch to qemu devel mailing list and cc all the people involved. I have reviewed and tested the change. Have tweaked the commit message accordingly. I hope that's okay with you Yu Zhang :) On 11/03/24 4:44 pm, Yu Zhang wrote: Hello Peter and Zhijian, I created a MR in gitlab. You may have a look and let me know whether it's fine for you. https://urldefense.proofpoint.com/v2/url?u=https-3A__gitlab.com_qemu_qemu_-2D_merge-5Frequests_4=DwIFaQ=s883GpUCOChKOHiocYtGcg=-qwZZzrw4EKSsq0BK7MBd3wW1WEpXmJeng3ZUT5uBCg=BBbf8F-Z4S5Gg9uS4sGrM2VjND752LeFYFQa4wRG27sLihZCc3Pot0NzTBxor1IK=1GBQqozpModsL-VR_THJwca1SMdWBo8pznn1un5RsPo= Best regards, Yu Zhang @ IONOS Compute Platform 11.03.2024 On Fri, Mar 8, 2024 at 10:13 AM Yu Zhang wrote: Hallo Alexei, vielen Dank! Habe probiert, aber auch an Permission Problem gestoßen. Ich versuche, Google Application-specific password zu erstellen für Zukunft. QEMU Upstream bietet diese Lösung für kleinen Patch an im Notfall. Sie werden es behanden. Viele Grüße Yu On Fri, Mar 8, 2024 at 8:09 AM Alexei Pastuchov wrote: Regards, Het Gala
Re: [PATCH v4 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
Can find the reference to the githab pipeline (before patchset) : https://gitlab.com/galahet/Qemu/-/pipelines/1207185095 Can find the reference to the githab pipeline (after patchset) : https://gitlab.com/galahet/Qemu/-/pipelines/1207183673 On 09/03/24 2:29 am, Het Gala wrote: With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. Patch Summary: - Patch 1-2: - Introduce 'to' object inside migrate_qmp() so and move the calls to migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri with args->connect_uri everywhere. Patch 3-6: - Add channels argument to allow both migration QAPI arguments independently into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to be changed from 0 to port value coming from migrate_get_socket_address. Add migrate_set_ports to address this change of port value. Patch 7-8: - Add 2 negative tests to validate mutually exhaustive behaviour of migration QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the initial entry point for migration QAPIs. v3->v4 Changelog: 1. introduced migrate_get_connect_uri and migrate_get_connect_qdict to both used migrate_get_socket_address to get dest uri in socket- address, and then use SokcketAddress_to_qdict to convert it into qdict. 2. Misc code changes. v2->v3 Changelog: - 1. 'channels' introduction is not required now for migrate_qmp_incoming 2. Refactor the code into 7 different patches 3. 'channels' introduction is not required now for migrate_qmp_incoming 4. Remove custom function for converting string to MigrationChannelList 5. move calls for migrate_get_socket_address inside migrate_qmp so that migrate_set_ports can replace the QAPI's port with correct value. Het Gala (8): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Replace migrate_get_connect_uri inplace of migrate_get_socket_address Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to update migration port value Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 177 ++-- 3 files changed, 258 insertions(+), 87 deletions(-)
[PATCH v4 8/8] Add negative tests to validate migration QAPIs
Migration QAPI arguments - uri and channels are mutually exhaustive.
Add negative validation tests, one with both arguments present and
one with none present.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 54
1 file changed, 54 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 6ba3cfd1e4..385f696a3d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2612,6 +2612,56 @@ static void test_validate_uuid_dst_not_set(void)
do_test_validate_uuid(, false);
}
+static void do_test_validate_uri_channel(MigrateCommon *args)
+{
+QTestState *from, *to;
+g_autofree char *connect_uri = NULL;
+
+if (test_migrate_start(, , args->listen_uri, >start)) {
+return;
+}
+
+/* Wait for the first serial output from the source */
+wait_for_serial("src_serial");
+
+/*
+ * 'uri' and 'channels' validation is checked even before the migration
+ * starts.
+ */
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
+test_migrate_end(from, to, false);
+}
+
+static void test_validate_uri_channels_both_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+.connect_uri = "tcp:127.0.0.1:0",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+do_test_validate_uri_channel();
+}
+
+static void test_validate_uri_channels_none_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+};
+
+do_test_validate_uri_channel();
+}
+
/*
* The way auto_converge works, we need to do too many passes to
* run this test. Auto_converge logic is only run once every
@@ -3678,6 +3728,10 @@ int main(int argc, char **argv)
test_validate_uuid_src_not_set);
migration_test_add("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
+migration_test_add("/migration/validate_uri/channels/both_set",
+ test_validate_uri_channels_both_set);
+migration_test_add("/migration/validate_uri/channels/none_set",
+ test_validate_uri_channels_none_set);
/*
* See explanation why this test is slow on function definition
*/
--
2.22.3
[PATCH v4 6/8] Add channels parameter in migrate_qmp
Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 22 +-
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 29 insertions(+), 25 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 7c17d78d6b..bf9fd61035 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -135,10 +135,6 @@ static void migrate_set_ports(QTestState *to, QList
*channel_list)
QListEntry *entry;
g_autofree const char *addr_port = NULL;
-if (channel_list == NULL) {
-return;
-}
-
addr = migrate_get_connect_qdict(to, "socket-address");
QLIST_FOREACH_ENTRY(channel_list, entry) {
@@ -208,11 +204,10 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...)
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
-QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -220,11 +215,20 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-if (!uri) {
+if (uri) {
+qdict_put_str(args, "uri", uri);
+} else if (!channels) {
connect_uri = migrate_get_connect_uri(to, "socket-address");
+qdict_put_str(args, "uri", connect_uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+QList *channel_list = qobject_to(QList, channels_obj);
+migrate_set_ports(to, channel_list);
+qdict_put_obj(args, "channels", channels_obj);
}
-migrate_set_ports(to, channel_list);
-qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 4e664148a5..1339835698 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,9 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(4, 5)
+G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 61aa53c3f7..b1e5660dbf 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1305,7 +1305,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, "{}");
+migrate_qmp(from, to, NULL, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1455,7 +1455,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", NULL, "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1543,7 +1543,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, to, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, NULL, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1624,7 +1624,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1663,7 +1663,7 @@ static void test_analyze_scrip
[PATCH v4 5/8] Add migrate_set_ports into migrate_qmp to update migration port value
migrate_set_get_qdict gets qdict with the dst QEMU parameters
migrate_set_ports() from list of channels reads each QDict for port,
and fills the port with correct value in case it was 0 in the test.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 73 +
1 file changed, 73 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 91c8a817d2..7c17d78d6b 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+#include "include/qemu/cutils.h"
#include "migration-helpers.h"
@@ -49,6 +51,37 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
+static QDict *SocketAddress_to_qdict(SocketAddress *addr)
+{
+QDict *dict = qdict_new();
+
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+qdict_put_str(dict, "type", "inet");
+qdict_put_str(dict, "host", addr->u.inet.host);
+qdict_put_str(dict, "port", addr->u.inet.port);
+break;
+case SOCKET_ADDRESS_TYPE_UNIX:
+qdict_put_str(dict, "type", "unix");
+qdict_put_str(dict, "path", addr->u.q_unix.path);
+break;
+case SOCKET_ADDRESS_TYPE_FD:
+qdict_put_str(dict, "type", "fd");
+qdict_put_str(dict, "str", addr->u.fd.str);
+break;
+case SOCKET_ADDRESS_TYPE_VSOCK:
+qdict_put_str(dict, "type", "vsock");
+qdict_put_str(dict, "cid", addr->u.vsock.cid);
+qdict_put_str(dict, "port", addr->u.vsock.port);
+break;
+default:
+g_assert_not_reached();
+break;
+}
+
+return dict;
+}
+
static SocketAddress *
migrate_get_socket_address(QTestState *who, const char *parameter)
{
@@ -83,6 +116,44 @@ migrate_get_connect_uri(QTestState *who, const char
*parameter)
return connect_uri;
}
+static QDict *
+migrate_get_connect_qdict(QTestState *who, const char *parameter)
+{
+SocketAddress *addrs;
+QDict *connect_qdict;
+
+addrs = migrate_get_socket_address(who, parameter);
+connect_qdict = SocketAddress_to_qdict(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_qdict;
+}
+
+static void migrate_set_ports(QTestState *to, QList *channel_list)
+{
+QDict *addr;
+QListEntry *entry;
+g_autofree const char *addr_port = NULL;
+
+if (channel_list == NULL) {
+return;
+}
+
+addr = migrate_get_connect_qdict(to, "socket-address");
+
+QLIST_FOREACH_ENTRY(channel_list, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QDict *addrdict = qdict_get_qdict(channel, "addr");
+
+if (qdict_haskey(addrdict, "port") &&
+qdict_haskey(addr, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+addr_port = qdict_get_str(addr, "port");
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -141,6 +212,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+QList *channel_list = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -151,6 +223,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_connect_uri(to, "socket-address");
}
+migrate_set_ports(to, channel_list);
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
--
2.22.3
[PATCH v4 4/8] Add channels parameter in migrate_qmp_fail
Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 13 +++--
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 4 ++--
3 files changed, 16 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 3c3fe9d8aa..91c8a817d2 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -102,7 +102,8 @@ bool migrate_watch_for_events(QTestState *who, const char
*name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
@@ -112,7 +113,15 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
const char *fmt, ...)
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+QObject *channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
err = qtest_qmp_assert_failure_ref(
who, "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e16a34c796..4e664148a5 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,8 +33,9 @@ G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...);
void migrate_set_capability(QTestState *who, const char *capability,
bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 01255e7e7e..61aa53c3f7 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1721,7 +1721,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
@@ -1816,7 +1816,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
--
2.22.3
[PATCH v4 7/8] Add multifd_tcp_plain test using list of channels instead of uri
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 30 +++---
1 file changed, 27 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index b1e5660dbf..6ba3cfd1e4 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -659,6 +659,13 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: JSON-formatted list of src QEMU URIs. If a port is
+ * defined as '0' in any QDict key a value of '0' will be
+ * automatically converted to the correct destination port.
+ */
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2744,7 +2751,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer",
@@ -2759,6 +2766,21 @@ static void test_multifd_tcp_none(void)
test_precopy_common();
}
+static void test_multifd_tcp_channels_none(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3668,8 +3690,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
migration_test_add("/migration/multifd/tcp/plain/cancel",
test_multifd_tcp_cancel);
migration_test_add("/migration/multifd/tcp/plain/zlib",
--
2.22.3
[PATCH v4 2/8] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp
Move the calls to migrate_get_socket_address() into migrate_qmp().
Get rid of connect_uri and replace it with args->connect_uri only
because 'to' object will help to generate connect_uri with the
correct port number.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 55 ++-
tests/qtest/migration-test.c| 79 +
2 files changed, 64 insertions(+), 70 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..9af3c7d4d5 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,10 @@
#include "qemu/osdep.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
+#include "qemu/sockets.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
#include "migration-helpers.h"
@@ -24,6 +28,51 @@
*/
#define MIGRATION_STATUS_WAIT_TIMEOUT 120
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.inet.host,
+ addr->u.inet.port);
+case SOCKET_ADDRESS_TYPE_UNIX:
+return g_strdup_printf("unix:%s",
+ addr->u.q_unix.path);
+case SOCKET_ADDRESS_TYPE_FD:
+return g_strdup_printf("fd:%s", addr->u.fd.str);
+case SOCKET_ADDRESS_TYPE_VSOCK:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.vsock.cid,
+ addr->u.vsock.port);
+default:
+return g_strdup("unknown address type");
+}
+}
+
+static char *
+migrate_get_socket_address(QTestState *who, const char *parameter)
+{
+QDict *rsp;
+char *result;
+SocketAddressList *addrs;
+Visitor *iv = NULL;
+QObject *object;
+
+rsp = migrate_query(who);
+object = qdict_get(rsp, parameter);
+
+iv = qobject_input_visitor_new(object);
+visit_type_SocketAddressList(iv, NULL, , _abort);
+visit_free(iv);
+
+/* we are only using a single address */
+result = SocketAddress_to_str(addrs->value);
+
+qapi_free_SocketAddressList(addrs);
+qobject_unref(rsp);
+return result;
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -73,13 +122,17 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (!uri) {
+connect_uri = migrate_get_socket_address(to, "socket-address");
+}
+qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index d9b4e28c12..01255e7e7e 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -369,50 +369,6 @@ static void cleanup(const char *filename)
unlink(path);
}
-static char *SocketAddress_to_str(SocketAddress *addr)
-{
-switch (addr->type) {
-case SOCKET_ADDRESS_TYPE_INET:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.inet.host,
- addr->u.inet.port);
-case SOCKET_ADDRESS_TYPE_UNIX:
-return g_strdup_printf("unix:%s",
- addr->u.q_unix.path);
-case SOCKET_ADDRESS_TYPE_FD:
-return g_strdup_printf("fd:%s", addr->u.fd.str);
-case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.vsock.cid,
- addr->u.vsock.port);
-default:
-return g_strdup("unknown address type");
-}
-}
-
-static char *migrate_get_socket_address(QTestState *who, const char *parameter)
-{
-QDict *rsp;
-char *result;
-SocketAddressList *addrs;
-Visitor *iv = NULL;
-QObject *object;
-
-rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
-
-iv = qobject_input_visitor_new(object);
-visit_type_SocketAddressList(iv, NULL, , _abort);
-visit_free(iv);
-
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
-qobject_unref(rsp);
-return result;
-}
-
[PATCH v4 3/8] Replace migrate_get_connect_uri inplace of migrate_get_socket_address
migrate_get_socket_address implicitly converts SocketAddress into str.
Move migrate_get_socket_address inside migrate_get_connect_uri which
should return the uri string instead.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 26 ++
1 file changed, 18 insertions(+), 8 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 9af3c7d4d5..3c3fe9d8aa 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -49,12 +49,12 @@ static char *SocketAddress_to_str(SocketAddress *addr)
}
}
-static char *
+static SocketAddress *
migrate_get_socket_address(QTestState *who, const char *parameter)
{
QDict *rsp;
-char *result;
SocketAddressList *addrs;
+SocketAddress *addr;
Visitor *iv = NULL;
QObject *object;
@@ -63,14 +63,24 @@ migrate_get_socket_address(QTestState *who, const char
*parameter)
iv = qobject_input_visitor_new(object);
visit_type_SocketAddressList(iv, NULL, , _abort);
+addr = addrs->value;
visit_free(iv);
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
qobject_unref(rsp);
-return result;
+return addr;
+}
+
+static char *
+migrate_get_connect_uri(QTestState *who, const char *parameter)
+{
+SocketAddress *addrs;
+char *connect_uri;
+
+addrs = migrate_get_socket_address(who, parameter);
+connect_uri = SocketAddress_to_str(addrs);
+
+qapi_free_SocketAddress(addrs);
+return connect_uri;
}
bool migrate_watch_for_events(QTestState *who, const char *name,
@@ -130,7 +140,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
g_assert(!qdict_haskey(args, "uri"));
if (!uri) {
-connect_uri = migrate_get_socket_address(to, "socket-address");
+connect_uri = migrate_get_connect_uri(to, "socket-address");
}
qdict_put_str(args, "uri", uri ? uri : connect_uri);
--
2.22.3
[PATCH v4 1/8] Add 'to' object into migrate_qmp()
Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() inside migrate_qmp() to get
the port value. This is not applied to other migrate_qmp*
because they don't need the port.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
Reviewed-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 3 ++-
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..b6206a04fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -68,7 +68,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const
char *fmt, ...)
* Arguments are built from @fmt... (formatted like
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...)
{
va_list ap;
QDict *args;
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..e16a34c796 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,8 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 4023d808f9..d9b4e28c12 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1350,7 +1350,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_suspend(from, _state);
g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1500,7 +1500,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1588,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1669,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1708,7 +1708,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
wait_for_migration_complete(from);
pid = fork();
@@ -1777,7 +1777,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1873,7 +1873,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
wait_for_migration_complete(from);
/*
@@ -2029,7 +2029,7 @@ static void test_ignore_shared(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2605,7 +2605,7 @@ static void do_test_validate_uuid(MigrateStart *args,
bool should_fail)
/* Wait for the first serial output from the source */
wait_for
[PATCH v4 0/8] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. Patch Summary: - Patch 1-2: - Introduce 'to' object inside migrate_qmp() so and move the calls to migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri with args->connect_uri everywhere. Patch 3-6: - Add channels argument to allow both migration QAPI arguments independently into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to be changed from 0 to port value coming from migrate_get_socket_address. Add migrate_set_ports to address this change of port value. Patch 7-8: - Add 2 negative tests to validate mutually exhaustive behaviour of migration QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the initial entry point for migration QAPIs. v3->v4 Changelog: 1. introduced migrate_get_connect_uri and migrate_get_connect_qdict to both used migrate_get_socket_address to get dest uri in socket- address, and then use SokcketAddress_to_qdict to convert it into qdict. 2. Misc code changes. v2->v3 Changelog: - 1. 'channels' introduction is not required now for migrate_qmp_incoming 2. Refactor the code into 7 different patches 3. 'channels' introduction is not required now for migrate_qmp_incoming 4. Remove custom function for converting string to MigrationChannelList 5. move calls for migrate_get_socket_address inside migrate_qmp so that migrate_set_ports can replace the QAPI's port with correct value. Het Gala (8): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Replace migrate_get_connect_uri inplace of migrate_get_socket_address Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to update migration port value Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 158 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 177 ++-- 3 files changed, 258 insertions(+), 87 deletions(-) -- 2.22.3
Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number
On 06/03/24 9:31 pm, Fabiano Rosas wrote:
Het Gala writes:
On 06/03/24 8:06 pm, Fabiano Rosas wrote:
Het Gala writes:
Add a migrate_set_ports() function that from each QDict, fills in
the port in case it was 0 in the test.
Handle a list of channels so we can add a negative test that
passes more than one channel.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 478c1f259b..df4978bf17 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+
Extra line here. This is unwanted because it sometimes trips git into
thinking there's a conflict here when another patch changes the
surrounding lines.
Ack, that makes sense
#include "migration-helpers.h"
@@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
return result;
}
+static void migrate_set_ports(QTestState *to, QList *channelList)
+{
+g_autofree char *addr = NULL;
+g_autofree char *addr_port = NULL;
+QListEntry *entry;
+
+addr = migrate_get_socket_address(to, "socket-address");
+addr_port = g_strsplit(addr, ":", 3)[2];
Will this always do the right thing when the src/dst use different types
of channels? If there is some kind of mismatch (say one side uses vsock
and the other inet), it's better that this function doesn't touch the
channels dict instead of putting garbage in the port field.
Yes you are right. This will fail if there is a mismatch in type of
channels.
Better idea would be to check if 'port' key is present in both, i.e. in
'addr'
as well as 'addrdict' and only then change the port ?
Yep, either parse the type from string or add a version of
migrate_get_socket_address that returns a dict. Then check if type
matches and port exists.
one silly question here, why are we not having tests for exec and rdma
specifically ?
Another suggestion required: Parsing uri to qdict is easy to implement
but (little)
messy codewise, and the other hand migrate_get_qdict looks clean, but
under the hood we would convert it to socketaddress and then call
SocketAddress_to_qdict. Which one we can prefer more here ?
Regards,
Het Gala
Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number
On 06/03/24 9:31 pm, Fabiano Rosas wrote:
Het Gala writes:
On 06/03/24 8:06 pm, Fabiano Rosas wrote:
Het Gala writes:
Add a migrate_set_ports() function that from each QDict, fills in
the port in case it was 0 in the test.
Handle a list of channels so we can add a negative test that
passes more than one channel.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 478c1f259b..df4978bf17 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+
Extra line here. This is unwanted because it sometimes trips git into
thinking there's a conflict here when another patch changes the
surrounding lines.
Ack, that makes sense
#include "migration-helpers.h"
@@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
return result;
}
+static void migrate_set_ports(QTestState *to, QList *channelList)
+{
+g_autofree char *addr = NULL;
+g_autofree char *addr_port = NULL;
+QListEntry *entry;
+
+addr = migrate_get_socket_address(to, "socket-address");
+addr_port = g_strsplit(addr, ":", 3)[2];
Will this always do the right thing when the src/dst use different types
of channels? If there is some kind of mismatch (say one side uses vsock
and the other inet), it's better that this function doesn't touch the
channels dict instead of putting garbage in the port field.
Yes you are right. This will fail if there is a mismatch in type of
channels.
Better idea would be to check if 'port' key is present in both, i.e. in
'addr'
as well as 'addrdict' and only then change the port ?
Yep, either parse the type from string or add a version of
migrate_get_socket_address that returns a dict. Then check if type
matches and port exists.
Also what happens if the dst is using unix: or fd:?
yes in that case - how should the migration behaviour be ? src and dst
should be of the same type right
Remember this is test code. If the test was written incorrectly either
by developer mistake or on purpose to test some condition, then it's not
this function's reponsibility to fix that.
Replace the port only if the transport type allows for a port, there is
a port in both addr and addrdict and port is 0. Anything else, leave the
channelList untouched and let QEMU deal with the bad input.
+
+QLIST_FOREACH_ENTRY(channelList, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QObject *addr_obj = qdict_get(channel, "addr");
+
+if (qobject_type(addr_obj) == QTYPE_QDICT) {
+QDict *addrdict = qobject_to(QDict, addr_obj);
You might not need these two lines if at the start you use:
QDict *addr = qdict_get_dict(channel, "addr");
If you are commenting regarding this two lines:
+if (qobject_type(addr_obj) == QTYPE_QDICT) {
+QDict *addrdict = qobject_to(QDict, addr_obj);
then, I am not sure, because addrdict and addr is different right? Also
addrdict can also
be a QList, like in the case of exec migration, it can be a list instead of dict
ex:
# -> { "execute": "migrate",
# "arguments": {
# "channels": [ { "channel-type": "main",
# "addr": { "transport": "exec",
#"args": [ "/bin/nc", "-p", "6000",
# "/some/sock" ] } } ] } }
"addr" is always a dict, no? Even in the example you just gave.
Sorry, my apologies. I had args <-> addrdict in back of my mind.
You are correct. Will make the changes in next patchset.
+if (qdict_haskey(addrdict, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+}
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_socket_address(to, "socket-address");
}
+migrate_set_ports(to, NULL);
migrate_set_ports is not prepared to take NULL. This breaks the tests in
this commit. All individual commits should work, otherwise it will break
bisecting.
Okay, so in that case, is it better to merge this with the next patch ?
because if I just
define the migrate_set_ports function and not use it anywhere, it gives
a warning/error
(depending upon what compiler is used)
You can return early at migrate_set_ports if channelList is NULL.
Also, I just noticed: s/channelList/channel_list/
Ack. Thanks
Regards,
Het Gala
Re: [PATCH v3 6/7] Add multifd_tcp_plain test using list of channels instead of uri
On 06/03/24 8:37 pm, Fabiano Rosas wrote:
Het Gala writes:
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 29 ++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f94fe713b2..05e5f3ebe5 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -659,6 +659,12 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: the JSON formatted list of URIs for the src
+ * QEMU to connect to
+ */
You could add some words here mentioning that a port of '0' will be
automatically converted to the correct port that the destination is
using.
Ack, will add these while defining connect_channels.
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2623,7 +2629,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer", @@ -2638,6 +2644,21 @@ static void test_multifd_tcp_none(void)
test_precopy_common(); } +static void
test_multifd_tcp_channels_none(void) +{ + MigrateCommon args = { +
.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3531,8 +3552,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
We should eventually make a pass to standardize/simplify these
strings. We could have a little "grammar" defined on how to construct
them.
//
test-type :: migrate | validate
migration-mode :: multifd | precopy | postcopy
transport :: tcp | fd | unix | file
invocation :: uri | channels
compression:: zlib | zstd | none
encryption :: tls | plain
Anyway, work for the future.
Yes, completely agree with you. It makes it much easier for people to
identify and define every test.
I can take this up as a separate patchset after this one gets merged maybe
Regards,
Het Gala
Re: [PATCH v3 5/7] Add channels parameter in migrate_qmp
On 06/03/24 8:12 pm, Fabiano Rosas wrote:
Het Gala writes:
Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 20 +++-
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index df4978bf17..0b891351a5 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -19,7 +19,6 @@
#include "qapi/error.h"
#include "qapi/qmp/qlist.h"
-
#include "migration-helpers.h"
/*
@@ -154,10 +153,12 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...)
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
+Error *error_abort = NULL;
Remove this.
Ack.
+QObject *channels_obj = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
Regards,
Het Gala
Re: [PATCH v3 3/7] Add channels parameter in migrate_qmp_fail
On 06/03/24 8:10 pm, Fabiano Rosas wrote:
Het Gala writes:
Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 15 +--
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 4 ++--
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 9af3c7d4d5..478c1f259b 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -92,17 +92,28 @@ bool migrate_watch_for_events(QTestState *who, const char
*name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
+Error *error_abort = NULL;
The error_abort needs to be the one defined in error.c. Just remove this
line.
Ack.
Regards
Het Gala
Re: [PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number
On 06/03/24 8:06 pm, Fabiano Rosas wrote:
Het Gala writes:
Add a migrate_set_ports() function that from each QDict, fills in
the port in case it was 0 in the test.
Handle a list of channels so we can add a negative test that
passes more than one channel.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 478c1f259b..df4978bf17 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+
Extra line here. This is unwanted because it sometimes trips git into
thinking there's a conflict here when another patch changes the
surrounding lines.
Ack, that makes sense
#include "migration-helpers.h"
@@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char *parameter)
return result;
}
+static void migrate_set_ports(QTestState *to, QList *channelList)
+{
+g_autofree char *addr = NULL;
+g_autofree char *addr_port = NULL;
+QListEntry *entry;
+
+addr = migrate_get_socket_address(to, "socket-address");
+addr_port = g_strsplit(addr, ":", 3)[2];
Will this always do the right thing when the src/dst use different types
of channels? If there is some kind of mismatch (say one side uses vsock
and the other inet), it's better that this function doesn't touch the
channels dict instead of putting garbage in the port field.
Yes you are right. This will fail if there is a mismatch in type of
channels.
Better idea would be to check if 'port' key is present in both, i.e. in
'addr'
as well as 'addrdict' and only then change the port ?
Also what happens if the dst is using unix: or fd:?
yes in that case - how should the migration behaviour be ? src and dst
should be of the same type right
+
+QLIST_FOREACH_ENTRY(channelList, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QObject *addr_obj = qdict_get(channel, "addr");
+
+if (qobject_type(addr_obj) == QTYPE_QDICT) {
+QDict *addrdict = qobject_to(QDict, addr_obj);
You might not need these two lines if at the start you use:
QDict *addr = qdict_get_dict(channel, "addr");
If you are commenting regarding this two lines:
+if (qobject_type(addr_obj) == QTYPE_QDICT) {
+QDict *addrdict = qobject_to(QDict, addr_obj);
then, I am not sure, because addrdict and addr is different right? Also
addrdict can also
be a QList, like in the case of exec migration, it can be a list instead of dict
ex:
# -> { "execute": "migrate",
# "arguments": {
# "channels": [ { "channel-type": "main",
# "addr": { "transport": "exec",
#"args": [ "/bin/nc", "-p", "6000",
# "/some/sock" ] } } ] } }
+if (qdict_haskey(addrdict, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+}
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_socket_address(to, "socket-address");
}
+migrate_set_ports(to, NULL);
migrate_set_ports is not prepared to take NULL. This breaks the tests in
this commit. All individual commits should work, otherwise it will break
bisecting.
Okay, so in that case, is it better to merge this with the next patch ?
because if I just
define the migrate_set_ports function and not use it anywhere, it gives
a warning/error
(depending upon what compiler is used)
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
Regards,
Het Gala
Re: [PATCH v3 0/7] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
On 06/03/24 4:19 pm, Het Gala wrote: Can also find the successful build here: https://gitlab.com/galahet/Qemu/-/pipelines/1201488612 Het Gala (7): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to change migration port number Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 109 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 176 ++-- 3 files changed, 208 insertions(+), 87 deletions(-) Regards, Het Gala
[PATCH v3 7/7] Add negative tests to validate migration QAPIs
Migration QAPI arguments - uri and channels are mutually exhaustive.
Add negative validation tests, one with both arguments present and
one with none present.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 54
1 file changed, 54 insertions(+)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 05e5f3ebe5..1317f87b22 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -2500,6 +2500,56 @@ static void test_validate_uuid_dst_not_set(void)
do_test_validate_uuid(, false);
}
+static void do_test_validate_uri_channel(MigrateCommon *args)
+{
+QTestState *from, *to;
+g_autofree char *connect_uri = NULL;
+
+if (test_migrate_start(, , args->listen_uri, >start)) {
+return;
+}
+
+/* Wait for the first serial output from the source */
+wait_for_serial("src_serial");
+
+/*
+ * 'uri' and 'channels' validation is checked even before the migration
+ * starts.
+ */
+migrate_qmp_fail(from, args->connect_uri, args->connect_channels, "{}");
+test_migrate_end(from, to, false);
+}
+
+static void test_validate_uri_channels_both_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+.connect_uri = "tcp:127.0.0.1:0",
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+
+do_test_validate_uri_channel();
+}
+
+static void test_validate_uri_channels_none_set(void)
+{
+MigrateCommon args = {
+.start = {
+.hide_stderr = true,
+},
+.listen_uri = "defer",
+};
+
+do_test_validate_uri_channel();
+}
+
/*
* The way auto_converge works, we need to do too many passes to
* run this test. Auto_converge logic is only run once every
@@ -3540,6 +3590,10 @@ int main(int argc, char **argv)
test_validate_uuid_src_not_set);
migration_test_add("/migration/validate_uuid_dst_not_set",
test_validate_uuid_dst_not_set);
+migration_test_add("/migration/validate_uri/channels/both_set",
+ test_validate_uri_channels_both_set);
+migration_test_add("/migration/validate_uri/channels/none_set",
+ test_validate_uri_channels_none_set);
/*
* See explanation why this test is slow on function definition
*/
--
2.22.3
[PATCH v3 3/7] Add channels parameter in migrate_qmp_fail
Alter migrate_qmp_fail() to allow both uri and channels
independently. For channels, convert string to a Dict.
No dealing with migrate_get_socket_address() here because
we will fail before starting the migration anyway.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 15 +--
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 4 ++--
3 files changed, 18 insertions(+), 6 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 9af3c7d4d5..478c1f259b 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -92,17 +92,28 @@ bool migrate_watch_for_events(QTestState *who, const char
*name,
return false;
}
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args, *err;
+Error *error_abort = NULL;
+QObject *channels_obj = NULL;
va_start(ap, fmt);
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (uri) {
+qdict_put_str(args, "uri", uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+channels_obj = qobject_from_json(channels, _abort);
+qdict_put_obj(args, "channels", channels_obj);
+}
err = qtest_qmp_assert_failure_ref(
who, "{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index e16a34c796..4e664148a5 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -33,8 +33,9 @@ G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
const char *fmt, ...);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp_fail(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp_fail(QTestState *who, const char *uri,
+ const char *channels, const char *fmt, ...);
void migrate_set_capability(QTestState *who, const char *capability,
bool value);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 20b1dd031a..19bb93cb7d 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1721,7 +1721,7 @@ static void test_precopy_common(MigrateCommon *args)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
@@ -1816,7 +1816,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
}
if (args->result == MIG_TEST_QMP_ERROR) {
-migrate_qmp_fail(from, args->connect_uri, "{}");
+migrate_qmp_fail(from, args->connect_uri, NULL, "{}");
goto finish;
}
--
2.22.3
[PATCH v3 5/7] Add channels parameter in migrate_qmp
Alter migrate_qmp() to allow use of channels parameter, but only
fill the uri with correct port number if there are no channels.
Here we don't want to allow the wrong cases of having both or
none (ex: migrate_qmp_fail).
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 20 +++-
tests/qtest/migration-helpers.h | 4 ++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 31 insertions(+), 21 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index df4978bf17..0b891351a5 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -19,7 +19,6 @@
#include "qapi/error.h"
#include "qapi/qmp/qlist.h"
-
#include "migration-helpers.h"
/*
@@ -154,10 +153,12 @@ void migrate_qmp_fail(QTestState *who, const char *uri,
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...)
+ const char *channels, const char *fmt, ...)
{
va_list ap;
QDict *args;
+Error *error_abort = NULL;
+QObject *channels_obj = NULL;
g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
@@ -165,11 +166,20 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-if (!uri) {
+if (uri) {
+qdict_put_str(args, "uri", uri);
+} else if (!channels) {
connect_uri = migrate_get_socket_address(to, "socket-address");
+qdict_put_str(args, "uri", connect_uri);
+}
+
+g_assert(!qdict_haskey(args, "channels"));
+if (channels) {
+channels_obj = qobject_from_json(channels, _abort);
+QList *channelList = qobject_to(QList, channels_obj);
+migrate_set_ports(to, channelList);
+qdict_put_obj(args, "channels", channels_obj);
}
-migrate_set_ports(to, NULL);
-qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 4e664148a5..1339835698 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,9 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(4, 5)
+G_GNUC_PRINTF(5, 6)
void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
- const char *fmt, ...);
+ const char *channels, const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 19bb93cb7d..f94fe713b2 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1305,7 +1305,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_serial("src_serial");
wait_for_suspend(from, _state);
-migrate_qmp(from, to, NULL, "{}");
+migrate_qmp(from, to, NULL, NULL, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1455,7 +1455,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", NULL, "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1543,7 +1543,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, to, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, NULL, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1624,7 +1624,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", NULL, "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1663,7 +1663,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
-migrate_qmp(from, to, uri, "{}");
+migrate
[PATCH v3 4/7] Add migrate_set_ports into migrate_qmp to change migration port number
Add a migrate_set_ports() function that from each QDict, fills in
the port in case it was 0 in the test.
Handle a list of channels so we can add a negative test that
passes more than one channel.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 26 ++
1 file changed, 26 insertions(+)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index 478c1f259b..df4978bf17 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -17,6 +17,8 @@
#include "qapi/qapi-visit-sockets.h"
#include "qapi/qobject-input-visitor.h"
#include "qapi/error.h"
+#include "qapi/qmp/qlist.h"
+
#include "migration-helpers.h"
@@ -73,6 +75,29 @@ migrate_get_socket_address(QTestState *who, const char
*parameter)
return result;
}
+static void migrate_set_ports(QTestState *to, QList *channelList)
+{
+g_autofree char *addr = NULL;
+g_autofree char *addr_port = NULL;
+QListEntry *entry;
+
+addr = migrate_get_socket_address(to, "socket-address");
+addr_port = g_strsplit(addr, ":", 3)[2];
+
+QLIST_FOREACH_ENTRY(channelList, entry) {
+QDict *channel = qobject_to(QDict, qlist_entry_obj(entry));
+QObject *addr_obj = qdict_get(channel, "addr");
+
+if (qobject_type(addr_obj) == QTYPE_QDICT) {
+QDict *addrdict = qobject_to(QDict, addr_obj);
+if (qdict_haskey(addrdict, "port") &&
+(strcmp(qdict_get_str(addrdict, "port"), "0") == 0)) {
+qdict_put_str(addrdict, "port", addr_port);
+}
+}
+}
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -143,6 +168,7 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
if (!uri) {
connect_uri = migrate_get_socket_address(to, "socket-address");
}
+migrate_set_ports(to, NULL);
qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
--
2.22.3
[PATCH v3 1/7] Add 'to' object into migrate_qmp()
Add the 'to' object into migrate_qmp(), so we can use
migrate_get_socket_address() inside migrate_qmp() to get
the port value. This is not applied to other migrate_qmp*
because they don't need the port.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 3 ++-
tests/qtest/migration-helpers.h | 5 +++--
tests/qtest/migration-test.c| 28 ++--
3 files changed, 19 insertions(+), 17 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index e451dbdbed..b6206a04fb 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -68,7 +68,8 @@ void migrate_qmp_fail(QTestState *who, const char *uri, const
char *fmt, ...)
* Arguments are built from @fmt... (formatted like
* qobject_from_jsonf_nofail()) with "uri": @uri spliced in.
*/
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...)
{
va_list ap;
QDict *args;
diff --git a/tests/qtest/migration-helpers.h b/tests/qtest/migration-helpers.h
index 3bf7ded1b9..e16a34c796 100644
--- a/tests/qtest/migration-helpers.h
+++ b/tests/qtest/migration-helpers.h
@@ -25,8 +25,9 @@ typedef struct QTestMigrationState {
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque);
-G_GNUC_PRINTF(3, 4)
-void migrate_qmp(QTestState *who, const char *uri, const char *fmt, ...);
+G_GNUC_PRINTF(4, 5)
+void migrate_qmp(QTestState *who, QTestState *to, const char *uri,
+ const char *fmt, ...);
G_GNUC_PRINTF(3, 4)
void migrate_incoming_qmp(QTestState *who, const char *uri,
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index 83512bce85..f645ab26f2 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -1350,7 +1350,7 @@ static int migrate_postcopy_prepare(QTestState **from_ptr,
wait_for_suspend(from, _state);
g_autofree char *uri = migrate_get_socket_address(to, "socket-address");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -1500,7 +1500,7 @@ static void postcopy_recover_fail(QTestState *from,
QTestState *to)
g_assert_cmpint(ret, ==, 1);
migrate_recover(to, "fd:fd-mig");
-migrate_qmp(from, "fd:fd-mig", "{'resume': true}");
+migrate_qmp(from, to, "fd:fd-mig", "{'resume': true}");
/*
* Make sure both QEMU instances will go into RECOVER stage, then test
@@ -1588,7 +1588,7 @@ static void test_postcopy_recovery_common(MigrateCommon
*args)
* Try to rebuild the migration channel using the resume flag and
* the newly created channel
*/
-migrate_qmp(from, uri, "{'resume': true}");
+migrate_qmp(from, to, uri, "{'resume': true}");
/* Restore the postcopy bandwidth to unlimited */
migrate_set_parameter_int(from, "max-postcopy-bandwidth", 0);
@@ -1669,7 +1669,7 @@ static void test_baddest(void)
if (test_migrate_start(, , "tcp:127.0.0.1:0", )) {
return;
}
-migrate_qmp(from, "tcp:127.0.0.1:0", "{}");
+migrate_qmp(from, to, "tcp:127.0.0.1:0", "{}");
wait_for_migration_fail(from, false);
test_migrate_end(from, to, false);
}
@@ -1708,7 +1708,7 @@ static void test_analyze_script(void)
uri = g_strdup_printf("exec:cat > %s", file);
migrate_ensure_converge(from);
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
wait_for_migration_complete(from);
pid = fork();
@@ -1777,7 +1777,7 @@ static void test_precopy_common(MigrateCommon *args)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
if (args->result != MIG_TEST_SUCCEED) {
bool allow_active = args->result == MIG_TEST_FAIL;
@@ -1873,7 +1873,7 @@ static void test_file_common(MigrateCommon *args, bool
stop_src)
goto finish;
}
-migrate_qmp(from, connect_uri, "{}");
+migrate_qmp(from, to, connect_uri, "{}");
wait_for_migration_complete(from);
/*
@@ -2029,7 +2029,7 @@ static void test_ignore_shared(void)
/* Wait for the first serial output from the source */
wait_for_serial("src_serial");
-migrate_qmp(from, uri, "{}");
+migrate_qmp(from, to, uri, "{}");
migrate_wait_for_dirty_mem(from, to);
@@ -2494,7 +2494,7 @@ static void do_test_validate_uuid(MigrateStart *args,
bool should_fail)
/* Wait for the first serial output from the source */
wait_for_serial("src_seria
[PATCH v3 2/7] Replace connect_uri and move migrate_get_socket_address inside migrate_qmp
Move the calls to migrate_get_socket_address() into migrate_qmp().
Get rid of connect_uri and replace it with args->connect_uri only
because 'to' object will help to generate connect_uri with the
correct port number.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-helpers.c | 55 ++-
tests/qtest/migration-test.c| 79 +
2 files changed, 64 insertions(+), 70 deletions(-)
diff --git a/tests/qtest/migration-helpers.c b/tests/qtest/migration-helpers.c
index b6206a04fb..9af3c7d4d5 100644
--- a/tests/qtest/migration-helpers.c
+++ b/tests/qtest/migration-helpers.c
@@ -13,6 +13,10 @@
#include "qemu/osdep.h"
#include "qemu/ctype.h"
#include "qapi/qmp/qjson.h"
+#include "qemu/sockets.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/error.h"
#include "migration-helpers.h"
@@ -24,6 +28,51 @@
*/
#define MIGRATION_STATUS_WAIT_TIMEOUT 120
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+switch (addr->type) {
+case SOCKET_ADDRESS_TYPE_INET:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.inet.host,
+ addr->u.inet.port);
+case SOCKET_ADDRESS_TYPE_UNIX:
+return g_strdup_printf("unix:%s",
+ addr->u.q_unix.path);
+case SOCKET_ADDRESS_TYPE_FD:
+return g_strdup_printf("fd:%s", addr->u.fd.str);
+case SOCKET_ADDRESS_TYPE_VSOCK:
+return g_strdup_printf("tcp:%s:%s",
+ addr->u.vsock.cid,
+ addr->u.vsock.port);
+default:
+return g_strdup("unknown address type");
+}
+}
+
+static char *
+migrate_get_socket_address(QTestState *who, const char *parameter)
+{
+QDict *rsp;
+char *result;
+SocketAddressList *addrs;
+Visitor *iv = NULL;
+QObject *object;
+
+rsp = migrate_query(who);
+object = qdict_get(rsp, parameter);
+
+iv = qobject_input_visitor_new(object);
+visit_type_SocketAddressList(iv, NULL, , _abort);
+visit_free(iv);
+
+/* we are only using a single address */
+result = SocketAddress_to_str(addrs->value);
+
+qapi_free_SocketAddressList(addrs);
+qobject_unref(rsp);
+return result;
+}
+
bool migrate_watch_for_events(QTestState *who, const char *name,
QDict *event, void *opaque)
{
@@ -73,13 +122,17 @@ void migrate_qmp(QTestState *who, QTestState *to, const
char *uri,
{
va_list ap;
QDict *args;
+g_autofree char *connect_uri = NULL;
va_start(ap, fmt);
args = qdict_from_vjsonf_nofail(fmt, ap);
va_end(ap);
g_assert(!qdict_haskey(args, "uri"));
-qdict_put_str(args, "uri", uri);
+if (!uri) {
+connect_uri = migrate_get_socket_address(to, "socket-address");
+}
+qdict_put_str(args, "uri", uri ? uri : connect_uri);
qtest_qmp_assert_success(who,
"{ 'execute': 'migrate', 'arguments': %p}", args);
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f645ab26f2..20b1dd031a 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -369,50 +369,6 @@ static void cleanup(const char *filename)
unlink(path);
}
-static char *SocketAddress_to_str(SocketAddress *addr)
-{
-switch (addr->type) {
-case SOCKET_ADDRESS_TYPE_INET:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.inet.host,
- addr->u.inet.port);
-case SOCKET_ADDRESS_TYPE_UNIX:
-return g_strdup_printf("unix:%s",
- addr->u.q_unix.path);
-case SOCKET_ADDRESS_TYPE_FD:
-return g_strdup_printf("fd:%s", addr->u.fd.str);
-case SOCKET_ADDRESS_TYPE_VSOCK:
-return g_strdup_printf("tcp:%s:%s",
- addr->u.vsock.cid,
- addr->u.vsock.port);
-default:
-return g_strdup("unknown address type");
-}
-}
-
-static char *migrate_get_socket_address(QTestState *who, const char *parameter)
-{
-QDict *rsp;
-char *result;
-SocketAddressList *addrs;
-Visitor *iv = NULL;
-QObject *object;
-
-rsp = migrate_query(who);
-object = qdict_get(rsp, parameter);
-
-iv = qobject_input_visitor_new(object);
-visit_type_SocketAddressList(iv, NULL, , _abort);
-visit_free(iv);
-
-/* we are only using a single address */
-result = SocketAddress_to_str(addrs->value);
-
-qapi_free_SocketAddressList(addrs);
-qobject_unref(rsp);
-return result;
-}
-
static long long migrate_get_p
[PATCH v3 6/7] Add multifd_tcp_plain test using list of channels instead of uri
Add a positive test to check multifd live migration but this time
using list of channels (restricted to 1) as the starting point
instead of simple uri string.
Signed-off-by: Het Gala
Suggested-by: Fabiano Rosas
---
tests/qtest/migration-test.c | 29 ++---
1 file changed, 26 insertions(+), 3 deletions(-)
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c
index f94fe713b2..05e5f3ebe5 100644
--- a/tests/qtest/migration-test.c
+++ b/tests/qtest/migration-test.c
@@ -659,6 +659,12 @@ typedef struct {
*/
const char *connect_uri;
+/*
+ * Optional: the JSON formatted list of URIs for the src
+ * QEMU to connect to
+ */
+const char *connect_channels;
+
/* Optional: callback to run at start to set migration parameters */
TestMigrateStartHook start_hook;
/* Optional: callback to run at finish to cleanup */
@@ -2623,7 +2629,7 @@ test_migrate_precopy_tcp_multifd_zstd_start(QTestState
*from,
}
#endif /* CONFIG_ZSTD */
-static void test_multifd_tcp_none(void)
+static void test_multifd_tcp_uri_none(void)
{
MigrateCommon args = {
.listen_uri = "defer",
@@ -2638,6 +2644,21 @@ static void test_multifd_tcp_none(void)
test_precopy_common();
}
+static void test_multifd_tcp_channels_none(void)
+{
+MigrateCommon args = {
+.listen_uri = "defer",
+.start_hook = test_migrate_precopy_tcp_multifd_start,
+.live = true,
+.connect_channels = "[ { 'channel-type': 'main',"
+"'addr': { 'transport': 'socket',"
+" 'type': 'inet',"
+" 'host': '127.0.0.1',"
+" 'port': '0' } } ]",
+};
+test_precopy_common();
+}
+
static void test_multifd_tcp_zlib(void)
{
MigrateCommon args = {
@@ -3531,8 +3552,10 @@ int main(int argc, char **argv)
test_migrate_dirty_limit);
}
}
-migration_test_add("/migration/multifd/tcp/plain/none",
- test_multifd_tcp_none);
+migration_test_add("/migration/multifd/tcp/uri/plain/none",
+ test_multifd_tcp_uri_none);
+migration_test_add("/migration/multifd/tcp/channels/plain/none",
+ test_multifd_tcp_channels_none);
migration_test_add("/migration/multifd/tcp/plain/cancel",
test_multifd_tcp_cancel);
migration_test_add("/migration/multifd/tcp/plain/zlib",
--
2.22.3
[PATCH v3 0/7] qtest: migration: Add tests for introducing 'channels' argument in migrate QAPIs
With recent migrate QAPI changes, enabling the direct use of the 'channels' argument to avoid redundant URI string parsing is achieved. To ensure backward compatibility, both 'uri' and 'channels' are kept as optional parameters in migration QMP commands. However, they are mutually exhaustive, requiring at least one for a successful migration connection. This patchset adds qtests to validate 'uri' and 'channels' arguments' mututally exhaustive behaviour. Additionally, all migration qtests fail to employ 'channel' as the primary method for validating migration QAPIs. This patchset also adds test to enforce only use of 'channel' argument as the initial entry point for migration QAPIs. Patch Summary: - Patch 1-2: - Introduce 'to' object inside migrate_qmp() so and move the calls to migrate_get_socket_address() inside migrate_qmp. Also, replace connect_uri with args->connect_uri everywhere. Patch 3-5: - Add channels argument to allow both migration QAPI arguments independently into migrate_qmp and migrate_qmp_fail. migrate_qmp requires the port value to be changed from 0 to port value coming from migrate_get_socket_address. Add migrate_set_ports to address this change of port value. Patch 6-7: - Add 2 negative tests to validate mutually exhaustive behaviour of migration QAPIs. Add a positive multifd_tcp_plain qtest with only channels as the initial entry point for migration QAPIs. v2->v3 Changelog: - 1. 'channels' introduction is not required now for migrate_qmp_incoming 2. Refactor the code into 7 different patches 3. 'channels' introduction is not required now for migrate_qmp_incoming 4. Remove custom function for converting string to MigrationChannelList 5. move calls for migrate_get_socket_address inside migrate_qmp so that migrate_set_ports can replace the QAPI's port with correct value. Het Gala (7): Add 'to' object into migrate_qmp() Replace connect_uri and move migrate_get_socket_address inside migrate_qmp Add channels parameter in migrate_qmp_fail Add migrate_set_ports into migrate_qmp to change migration port number Add channels parameter in migrate_qmp Add multifd_tcp_plain test using list of channels instead of uri Add negative tests to validate migration QAPIs tests/qtest/migration-helpers.c | 109 +++- tests/qtest/migration-helpers.h | 10 +- tests/qtest/migration-test.c| 176 ++-- 3 files changed, 208 insertions(+), 87 deletions(-) -- 2.22.3
Re: [PATCH v2 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
On 01/03/24 2:19 pm, Het Gala wrote:
On 29/02/24 6:47 am, Fabiano Rosas wrote:
Het Gala writes:
On 27/02/24 1:04 am, Het Gala wrote:
On 26/02/24 6:31 pm, Fabiano Rosas wrote:
Het Gala writes:
On 24/02/24 1:42 am, Fabiano Rosas wrote:
this was the same first approach that I attempted. It won't work
because
The final 'migrate' QAPI with channels string would look like
[...]
I'm not sure what you tried. This works:
g_assert(!qdict_haskey(args, "channels"));
if (channels) {
channels_obj = qobject_from_json(channels, errp);
qdict_put_obj(args, "channels", channels_obj);
}
Are you sure the above works ?
Sorry, please ignore the below doubt. I understood what silly mistakes I
was doing. you were right, qobject_from_json() works fine and converts
string to json object.
Will follow your latest reply and send out the patch really soon. Thank
you for unblocking me here quickly :)
[...]
Regards,
Het Gala
Re: [PATCH v2 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
On 29/02/24 6:47 am, Fabiano Rosas wrote:
Het Gala writes:
On 27/02/24 1:04 am, Het Gala wrote:
On 26/02/24 6:31 pm, Fabiano Rosas wrote:
Het Gala writes:
On 24/02/24 1:42 am, Fabiano Rosas wrote:
this was the same first approach that I attempted. It won't work because
The final 'migrate' QAPI with channels string would look like
{ "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
"main", "addr": { "transport": "socket", "type": "inet", "host":
"10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
instead of
{ "execute": "migrate", "arguments": { "channels": [ { "channel-type":
"main", "addr": { "transport": "socket", "type": "inet", "host":
"10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
It would complain, that channels should be an *array* and not a string.
So, that's the reason parsing was required in qtest too.
I would be glad to hear if there are any ideas to convert /string ->
json object -> add it inside qdict along with uri/ ?
Isn't this what the various qobject_from_json do? How does it work with
the existing tests?
qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
" 'arguments': { "
" 'channels': [ { 'channel-type': 'main',"
" 'addr': { 'transport': 'socket',"
"'type': 'inet',"
"'host': '127.0.0.1',"
"'port': '0' } } ] } }");
We can pass this^ string successfully to QMP somehow...
I think, here in qtest_qmp_assert_success, we actually can pass the
whole QMP command, and it just asserts that return key is present in
the response, though I am not very much familiar with qtest codebase
to verify how swiftly we can convert string into an actual QObject.
[...]
I tried with qobject_from_json type of utility functions and the error I
got was this :
migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126:
qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type <
QTYPE__MAX' failed.
And I suppose this was the case because, there are only limited types of
QTYPE available
typedefenumQType{
QTYPE_NONE,
QTYPE_QNULL,
QTYPE_QNUM,
QTYPE_QSTRING,
QTYPE_QDICT,
QTYPE_QLIST,
QTYPE_QBOOL,
QTYPE__MAX,
} QType;
And 'channels' is a mixture of QDICT and QLIST and hence it is not able
to easily convert from string to json.
Thoughts on this ?
I'm not sure what you tried. This works:
g_assert(!qdict_haskey(args, "channels"));
if (channels) {
channels_obj = qobject_from_json(channels, errp);
qdict_put_obj(args, "channels", channels_obj);
}
Are you sure the above works ?
Sorry I want to get this doubt cleared first, Let's me take a step back
and just focus on the above part and not focus on port issue for now.
Adding a validation test for uri and channels both used together
migration_test_add("/migration/validate_uri/channels/both_set",
test_validate_uri_channels_both_set);
static void test_validate_uri_channels_both_set(void)
{
QTestState *from, *to;
MigrateCommon args = {
.connect_channels = "'channels': [ { 'channel-type': 'main',"
" 'addr': { 'transport': 'socket',"
" 'type': 'inet',"
" 'host': '127.0.0.1',"
" 'port': '0' } } ]",
.listen_uri = "tcp:127.0.0.1:0",
.result = MIG_TEST_QMP_ERROR,
};
if (test_migrate_start(, , args.listen_uri, )) {
return;
}
wait_for_serial("src_serial");
if (args.result == MIG_TEST_QMP_ERROR) {
migrate_qmp_fail(from, args.connect_uri, args.connect_channels,
"{}");
} else {
migrate_qmp(from, args.connect_uri, args.connect_channels, "{}");
}
test_migrate_end(from, to, false);
}
In the migration-helpers.c file, inside migrate_qmp_fail function:
void migrate_qmp_fail(QTestState *who, const char *uri,
const char *channels, const char *fmt, ...)
{
[...]
Error **errp = NULL;
QObject *channels_obj = NULL;
[...]
g_assert(!qdict_haskey(args, "channels"));
if (channels) {
channels_obj = qobject_from_json(channels, errp);
Re: [PATCH v2 1/3] qtest: migration: Enhance qtest migration functions to support 'channels' argument
On 27/02/24 1:04 am, Het Gala wrote:
On 26/02/24 6:31 pm, Fabiano Rosas wrote:
Het Gala writes:
On 24/02/24 1:42 am, Fabiano Rosas wrote:
this was the same first approach that I attempted. It won't work because
The final 'migrate' QAPI with channels string would look like
{ "execute": "migrate", "arguments": { "channels": "[ { "channel-type":
"main", "addr": { "transport": "socket", "type": "inet", "host":
"10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ]" } }
instead of
{ "execute": "migrate", "arguments": { "channels": [ { "channel-type":
"main", "addr": { "transport": "socket", "type": "inet", "host":
"10.117.29.84", "port": "4000" }, "multifd-channels": 2 } ] } }
It would complain, that channels should be an *array* and not a string.
So, that's the reason parsing was required in qtest too.
I would be glad to hear if there are any ideas to convert /string ->
json object -> add it inside qdict along with uri/ ?
Isn't this what the various qobject_from_json do? How does it work with
the existing tests?
qtest_qmp_assert_success(to, "{ 'execute': 'migrate-incoming',"
" 'arguments': { "
" 'channels': [ { 'channel-type': 'main',"
" 'addr': { 'transport': 'socket',"
"'type': 'inet',"
"'host': '127.0.0.1',"
"'port': '0' } } ] } }");
We can pass this^ string successfully to QMP somehow...
I think, here in qtest_qmp_assert_success, we actually can pass the
whole QMP command, and it just asserts that return key is present in
the response, though I am not very much familiar with qtest codebase
to verify how swiftly we can convert string into an actual QObject.
[...]
I tried with qobject_from_json type of utility functions and the error I
got was this :
migration-test: /rpmbuild/SOURCES/qemu/include/qapi/qmp/qobject.h:126:
qobject_type: Assertion `QTYPE_NONE < obj->base.type && obj->base.type <
QTYPE__MAX' failed.
And I suppose this was the case because, there are only limited types of
QTYPE available
typedefenumQType{
QTYPE_NONE,
QTYPE_QNULL,
QTYPE_QNUM,
QTYPE_QSTRING,
QTYPE_QDICT,
QTYPE_QLIST,
QTYPE_QBOOL,
QTYPE__MAX,
} QType;
And 'channels' is a mixture of QDICT and QLIST and hence it is not able
to easily convert from string to json.
Thoughts on this ?
static void do_test_validate_uri_channel(MigrateCommon *args)
{
QTestState *from, *to;
g_autofree char *connect_uri = NULL;
if (test_migrate_start(, , args->listen_uri, >start)) {
return;
}
Regards,
Het Gala
Regards,
Het Gala
