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);
+    }
+    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
      */
-- 
2.22.3

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
> 

-- 
Peter Xu

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