[Qemu-devel] [PATCH v5 0/5] Add make check tests for Migration

[Qemu-devel] [PATCH v5 0/5] Add make check tests for Migration

[Juan Quintela]

Hi

In v5:
- Several patches moved to pull request
- merge info_migrate and migration_tests
  only missing bit is tcp_port, needed for tcp tests
- Rename tcp-port to x-tcp-port
  We will get better naming from David at some point, and we will use that bit
- ppc: use inline code as suggested by lvivier

Please, review.

It is based on my previous pull request

Based-on: 20180129120932.12874-1-quintela@redhat.com

[v4]
- rebase on top on v4 info_migrate patches
- Tune sleeps to make patches fast
- Create a deprecated test for deprecated commands (i.e. make peterxu happy)
- create migrate_start_postcopy function
- fix naming/sizes between power and x86
- cleanup comments to match code

[v3]

- No more tests for deprecated parameters. Now I only use
  migrate_set_parameter.  If there is a deprecated command for that,
  we tests it there.
- free "result" string, always good to return memory (Peter found it)
- use the new tcp_port parameter from info migrate.  So we are
  handling well the tcp case.
- lots of code movement around to make everything consistent.
- Several patches already integrated upstream.

[v2]
- to make review easier, I started renaming postcopy-test.c to migration-test.c
- Did cleanups/refactoring there
- Don't use global-qtest anymore
- check that the parameters that we sent got really set
- RFH: comrpress threads tests is not working for some weird reason.  Using the same code on command line works.
  still investigating why.

ToDoo:

- tcp: after discussions with dave, we ended in conclusion that we
  need to use the 0 port and let the system gives us a free one

  But .... that means that we need to be able to get that port back somehow.
  "info migrate" woring on destination side?

- compression threads.  There is some weird interaction with the test
  hardness and every migration thread get waiting in a different
  semaphore.  Investigating if it is a race/bug/whateverr

- deprecated commands: There was a suggestion to make
  migrate_set_parameter look at the parameter name and test old/new
  depending on something.  Not sure what to do here.

- testing commands: Is there a way to launch qemu and just sent
  qmp/hmp commands without having to really run anything else?

[v1]
- add test for precopy for unix/tcp
  exec and fd to came, don't know how to test rdma without hardware
- add tests using deprecated interfaces
- add test for xbzrle
  Note to myself, there is no way to set the cache size with migraton_set_parameters
- Add test for compress threads
  disabled on the series, right now it appears that compression is not working at all
- Move postcopy to use new results
  Idea is to move it on top of migration-test.c, but first I want some reviews on basic idea


Juan Quintela (5):
  migration: Create tcp_port parameter
  migration: Set the migration tcp port
  tests: Migration ppc now inlines its program
  tests: Add basic migration precopy tcp test
  [RFH] tests: Add migration compress threads tests

 hmp.c                  |   3 +
 migration/migration.c  |  18 ++++++
 migration/migration.h  |   2 +
 migration/socket.c     |  35 ++++++++++--
 qapi/migration.json    |  19 ++++++-
 tests/migration-test.c | 151 ++++++++++++++++++++++++++++++++++++-------------
 6 files changed, 181 insertions(+), 47 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v5 1/5] migration: Create tcp_port parameter

[Juan Quintela]

It will be used to store the uri tcp_port parameter.  This is the only
parameter than can change and we can need to be able to connect to it.

Signed-off-by: Juan Quintela <quintela@redhat.com>

--

This used to be uri parameter, but it has so many troubles to
reproduce that it don't just make sense.
---
 hmp.c                 |  3 +++
 migration/migration.c |  8 ++++++++
 qapi/migration.json   | 19 ++++++++++++++++---
 3 files changed, 27 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index b3de32d219..369cb991f2 100644
--- a/hmp.c
+++ b/hmp.c
@@ -345,6 +345,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict)
         monitor_printf(mon, "%s: %" PRIu64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
+        monitor_printf(mon, "%s: %d\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_X_TCP_PORT),
+            params->x_tcp_port);
     }
 
     qapi_free_MigrationParameters(params);
diff --git a/migration/migration.c b/migration/migration.c
index 44cbfb0ddd..eb6958dcda 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -522,6 +522,8 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
     params->x_multifd_page_count = s->parameters.x_multifd_page_count;
     params->has_xbzrle_cache_size = true;
     params->xbzrle_cache_size = s->parameters.xbzrle_cache_size;
+    params->has_x_tcp_port = true;
+    params->x_tcp_port = s->parameters.x_tcp_port;
 
     return params;
 }
@@ -889,6 +891,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
     if (params->has_xbzrle_cache_size) {
         dest->xbzrle_cache_size = params->xbzrle_cache_size;
     }
+    if (params->has_x_tcp_port) {
+        dest->x_tcp_port = params->x_tcp_port;
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -961,6 +966,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
         s->parameters.xbzrle_cache_size = params->xbzrle_cache_size;
         xbzrle_cache_resize(params->xbzrle_cache_size, errp);
     }
+    if (params->has_x_tcp_port) {
+        s->parameters.x_tcp_port = params->x_tcp_port;
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
diff --git a/qapi/migration.json b/qapi/migration.json
index 4cd3d13158..26661b0df6 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -488,6 +488,9 @@
 #                     and a power of 2
 #                     (Since 2.11)
 #
+# @x-tcp-port: Only used for tcp, to know what the real port is
+#                     (Since 2.12)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -496,7 +499,7 @@
            'tls-creds', 'tls-hostname', 'max-bandwidth',
            'downtime-limit', 'x-checkpoint-delay', 'block-incremental',
            'x-multifd-channels', 'x-multifd-page-count',
-           'xbzrle-cache-size' ] }
+           'xbzrle-cache-size', 'x-tcp-port' ] }
 
 ##
 # @MigrateSetParameters:
@@ -564,6 +567,10 @@
 #                     needs to be a multiple of the target page size
 #                     and a power of 2
 #                     (Since 2.11)
+#
+# @x-tcp-port: Only used for tcp, to know what the real port is
+#                     (Since 2.12)
+#
 # Since: 2.4
 ##
 # TODO either fuse back into MigrationParameters, or make
@@ -582,7 +589,8 @@
             '*block-incremental': 'bool',
             '*x-multifd-channels': 'int',
             '*x-multifd-page-count': 'int',
-            '*xbzrle-cache-size': 'size' } }
+            '*xbzrle-cache-size': 'size',
+            '*x-tcp-port': 'uint16'} }
 
 ##
 # @migrate-set-parameters:
@@ -665,6 +673,10 @@
 #                     needs to be a multiple of the target page size
 #                     and a power of 2
 #                     (Since 2.11)
+#
+# @x-tcp-port: Only used for tcp, to know what the real port is
+#                     (Since 2.12)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -681,7 +693,8 @@
             '*block-incremental': 'bool' ,
             '*x-multifd-channels': 'uint8',
             '*x-multifd-page-count': 'uint32',
-            '*xbzrle-cache-size': 'size' } }
+            '*xbzrle-cache-size': 'size',
+            '*x-tcp-port': 'uint16'} }
 
 ##
 # @query-migrate-parameters:
-- 
2.14.3

[Qemu-devel] [PATCH v5 2/5] migration: Set the migration tcp port

[Juan Quintela]

We can set the port parameter as zero.  This patch lets us know what
port the system was choosen for us.  Now we can migrate to this place.

Signed-off-by: Juan Quintela <quintela@redhat.com>

--

This was migrate_set_uri(), but as we only need the tcp_port, change
to that one.
---
 migration/migration.c | 10 ++++++++++
 migration/migration.h |  2 ++
 migration/socket.c    | 35 ++++++++++++++++++++++++++++++-----
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index eb6958dcda..53818a87af 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -245,6 +245,16 @@ void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
     }
 }
 
+void migrate_set_port(const uint16_t port, Error **errp)
+{
+    MigrateSetParameters p = {
+        .has_x_tcp_port = true,
+        .x_tcp_port = port,
+    };
+
+    qmp_migrate_set_parameters(&p, errp);
+}
+
 void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
diff --git a/migration/migration.h b/migration/migration.h
index d3b214e5ba..7c9ac3f00b 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -232,4 +232,6 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
 void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname,
                               ram_addr_t start, size_t len);
 
+void migrate_set_port(const uint16_t port, Error **errp);
+
 #endif
diff --git a/migration/socket.c b/migration/socket.c
index e090097077..08606c665d 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -15,6 +15,7 @@
  */
 
 #include "qemu/osdep.h"
+#include "qemu/cutils.h"
 
 #include "qemu-common.h"
 #include "qemu/error-report.h"
@@ -160,17 +161,24 @@ out:
 }
 
 
-static void socket_start_incoming_migration(SocketAddress *saddr,
-                                            Error **errp)
+static SocketAddress *socket_start_incoming_migration(SocketAddress *saddr,
+                                                      Error **errp)
 {
     QIOChannelSocket *listen_ioc = qio_channel_socket_new();
+    SocketAddress *address;
 
     qio_channel_set_name(QIO_CHANNEL(listen_ioc),
                          "migration-socket-listener");
 
     if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
         object_unref(OBJECT(listen_ioc));
-        return;
+        return NULL;
+    }
+
+    address = qio_channel_socket_get_local_address(listen_ioc, errp);
+    if (address < 0) {
+        object_unref(OBJECT(listen_ioc));
+        return NULL;
     }
 
     qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
@@ -178,14 +186,28 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
                           socket_accept_incoming_migration,
                           listen_ioc,
                           (GDestroyNotify)object_unref);
+    return address;
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
 {
     Error *err = NULL;
     SocketAddress *saddr = tcp_build_address(host_port, &err);
+
     if (!err) {
-        socket_start_incoming_migration(saddr, &err);
+        SocketAddress *address = socket_start_incoming_migration(saddr, &err);
+
+        if (address) {
+            unsigned long long port;
+
+            if (parse_uint_full(address->u.inet.port, &port, 10) < 0) {
+                error_setg(errp, "error parsing port in '%s'",
+                           address->u.inet.port);
+            } else {
+                migrate_set_port(port, errp);
+            }
+            qapi_free_SocketAddress(address);
+        }
     }
     qapi_free_SocketAddress(saddr);
     error_propagate(errp, err);
@@ -194,6 +216,9 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
 void unix_start_incoming_migration(const char *path, Error **errp)
 {
     SocketAddress *saddr = unix_build_address(path);
-    socket_start_incoming_migration(saddr, errp);
+    SocketAddress *address;
+
+    address = socket_start_incoming_migration(saddr, errp);
+    qapi_free_SocketAddress(address);
     qapi_free_SocketAddress(saddr);
 }
-- 
2.14.3

[Qemu-devel] [PATCH v5 3/5] tests: Migration ppc now inlines its program

[Juan Quintela]

No need to write it to a file.  Just need a proper firmware O:-)

Signed-off-by: Juan Quintela <quintela@redhat.com>
CC: Laurent Vivier <lvivier@redhat.com>
---
 tests/migration-test.c | 41 +++++------------------------------------
 1 file changed, 5 insertions(+), 36 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index fcad2ed460..26439f6071 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -18,9 +18,6 @@
 #include "qemu/sockets.h"
 #include "chardev/char.h"
 #include "sysemu/sysemu.h"
-#include "hw/nvram/chrp_nvram.h"
-
-#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
 
 const unsigned start_address = 1024 * 1024;
 const unsigned end_address = 100 * 1024 * 1024;
@@ -133,36 +130,6 @@ static void init_bootfile_x86(const char *bootpath)
     fclose(bootfile);
 }
 
-static void init_bootfile_ppc(const char *bootpath)
-{
-    FILE *bootfile;
-    char buf[MIN_NVRAM_SIZE];
-    ChrpNvramPartHdr *header = (ChrpNvramPartHdr *)buf;
-
-    memset(buf, 0, MIN_NVRAM_SIZE);
-
-    /* Create a "common" partition in nvram to store boot-command property */
-
-    header->signature = CHRP_NVPART_SYSTEM;
-    memcpy(header->name, "common", 6);
-    chrp_nvram_finish_partition(header, MIN_NVRAM_SIZE);
-
-    /* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
-     * so let's modify memory between 1MB and 100MB
-     * to do like PC bootsector
-     */
-
-    sprintf(buf + 16,
-            "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i c! 1000 +loop "
-            ".\" B\" 0 until", end_address, start_address);
-
-    /* Write partition to the NVRAM file */
-
-    bootfile = fopen(bootpath, "wb");
-    g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
-    fclose(bootfile);
-}
-
 /*
  * Wait for some output in the serial output file,
  * we get an 'A' followed by an endless string of 'B's
@@ -453,12 +420,14 @@ static void test_migrate_start(QTestState **from, QTestState **to,
         if (access("/sys/module/kvm_hv", F_OK)) {
             accel = "tcg";
         }
-        init_bootfile_ppc(bootpath);
         cmd_src = g_strdup_printf("-machine accel=%s -m 256M"
                                   " -name source,debug-threads=on"
                                   " -serial file:%s/src_serial"
-                                  " -drive file=%s,if=pflash,format=raw",
-                                  accel, tmpfs, bootpath);
+                                  " -prom-env '"
+                                  "boot-command=hex .\" _\" begin %x %x "
+                                  "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
+                                  "until'",  accel, tmpfs, end_address,
+                                  start_address);
         cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
                                   " -name target,debug-threads=on"
                                   " -serial file:%s/dest_serial"
-- 
2.14.3

[Qemu-devel] [PATCH v5 4/5] tests: Add basic migration precopy tcp test

[Juan Quintela]

Not sharing code from precopy/unix because we have to read back the
tcp parameter.

Signed-off-by: Juan Quintela <quintela@redhat.com>
Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
Reviewed-by: Peter Xu <peterx@redhat.com>
---
 tests/migration-test.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 55 insertions(+), 3 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 26439f6071..355e6d18e9 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -320,8 +320,7 @@ static void cleanup(const char *filename)
     g_free(path);
 }
 
-static void migrate_check_parameter(QTestState *who, const char *parameter,
-                                    const char *value)
+static char *migrate_get_parameter(QTestState *who, const char *parameter)
 {
     QDict *rsp, *rsp_return;
     char *result;
@@ -330,9 +329,18 @@ static void migrate_check_parameter(QTestState *who, const char *parameter,
     rsp_return = qdict_get_qdict(rsp, "return");
     result = g_strdup_printf("%" PRId64,
                              qdict_get_try_int(rsp_return,  parameter, -1));
+    QDECREF(rsp);
+    return result;
+}
+
+static void migrate_check_parameter(QTestState *who, const char *parameter,
+                                    const char *value)
+{
+    char *result;
+
+    result = migrate_get_parameter(who, parameter);
     g_assert_cmpstr(result, ==, value);
     g_free(result);
-    QDECREF(rsp);
 }
 
 static void migrate_set_parameter(QTestState *who, const char *parameter,
@@ -664,6 +672,49 @@ static void test_xbzrle_unix(void)
     g_free(uri);
 }
 
+static void test_precopy_tcp(void)
+{
+    char *uri;
+    char *port;
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, "tcp:127.0.0.1:0");
+
+    /* We want to pick a speed slow enough that the test completes
+     * quickly, but that it doesn't complete precopy even on a slow
+     * machine, so also set the downtime.
+     */
+    /* 1 ms should make it not converge*/
+    migrate_set_parameter(from, "downtime-limit", "1");
+    /* 1GB/s */
+    migrate_set_parameter(from, "max-bandwidth", "1000000000");
+
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    port = migrate_get_parameter(to, "x-tcp-port");
+    uri = g_strdup_printf("tcp:127.0.0.1:%s", port);
+
+    migrate(from, uri);
+
+    wait_for_migration_pass(from);
+
+    /* 300ms should converge */
+    migrate_set_parameter(from, "downtime-limit", "300");
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+    qtest_qmp_eventwait(to, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+
+    test_migrate_end(from, to);
+    g_free(uri);
+    g_free(port);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -684,6 +735,7 @@ int main(int argc, char **argv)
     module_call_init(MODULE_INIT_QOM);
 
     qtest_add_func("/migration/precopy/unix", test_precopy_unix);
+    qtest_add_func("/migration/precopy/tcp", test_precopy_tcp);
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/postcopy/unix", test_postcopy);
     qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
-- 
2.14.3

[Qemu-devel] [PATCH v5 5/5] [RFH] tests: Add migration compress threads tests

[Juan Quintela]

Yeap, it is still not working. trying to learn how to debug threads
for guests running from the testt hardness.

For some reason, compression is not working at the moment, test is
disabled until I found why.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 tests/migration-test.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 52 insertions(+)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 355e6d18e9..02d8ae7cb6 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -715,6 +715,55 @@ static void test_precopy_tcp(void)
     g_free(port);
 }
 
+static void test_compress(const char *uri)
+{
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, uri);
+
+    /* We want to pick a speed slow enough that the test completes
+     * quickly, but that it doesn't complete precopy even on a slow
+     * machine, so also set the downtime.
+     */
+    /* 1 ms should make it not converge*/
+    migrate_set_parameter(from, "downtime-limit", "1");
+    /* 1GB/s */
+    migrate_set_parameter(from, "max-bandwidth", "100000000");
+
+    migrate_set_parameter(from, "compress-threads", "4");
+    migrate_set_parameter(to, "decompress-threads", "3");
+
+    migrate_set_capability(from, "compress", "true");
+    migrate_set_capability(to, "compress", "true");
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    migrate(from, uri);
+
+    wait_for_migration_pass(from);
+
+    /* 300ms should converge */
+    migrate_set_parameter(from, "downtime-limit", "300");
+
+    if (!got_stop) {
+        qtest_qmp_eventwait(from, "STOP");
+    }
+    qtest_qmp_eventwait(to, "RESUME");
+
+    wait_for_serial("dest_serial");
+    wait_for_migration_complete(from);
+
+    test_migrate_end(from, to);
+}
+
+static void test_compress_unix(void)
+{
+    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+
+    test_compress(uri);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -739,6 +788,9 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/postcopy/unix", test_postcopy);
     qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
+    if (0) {
+        qtest_add_func("/migration/compress/unix", test_compress_unix);
+    }
 
     ret = g_test_run();
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v5 3/5] tests: Migration ppc now inlines its program

[Laurent Vivier]

On 29/01/2018 13:17, Juan Quintela wrote:
> No need to write it to a file.  Just need a proper firmware O:-)
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> CC: Laurent Vivier <lvivier@redhat.com>
> ---
>  tests/migration-test.c | 41 +++++------------------------------------
>  1 file changed, 5 insertions(+), 36 deletions(-)
> 
> diff --git a/tests/migration-test.c b/tests/migration-test.c
> index fcad2ed460..26439f6071 100644
> --- a/tests/migration-test.c
> +++ b/tests/migration-test.c
> @@ -18,9 +18,6 @@
>  #include "qemu/sockets.h"
>  #include "chardev/char.h"
>  #include "sysemu/sysemu.h"
> -#include "hw/nvram/chrp_nvram.h"
> -
> -#define MIN_NVRAM_SIZE 8192 /* from spapr_nvram.c */
>  
>  const unsigned start_address = 1024 * 1024;
>  const unsigned end_address = 100 * 1024 * 1024;
> @@ -133,36 +130,6 @@ static void init_bootfile_x86(const char *bootpath)
>      fclose(bootfile);
>  }
>  
> -static void init_bootfile_ppc(const char *bootpath)
> -{
> -    FILE *bootfile;
> -    char buf[MIN_NVRAM_SIZE];
> -    ChrpNvramPartHdr *header = (ChrpNvramPartHdr *)buf;
> -
> -    memset(buf, 0, MIN_NVRAM_SIZE);
> -
> -    /* Create a "common" partition in nvram to store boot-command property */
> -
> -    header->signature = CHRP_NVPART_SYSTEM;
> -    memcpy(header->name, "common", 6);
> -    chrp_nvram_finish_partition(header, MIN_NVRAM_SIZE);
> -
> -    /* FW_MAX_SIZE is 4MB, but slof.bin is only 900KB,
> -     * so let's modify memory between 1MB and 100MB
> -     * to do like PC bootsector
> -     */

You should copy this comment where we have inlined the code (below).

> -
> -    sprintf(buf + 16,
> -            "boot-command=hex .\" _\" begin %x %x do i c@ 1 + i c! 1000 +loop "
> -            ".\" B\" 0 until", end_address, start_address);
> -
> -    /* Write partition to the NVRAM file */
> -
> -    bootfile = fopen(bootpath, "wb");
> -    g_assert_cmpint(fwrite(buf, MIN_NVRAM_SIZE, 1, bootfile), ==, 1);
> -    fclose(bootfile);
> -}
> -
>  /*
>   * Wait for some output in the serial output file,
>   * we get an 'A' followed by an endless string of 'B's
> @@ -453,12 +420,14 @@ static void test_migrate_start(QTestState **from, QTestState **to,
>          if (access("/sys/module/kvm_hv", F_OK)) {
>              accel = "tcg";
>          }
> -        init_bootfile_ppc(bootpath);

I think you could move bootpath allocation/free inside the
'if (strcmp(arch, "i386") == 0 || strcmp(arch, "x86_64") == 0) {' as it
is not used anymore by ppc.

>          cmd_src = g_strdup_printf("-machine accel=%s -m 256M"
>                                    " -name source,debug-threads=on"
>                                    " -serial file:%s/src_serial"
> -                                  " -drive file=%s,if=pflash,format=raw",
> -                                  accel, tmpfs, bootpath);
> +                                  " -prom-env '"
> +                                  "boot-command=hex .\" _\" begin %x %x "
> +                                  "do i c@ 1 + i c! 1000 +loop .\" B\" 0 "
> +                                  "until'",  accel, tmpfs, end_address,
> +                                  start_address);
>          cmd_dst = g_strdup_printf("-machine accel=%s -m 256M"
>                                    " -name target,debug-threads=on"
>                                    " -serial file:%s/dest_serial"
> 

Thanks,
Laurent

Re: [Qemu-devel] [PATCH v5 2/5] migration: Set the migration tcp port

[Peter Xu]

On Mon, Jan 29, 2018 at 01:17:51PM +0100, Juan Quintela wrote:
> We can set the port parameter as zero.  This patch lets us know what
> port the system was choosen for us.  Now we can migrate to this place.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> --
> 
> This was migrate_set_uri(), but as we only need the tcp_port, change
> to that one.
> ---
>  migration/migration.c | 10 ++++++++++
>  migration/migration.h |  2 ++
>  migration/socket.c    | 35 ++++++++++++++++++++++++++++++-----
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index eb6958dcda..53818a87af 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -245,6 +245,16 @@ void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
>      }
>  }
>  
> +void migrate_set_port(const uint16_t port, Error **errp)
> +{
> +    MigrateSetParameters p = {
> +        .has_x_tcp_port = true,
> +        .x_tcp_port = port,
> +    };
> +
> +    qmp_migrate_set_parameters(&p, errp);

If we are calling qmp_migrate_set_parameters() here, does it mean that
user can also set this parameter via QMP?

> +}
> +
>  void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
> diff --git a/migration/migration.h b/migration/migration.h
> index d3b214e5ba..7c9ac3f00b 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -232,4 +232,6 @@ void migrate_send_rp_pong(MigrationIncomingState *mis,
>  void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname,
>                                ram_addr_t start, size_t len);
>  
> +void migrate_set_port(const uint16_t port, Error **errp);
> +
>  #endif
> diff --git a/migration/socket.c b/migration/socket.c
> index e090097077..08606c665d 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -15,6 +15,7 @@
>   */
>  
>  #include "qemu/osdep.h"
> +#include "qemu/cutils.h"
>  
>  #include "qemu-common.h"
>  #include "qemu/error-report.h"
> @@ -160,17 +161,24 @@ out:
>  }
>  
>  
> -static void socket_start_incoming_migration(SocketAddress *saddr,
> -                                            Error **errp)
> +static SocketAddress *socket_start_incoming_migration(SocketAddress *saddr,
> +                                                      Error **errp)
>  {
>      QIOChannelSocket *listen_ioc = qio_channel_socket_new();
> +    SocketAddress *address;
>  
>      qio_channel_set_name(QIO_CHANNEL(listen_ioc),
>                           "migration-socket-listener");
>  
>      if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
>          object_unref(OBJECT(listen_ioc));
> -        return;
> +        return NULL;
> +    }
> +
> +    address = qio_channel_socket_get_local_address(listen_ioc, errp);

[1]

> +    if (address < 0) {
> +        object_unref(OBJECT(listen_ioc));
> +        return NULL;
>      }
>  
>      qio_channel_add_watch(QIO_CHANNEL(listen_ioc),

[2]

> @@ -178,14 +186,28 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>                            socket_accept_incoming_migration,
>                            listen_ioc,
>                            (GDestroyNotify)object_unref);
> +    return address;
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
>  {
>      Error *err = NULL;
>      SocketAddress *saddr = tcp_build_address(host_port, &err);
> +
>      if (!err) {
> -        socket_start_incoming_migration(saddr, &err);
> +        SocketAddress *address = socket_start_incoming_migration(saddr, &err);
> +
> +        if (address) {
> +            unsigned long long port;
> +
> +            if (parse_uint_full(address->u.inet.port, &port, 10) < 0) {
> +                error_setg(errp, "error parsing port in '%s'",
> +                           address->u.inet.port);
> +            } else {
> +                migrate_set_port(port, errp);
> +            }

If *errp is non-NULL when reach here (I don't know when this will
happen, though), would there be a dangling incoming task in main
thread (which was created during socket_start_incoming_migration at
[2])?

Not sure whether it can be fixed by moving the set port logic to [1]
above.  Then the watch won't be created only if set port succeeded.

Thanks,

> +            qapi_free_SocketAddress(address);
> +        }
>      }
>      qapi_free_SocketAddress(saddr);
>      error_propagate(errp, err);
> @@ -194,6 +216,9 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
>  void unix_start_incoming_migration(const char *path, Error **errp)
>  {
>      SocketAddress *saddr = unix_build_address(path);
> -    socket_start_incoming_migration(saddr, errp);
> +    SocketAddress *address;
> +
> +    address = socket_start_incoming_migration(saddr, errp);
> +    qapi_free_SocketAddress(address);
>      qapi_free_SocketAddress(saddr);
>  }
> -- 
> 2.14.3
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH v5 2/5] migration: Set the migration tcp port

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> On Mon, Jan 29, 2018 at 01:17:51PM +0100, Juan Quintela wrote:
>> We can set the port parameter as zero.  This patch lets us know what
>> port the system was choosen for us.  Now we can migrate to this place.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> --
>> 
>> This was migrate_set_uri(), but as we only need the tcp_port, change
>> to that one.
>> ---
>>  migration/migration.c | 10 ++++++++++
>>  migration/migration.h |  2 ++
>>  migration/socket.c    | 35 ++++++++++++++++++++++++++++++-----
>>  3 files changed, 42 insertions(+), 5 deletions(-)
>> 
>> diff --git a/migration/migration.c b/migration/migration.c
>> index eb6958dcda..53818a87af 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -245,6 +245,16 @@ void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
>>      }
>>  }
>>  
>> +void migrate_set_port(const uint16_t port, Error **errp)
>> +{
>> +    MigrateSetParameters p = {
>> +        .has_x_tcp_port = true,
>> +        .x_tcp_port = port,
>> +    };
>> +
>> +    qmp_migrate_set_parameters(&p, errp);
>
> If we are calling qmp_migrate_set_parameters() here, does it mean that
> user can also set this parameter via QMP?

Yeap.  We do that, or we invent yet another mechanism to update the
tcp_port parameter :-(

You can't have both.

if the user modifies it, it just shots itself it its feet, no?

>>                           "migration-socket-listener");
>>  
>>      if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
>>          object_unref(OBJECT(listen_ioc));
>> -        return;
>> +        return NULL;
>> +    }
>> +
>> +    address = qio_channel_socket_get_local_address(listen_ioc, errp);
>
> [1]
>
>> +    if (address < 0) {
>> +        object_unref(OBJECT(listen_ioc));
>> +        return NULL;
>>      }
>>  
>>      qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
>
> [2]
>
>> @@ -178,14 +186,28 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>>                            socket_accept_incoming_migration,
>>                            listen_ioc,
>>                            (GDestroyNotify)object_unref);
>> +    return address;
>>  }
>>  
>>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
>>  {
>>      Error *err = NULL;
>>      SocketAddress *saddr = tcp_build_address(host_port, &err);
>> +
>>      if (!err) {
>> -        socket_start_incoming_migration(saddr, &err);
>> +        SocketAddress *address = socket_start_incoming_migration(saddr, &err);
>> +
>> +        if (address) {
>> +            unsigned long long port;
>> +
>> +            if (parse_uint_full(address->u.inet.port, &port, 10) < 0) {
>> +                error_setg(errp, "error parsing port in '%s'",
>> +                           address->u.inet.port);
>> +            } else {
>> +                migrate_set_port(port, errp);
>> +            }
>
> If *errp is non-NULL when reach here (I don't know when this will
> happen, though), would there be a dangling incoming task in main
> thread (which was created during socket_start_incoming_migration at
> [2])?
>
> Not sure whether it can be fixed by moving the set port logic to [1]
> above.  Then the watch won't be created only if set port succeeded.

Ok, will take a look at reorganizing the code.

Thanks for the review.

Later, Juan.

Re: [Qemu-devel] [PATCH v5 2/5] migration: Set the migration tcp port

[Peter Xu]

On Wed, Jan 31, 2018 at 01:35:33PM +0100, Juan Quintela wrote:
> Peter Xu <peterx@redhat.com> wrote:
> > On Mon, Jan 29, 2018 at 01:17:51PM +0100, Juan Quintela wrote:
> >> We can set the port parameter as zero.  This patch lets us know what
> >> port the system was choosen for us.  Now we can migrate to this place.
> >> 
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> 
> >> --
> >> 
> >> This was migrate_set_uri(), but as we only need the tcp_port, change
> >> to that one.
> >> ---
> >>  migration/migration.c | 10 ++++++++++
> >>  migration/migration.h |  2 ++
> >>  migration/socket.c    | 35 ++++++++++++++++++++++++++++++-----
> >>  3 files changed, 42 insertions(+), 5 deletions(-)
> >> 
> >> diff --git a/migration/migration.c b/migration/migration.c
> >> index eb6958dcda..53818a87af 100644
> >> --- a/migration/migration.c
> >> +++ b/migration/migration.c
> >> @@ -245,6 +245,16 @@ void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
> >>      }
> >>  }
> >>  
> >> +void migrate_set_port(const uint16_t port, Error **errp)
> >> +{
> >> +    MigrateSetParameters p = {
> >> +        .has_x_tcp_port = true,
> >> +        .x_tcp_port = port,
> >> +    };
> >> +
> >> +    qmp_migrate_set_parameters(&p, errp);
> >
> > If we are calling qmp_migrate_set_parameters() here, does it mean that
> > user can also set this parameter via QMP?
> 
> Yeap.  We do that, or we invent yet another mechanism to update the
> tcp_port parameter :-(
> 
> You can't have both.
> 
> if the user modifies it, it just shots itself it its feet, no?

How about set this directly? :)  Like:

  migrate_get_current()->parameters.x_tcp_port = xxx;

Maybe also add a comment showing that this is a special case.  I just
feel strange that if user can set it, but I'll follow your decision
even if you really want to keep the qmp_*() call since after all this
is for debugging, and QEMU itself won't use this value now.

Thanks,

-- 
Peter Xu