| Message ID | 20240216090624.75445-4-het.gala@nutanix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qtest: migration: Add validation tests for 'channels' argument in migrate QAPIs | expand |
On Fri, Feb 16, 2024 at 09:06:24AM +0000, Het Gala wrote: > Ideally QAPI 'migrate' and 'migrate-incoming' does not allow 'uri' and > 'channels' both arguments to be present in the arguments list as they > are mutually exhaustive. > > Add a negative test case to validate the same. Even before the migration > connection is established, qmp command will error out with > MIG_TEST_QMP_ERROR. > > Signed-off-by: Het Gala <het.gala@nutanix.com> > --- > tests/qtest/migration-test.c | 83 ++++++++++++++++++++++++++++++++++++ > 1 file changed, 83 insertions(+) > > diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c > index 0bc69b1943..9b9395307f 100644 > --- a/tests/qtest/migration-test.c > +++ b/tests/qtest/migration-test.c > @@ -26,6 +26,7 @@ > #include "qapi/qobject-output-visitor.h" > #include "crypto/tlscredspsk.h" > #include "qapi/qmp/qlist.h" > +#include "qemu/cutils.h" > > #include "migration-helpers.h" > #include "tests/migration/migration-test.h" > @@ -2516,6 +2517,86 @@ static void test_validate_uuid_dst_not_set(void) > do_test_validate_uuid(&args, false); > } > > +static MigrationChannelList *uri_to_channels(const char *uri) > +{ > + MigrationChannelList *channels = g_new0(MigrationChannelList, 1); > + MigrationChannel *val = g_new0(MigrationChannel, 1); > + MigrationAddress *addr = g_new0(MigrationAddress, 1); > + char **saddr; > + > + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; > + > + saddr = g_strsplit((char *)uri, ":", 3); > + if (!saddr[0] || saddr[0] != g_strdup("tcp")) { > + fprintf(stderr, "%s: Invalid URI: %s\n", __func__, uri); Can use g_assert() in such a test; stderr can be easily ignored and forgotten when it hits. I'd think parsing it from URI is a slight overkill, as we can pass in rubbish in the "channels" parameter, but it's still okay. > + } > + addr->u.socket.type = SOCKET_ADDRESS_TYPE_INET; > + addr->u.socket.u.inet.host = g_strdup(saddr[1]); > + addr->u.socket.u.inet.port = g_strdup(saddr[2]); > + g_strfreev(saddr); > + > + val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; > + val->addr = addr; > + channels->value = val; > + > + return channels; > +} > + > +static void do_test_validate_uri_channel(MigrateCommon *args, bool should_fail) > +{ > + QTestState *from, *to; > + > + if (test_migrate_start(&from, &to, args->listen_uri, &args->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. > + */ > + if (args->result == MIG_TEST_QMP_ERROR) { > + migrate_qmp_fail(from, > + args->connect_uri ? args->connect_uri : NULL, > + args->connect_channels ? args->connect_channels : NULL, "{}"); > + goto finish; > + } IIUC below are dead code. We can drop them. > + > + migrate_qmp(from, > + args->connect_uri ? args->connect_uri : NULL, > + args->connect_channels ? args->connect_channels : NULL, "{}"); > + > + if (should_fail) { > + qtest_set_expected_status(to, EXIT_FAILURE); > + wait_for_migration_fail(from, false); > + } else { > + wait_for_migration_complete(from); > + } > + > +finish: > + test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED); > +} > + > +static void > +test_validate_uri_channels_both_set(void) > +{ > + g_autofree char *uri = g_strdup("tcp:127.0.0.1:0"); > + > + MigrateCommon args = { > + .start = { > + .hide_stderr = true, > + }, > + .connect_uri = uri, > + .connect_channels = uri_to_channels(uri), > + .listen_uri = "defer", > + .result = MIG_TEST_QMP_ERROR, > + }; Instead of using MigrateCommon/MIG_TEST_QMP_ERROR, IMHO you can unwrap the whole do_test_validate_uri_channel() here, invoke migrate_qmp_fail() directly with the wrong parameter set. See, for excample, test_baddest(). PS: please scratch my previous comment on patch 1 over the assertion; I just read that all wrongly.. sorry. > + > + do_test_validate_uri_channel(&args, true); > +} > + > /* > * The way auto_converge works, we need to do too many passes to > * run this test. Auto_converge logic is only run once every > @@ -3544,6 +3625,8 @@ 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); > /* > * See explanation why this test is slow on function definition > */ > -- > 2.22.3 >
On 20/02/24 11:57 am, Peter Xu wrote: > On Fri, Feb 16, 2024 at 09:06:24AM +0000, Het Gala wrote: >> Ideally QAPI 'migrate' and 'migrate-incoming' does not allow 'uri' and >> 'channels' both arguments to be present in the arguments list as they >> are mutually exhaustive. >> >> Add a negative test case to validate the same. Even before the migration >> connection is established, qmp command will error out with >> MIG_TEST_QMP_ERROR. >> >> Signed-off-by: Het Gala<het.gala@nutanix.com> >> --- >> tests/qtest/migration-test.c | 83 ++++++++++++++++++++++++++++++++++++ >> 1 file changed, 83 insertions(+) >> >> diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c >> index 0bc69b1943..9b9395307f 100644 >> --- a/tests/qtest/migration-test.c >> +++ b/tests/qtest/migration-test.c >> @@ -26,6 +26,7 @@ >> #include "qapi/qobject-output-visitor.h" >> #include "crypto/tlscredspsk.h" >> #include "qapi/qmp/qlist.h" >> +#include "qemu/cutils.h" >> >> #include "migration-helpers.h" >> #include "tests/migration/migration-test.h" >> @@ -2516,6 +2517,86 @@ static void test_validate_uuid_dst_not_set(void) >> do_test_validate_uuid(&args, false); >> } >> >> +static MigrationChannelList *uri_to_channels(const char *uri) >> +{ >> + MigrationChannelList *channels = g_new0(MigrationChannelList, 1); >> + MigrationChannel *val = g_new0(MigrationChannel, 1); >> + MigrationAddress *addr = g_new0(MigrationAddress, 1); >> + char **saddr; >> + >> + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; >> + >> + saddr = g_strsplit((char *)uri, ":", 3); >> + if (!saddr[0] || saddr[0] != g_strdup("tcp")) { >> + fprintf(stderr, "%s: Invalid URI: %s\n", __func__, uri); > Can use g_assert() in such a test; stderr can be easily ignored and > forgotten when it hits. Ack. Will change that. > I'd think parsing it from URI is a slight overkill, as we can pass in > rubbish in the "channels" parameter, but it's still okay. That's why I didn't make the dummy function of socket_parse(), which would have been a overkill, and just a split function was added. Let me have it for the v2 patchset. If it still feels like a overkill, will just have a dummy value for host and port, and type. >> + } >> + addr->u.socket.type = SOCKET_ADDRESS_TYPE_INET; >> + addr->u.socket.u.inet.host = g_strdup(saddr[1]); >> + addr->u.socket.u.inet.port = g_strdup(saddr[2]); >> + g_strfreev(saddr); >> + >> + val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; >> + val->addr = addr; >> + channels->value = val; >> + >> + return channels; >> +} >> + >> +static void do_test_validate_uri_channel(MigrateCommon *args, bool should_fail) >> +{ >> + QTestState *from, *to; >> + >> + if (test_migrate_start(&from, &to, args->listen_uri, &args->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. >> + */ >> + if (args->result == MIG_TEST_QMP_ERROR) { >> + migrate_qmp_fail(from, >> + args->connect_uri ? args->connect_uri : NULL, >> + args->connect_channels ? args->connect_channels : NULL, "{}"); >> + goto finish; >> + } > IIUC below are dead code. We can drop them. I have kept it intentional, inorder to add positive test cases.In positive test cases, migration would be performed successfully. Will add a patch for positive test case too in v2 patchset series. >> + >> + migrate_qmp(from, >> + args->connect_uri ? args->connect_uri : NULL, >> + args->connect_channels ? args->connect_channels : NULL, "{}"); >> + >> + if (should_fail) { >> + qtest_set_expected_status(to, EXIT_FAILURE); >> + wait_for_migration_fail(from, false); >> + } else { >> + wait_for_migration_complete(from); >> + } >> + >> +finish: >> + test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED); >> +} >> + >> +static void >> +test_validate_uri_channels_both_set(void) >> +{ >> + g_autofree char *uri = g_strdup("tcp:127.0.0.1:0"); >> + >> + MigrateCommon args = { >> + .start = { >> + .hide_stderr = true, >> + }, >> + .connect_uri = uri, >> + .connect_channels = uri_to_channels(uri), >> + .listen_uri = "defer", >> + .result = MIG_TEST_QMP_ERROR, >> + }; > Instead of using MigrateCommon/MIG_TEST_QMP_ERROR, IMHO you can unwrap the > whole do_test_validate_uri_channel() here, invoke migrate_qmp_fail() > directly with the wrong parameter set. Again, kept this for separating negative test cases from positive ones. If it still does not make sense in 2nd patchset, can unwrap into a single function itself. > See, for excample, test_baddest(). > > PS: please scratch my previous comment on patch 1 over the assertion; I > just read that all wrongly.. sorry. Ack, no worries. >> + >> + do_test_validate_uri_channel(&args, true); >> +} >> + >> /* >> * The way auto_converge works, we need to do too many passes to >> * run this test. Auto_converge logic is only run once every >> @@ -3544,6 +3625,8 @@ 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); >> /* >> * See explanation why this test is slow on function definition >> */ >> -- >> 2.22.3 >> Regards, Het Gala
diff --git a/tests/qtest/migration-test.c b/tests/qtest/migration-test.c index 0bc69b1943..9b9395307f 100644 --- a/tests/qtest/migration-test.c +++ b/tests/qtest/migration-test.c @@ -26,6 +26,7 @@ #include "qapi/qobject-output-visitor.h" #include "crypto/tlscredspsk.h" #include "qapi/qmp/qlist.h" +#include "qemu/cutils.h" #include "migration-helpers.h" #include "tests/migration/migration-test.h" @@ -2516,6 +2517,86 @@ static void test_validate_uuid_dst_not_set(void) do_test_validate_uuid(&args, false); } +static MigrationChannelList *uri_to_channels(const char *uri) +{ + MigrationChannelList *channels = g_new0(MigrationChannelList, 1); + MigrationChannel *val = g_new0(MigrationChannel, 1); + MigrationAddress *addr = g_new0(MigrationAddress, 1); + char **saddr; + + addr->transport = MIGRATION_ADDRESS_TYPE_SOCKET; + + saddr = g_strsplit((char *)uri, ":", 3); + if (!saddr[0] || saddr[0] != g_strdup("tcp")) { + fprintf(stderr, "%s: Invalid URI: %s\n", __func__, uri); + } + addr->u.socket.type = SOCKET_ADDRESS_TYPE_INET; + addr->u.socket.u.inet.host = g_strdup(saddr[1]); + addr->u.socket.u.inet.port = g_strdup(saddr[2]); + g_strfreev(saddr); + + val->channel_type = MIGRATION_CHANNEL_TYPE_MAIN; + val->addr = addr; + channels->value = val; + + return channels; +} + +static void do_test_validate_uri_channel(MigrateCommon *args, bool should_fail) +{ + QTestState *from, *to; + + if (test_migrate_start(&from, &to, args->listen_uri, &args->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. + */ + if (args->result == MIG_TEST_QMP_ERROR) { + migrate_qmp_fail(from, + args->connect_uri ? args->connect_uri : NULL, + args->connect_channels ? args->connect_channels : NULL, "{}"); + goto finish; + } + + migrate_qmp(from, + args->connect_uri ? args->connect_uri : NULL, + args->connect_channels ? args->connect_channels : NULL, "{}"); + + if (should_fail) { + qtest_set_expected_status(to, EXIT_FAILURE); + wait_for_migration_fail(from, false); + } else { + wait_for_migration_complete(from); + } + +finish: + test_migrate_end(from, to, args->result == MIG_TEST_SUCCEED); +} + +static void +test_validate_uri_channels_both_set(void) +{ + g_autofree char *uri = g_strdup("tcp:127.0.0.1:0"); + + MigrateCommon args = { + .start = { + .hide_stderr = true, + }, + .connect_uri = uri, + .connect_channels = uri_to_channels(uri), + .listen_uri = "defer", + .result = MIG_TEST_QMP_ERROR, + }; + + do_test_validate_uri_channel(&args, true); +} + /* * The way auto_converge works, we need to do too many passes to * run this test. Auto_converge logic is only run once every @@ -3544,6 +3625,8 @@ 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); /* * See explanation why this test is slow on function definition */
Ideally QAPI 'migrate' and 'migrate-incoming' does not allow 'uri' and 'channels' both arguments to be present in the arguments list as they are mutually exhaustive. Add a negative test case to validate the same. Even before the migration connection is established, qmp command will error out with MIG_TEST_QMP_ERROR. Signed-off-by: Het Gala <het.gala@nutanix.com> --- tests/qtest/migration-test.c | 83 ++++++++++++++++++++++++++++++++++++ 1 file changed, 83 insertions(+)