[Qemu-devel] [PATCH v7 0/8] Add make check tests for Migration

[Qemu-devel] [PATCH v7 0/8] Add make check tests for Migration

[Juan Quintela]

Hi

New in v8:
- SocketAddress is now a list (a.k.a make happy danp)
- Rebase on top of upstream
  network listener is *interesting*
- *HACK* to make SocketAddress list in common
  see patch: create-socket-paramenter

Please review, Juan.

CC: berrange@redhat.com

[v7]
This is v6, it differest from the patches that I sent with previous
multifd post:
- Rename x-tcp-port to x-socket-address
  This is more *complicated* that it looks as:

  * it is a pointer, so I need to use QAPI_CLONE() to make info
    migrate work

  * this is an union of structs.  In QAPI. So, a dict of strings.  The
    only way that I was able to make things work is parsing the qdict
    to a SocketAddress and then output a SocketAddress as an str.  It
    needs to be an easier way, for sure.

  * Cleanups here andthere.

Please, review, Juan.

[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 (8):
  qemu-sockets: Export SocketAddress_to_str
  tests: Add migration precopy test
  tests: Add migration xbzrle test
  migration: Create socket-address parameter
  tests: Migration ppc now inlines its program
  tests: Add basic migration precopy tcp test
  migration: Add multifd test
  [RFH] tests: Add migration compress threads tests

 chardev/char-socket.c  |  29 -----
 hmp.c                  |  14 +++
 include/qemu/sockets.h |   3 +
 migration/migration.c  |  25 ++++
 migration/migration.h  |   1 +
 migration/socket.c     |  11 ++
 qapi/migration.json    |  13 +-
 qapi/sockets.json      |  13 ++
 tests/migration-test.c | 332 +++++++++++++++++++++++++++++++++++++++++++------
 util/qemu-sockets.c    |  29 +++++
 10 files changed, 398 insertions(+), 72 deletions(-)

-- 
2.14.3

[Qemu-devel] [PATCH v7 1/8] qemu-sockets: Export SocketAddress_to_str

[Juan Quintela]

Migration code needs that function in hmp.c (so we need to export it),
and it needs it on tests/migration-test.c, so we need to move it to a
place where it is compiled into the test framework.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 chardev/char-socket.c  | 29 -----------------------------
 include/qemu/sockets.h |  3 +++
 util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 29 deletions(-)

diff --git a/chardev/char-socket.c b/chardev/char-socket.c
index 159e69c3b1..3bbf3a37a0 100644
--- a/chardev/char-socket.c
+++ b/chardev/char-socket.c
@@ -382,35 +382,6 @@ static void tcp_chr_free_connection(Chardev *chr)
     s->connected = 0;
 }
 
-static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
-                                  bool is_listen, bool is_telnet)
-{
-    switch (addr->type) {
-    case SOCKET_ADDRESS_TYPE_INET:
-        return g_strdup_printf("%s%s:%s:%s%s", prefix,
-                               is_telnet ? "telnet" : "tcp",
-                               addr->u.inet.host,
-                               addr->u.inet.port,
-                               is_listen ? ",server" : "");
-        break;
-    case SOCKET_ADDRESS_TYPE_UNIX:
-        return g_strdup_printf("%sunix:%s%s", prefix,
-                               addr->u.q_unix.path,
-                               is_listen ? ",server" : "");
-        break;
-    case SOCKET_ADDRESS_TYPE_FD:
-        return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
-                               is_listen ? ",server" : "");
-        break;
-    case SOCKET_ADDRESS_TYPE_VSOCK:
-        return g_strdup_printf("%svsock:%s:%s", prefix,
-                               addr->u.vsock.cid,
-                               addr->u.vsock.port);
-    default:
-        abort();
-    }
-}
-
 static void update_disconnected_filename(SocketChardev *s)
 {
     Chardev *chr = CHARDEV(s);
diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
index 8140fea685..efea0ea850 100644
--- a/include/qemu/sockets.h
+++ b/include/qemu/sockets.h
@@ -110,4 +110,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
  */
 SocketAddress *socket_address_flatten(SocketAddressLegacy *addr);
 
+char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
+                           bool is_listen, bool is_telnet);
+
 #endif /* QEMU_SOCKETS_H */
diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index 8bd8bb64eb..09f04bf76b 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -1333,3 +1333,32 @@ SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
 
     return addr;
 }
+
+char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
+                                  bool is_listen, bool is_telnet)
+{
+    switch (addr->type) {
+    case SOCKET_ADDRESS_TYPE_INET:
+        return g_strdup_printf("%s%s:%s:%s%s", prefix,
+                               is_telnet ? "telnet" : "tcp",
+                               addr->u.inet.host,
+                               addr->u.inet.port,
+                               is_listen ? ",server" : "");
+        break;
+    case SOCKET_ADDRESS_TYPE_UNIX:
+        return g_strdup_printf("%sunix:%s%s", prefix,
+                               addr->u.q_unix.path,
+                               is_listen ? ",server" : "");
+        break;
+    case SOCKET_ADDRESS_TYPE_FD:
+        return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
+                               is_listen ? ",server" : "");
+        break;
+    case SOCKET_ADDRESS_TYPE_VSOCK:
+        return g_strdup_printf("%svsock:%s:%s", prefix,
+                               addr->u.vsock.cid,
+                               addr->u.vsock.port);
+    default:
+        abort();
+    }
+}
-- 
2.14.3

[Qemu-devel] [PATCH v7 2/8] tests: Add migration precopy test

[Juan Quintela]

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 | 44 ++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 42 insertions(+), 2 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 422bf1afdf..834cdf50f2 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -524,7 +524,7 @@ static void test_deprecated(void)
     qtest_quit(from);
 }
 
-static void test_migrate(void)
+static void test_postcopy(void)
 {
     char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
     QTestState *from, *to;
@@ -595,6 +595,45 @@ static void test_baddest(void)
     test_migrate_end(from, to, false);
 }
 
+static void test_precopy_unix(void)
+{
+    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, uri, false);
+
+    /* 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");
+
+    migrate(from, uri);
+
+    wait_for_migration_pass(from);
+
+    /* 300 ms 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, true);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -614,9 +653,10 @@ int main(int argc, char **argv)
 
     module_call_init(MODULE_INIT_QOM);
 
-    qtest_add_func("/migration/postcopy/unix", test_migrate);
+    qtest_add_func("/migration/postcopy/unix", test_postcopy);
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/bad_dest", test_baddest);
+    qtest_add_func("/migration/precopy/unix", test_precopy_unix);
 
     ret = g_test_run();
 
-- 
2.14.3

[Qemu-devel] [PATCH v7 3/8] tests: Add migration xbzrle test

[Juan Quintela]

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

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 834cdf50f2..fd885ba909 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -512,6 +512,20 @@ static void deprecated_set_speed(QTestState *who, const char *value)
     migrate_check_parameter(who, "max-bandwidth", value);
 }
 
+static void deprecated_set_cache_size(QTestState *who, const char *value)
+{
+    QDict *rsp;
+    gchar *cmd;
+
+    cmd = g_strdup_printf("{ 'execute': 'migrate-set-cache-size',"
+                          "'arguments': { 'value': %s } }", value);
+    rsp = qtest_qmp(who, cmd);
+    g_free(cmd);
+    g_assert(qdict_haskey(rsp, "return"));
+    QDECREF(rsp);
+    migrate_check_parameter(who, "xbzrle-cache-size", value);
+}
+
 static void test_deprecated(void)
 {
     QTestState *from;
@@ -520,6 +534,7 @@ static void test_deprecated(void)
 
     deprecated_set_downtime(from, 0.12345);
     deprecated_set_speed(from, "12345");
+    deprecated_set_cache_size(from, "4096");
 
     qtest_quit(from);
 }
@@ -634,6 +649,54 @@ static void test_precopy_unix(void)
     g_free(uri);
 }
 
+static void test_xbzrle(const char *uri)
+{
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, uri, false);
+
+    /* 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");
+
+    migrate_set_parameter(from, "xbzrle-cache-size", "33554432");
+
+    migrate_set_capability(from, "xbzrle", "true");
+    migrate_set_capability(to, "xbzrle", "true");
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    migrate(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, true);
+}
+
+static void test_xbzrle_unix(void)
+{
+    char *uri = g_strdup_printf("unix:%s/migsocket", tmpfs);
+
+    test_xbzrle(uri);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -657,6 +720,7 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/bad_dest", test_baddest);
     qtest_add_func("/migration/precopy/unix", test_precopy_unix);
+    qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
 
     ret = g_test_run();
 
-- 
2.14.3

[Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Juan Quintela]

It will be used to store the uri parameters. We want this only for
tcp, so we don't set it for other uris.  We need it to know what port
is migration running.

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.

This used to be a port parameter.  I was asked to move to
SocketAddress, done.
I also merged the setting of the migration tcp port in this one
because now I need to free the address, and this makes it easier.
This used to be x-socket-address with a single direction, now it is a
list of addresses.
---
 hmp.c                 | 14 ++++++++++++++
 migration/migration.c | 25 +++++++++++++++++++++++++
 migration/migration.h |  1 +
 migration/socket.c    | 11 +++++++++++
 qapi/migration.json   | 13 +++++++++++--
 qapi/sockets.json     | 13 +++++++++++++
 6 files changed, 75 insertions(+), 2 deletions(-)

diff --git a/hmp.c b/hmp.c
index a25c7bd9a8..caf94345c9 100644
--- a/hmp.c
+++ b/hmp.c
@@ -355,6 +355,20 @@ 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);
+        if (params->has_socket_address) {
+            SocketAddressList *addr;
+
+            monitor_printf(mon, "%s: [\n",
+                MigrationParameter_str(MIGRATION_PARAMETER_SOCKET_ADDRESS));
+
+            for (addr = params->socket_address; addr; addr = addr->next) {
+                char *s = SocketAddress_to_str("", addr->value,
+                                               false, false);
+                monitor_printf(mon, "\t%s\n", s);
+            }
+
+            monitor_printf(mon, "]\n");
+        }
     }
 
     qapi_free_MigrationParameters(params);
diff --git a/migration/migration.c b/migration/migration.c
index 58bd382730..71fa4c8176 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -31,6 +31,8 @@
 #include "migration/vmstate.h"
 #include "block/block.h"
 #include "qapi/error.h"
+#include "qapi/clone-visitor.h"
+#include "qapi/qapi-visit-sockets.h"
 #include "qapi/qapi-commands-migration.h"
 #include "qapi/qapi-events-migration.h"
 #include "qapi/qmp/qerror.h"
@@ -277,6 +279,21 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
     return migrate_send_rp_message(mis, msg_type, msglen, bufc);
 }
 
+void migrate_set_address(SocketAddress *address)
+{
+    MigrationState *s = migrate_get_current();
+    SocketAddressList *addrs;
+
+    addrs = g_new0(SocketAddressList, 1);
+    addrs->next = s->parameters.socket_address;
+    s->parameters.socket_address = addrs;
+
+    if (!s->parameters.has_socket_address) {
+        s->parameters.has_socket_address = true;
+    }
+    addrs->value = QAPI_CLONE(SocketAddress, address);
+}
+
 void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
@@ -564,6 +581,11 @@ 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;
+    if (s->parameters.socket_address) {
+        params->has_socket_address = true;
+        params->socket_address =
+            QAPI_CLONE(SocketAddressList, s->parameters.socket_address);
+    }
 
     return params;
 }
@@ -2571,6 +2593,9 @@ static void migration_instance_finalize(Object *obj)
     qemu_mutex_destroy(&ms->error_mutex);
     g_free(params->tls_hostname);
     g_free(params->tls_creds);
+    if (params->socket_address) {
+        qapi_free_SocketAddressList(params->socket_address);
+    }
     qemu_sem_destroy(&ms->pause_sem);
     error_free(ms->error);
 }
diff --git a/migration/migration.h b/migration/migration.h
index 8d2f320c48..4774ee305f 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -241,5 +241,6 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname,
 
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
+void migrate_set_address(SocketAddress *address);
 
 #endif
diff --git a/migration/socket.c b/migration/socket.c
index 122d8ccfbe..5195fd57c5 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"
@@ -152,6 +153,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
                                             Error **errp)
 {
     QIONetListener *listener = qio_net_listener_new();
+    int i;
 
     qio_net_listener_set_name(listener, "migration-socket-listener");
 
@@ -163,6 +165,15 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
     qio_net_listener_set_client_func(listener,
                                      socket_accept_incoming_migration,
                                      NULL, NULL);
+
+    for (i = 0; i < listener->nsioc; i++)  {
+        SocketAddress *address =
+            qio_channel_socket_get_local_address(listener->sioc[i], errp);
+        if (address < 0) {
+            return;
+        }
+        migrate_set_address(address);
+    }
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
diff --git a/qapi/migration.json b/qapi/migration.json
index 9d0bf82cf4..c3aafaf8fe 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -6,6 +6,7 @@
 ##
 
 { 'include': 'common.json' }
+{ 'include': 'sockets.json' }
 
 ##
 # @MigrationStats:
@@ -494,6 +495,9 @@
 #                     and a power of 2
 #                     (Since 2.11)
 #
+# @socket-address: Only used for tcp, to know what the real port is
+#                  (Since 2.13)
+#
 # Since: 2.4
 ##
 { 'enum': 'MigrationParameter',
@@ -502,7 +506,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', 'socket-address' ] }
 
 ##
 # @MigrateSetParameters:
@@ -671,6 +675,10 @@
 #                     needs to be a multiple of the target page size
 #                     and a power of 2
 #                     (Since 2.11)
+#
+# @socket-address: Only used for tcp, to know what the real port is
+#                  (Since 2.13)
+#
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -687,7 +695,8 @@
             '*block-incremental': 'bool' ,
             '*x-multifd-channels': 'uint8',
             '*x-multifd-page-count': 'uint32',
-            '*xbzrle-cache-size': 'size' } }
+            '*xbzrle-cache-size': 'size',
+            '*socket-address': ['SocketAddress'] } }
 
 ##
 # @query-migrate-parameters:
diff --git a/qapi/sockets.json b/qapi/sockets.json
index fc81d8d5e8..f1ca09a927 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -152,3 +152,16 @@
             'unix': 'UnixSocketAddress',
             'vsock': 'VsockSocketAddress',
             'fd': 'String' } }
+
+##
+# @DummyStruct:
+#
+# Both block-core and migration needs SocketAddressList
+# I am open to comments about how to share it
+#
+# @dummy-list: A dummy list
+#
+# Since: 2.13
+##
+{ 'struct': 'DummyStruct',
+  'data': { 'dummy-list': ['SocketAddress'] } }
-- 
2.14.3

[Qemu-devel] [PATCH v7 5/8] 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 fd885ba909..4a94d3d598 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -19,9 +19,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;
@@ -90,36 +87,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
@@ -410,12 +377,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 v7 6/8] 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 | 83 ++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 80 insertions(+), 3 deletions(-)

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 4a94d3d598..309ae4793e 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -19,6 +19,9 @@
 #include "qemu/sockets.h"
 #include "chardev/char.h"
 #include "sysemu/sysemu.h"
+#include "qapi/qapi-visit-sockets.h"
+#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
 
 const unsigned start_address = 1024 * 1024;
 const unsigned end_address = 100 * 1024 * 1024;
@@ -277,8 +280,32 @@ 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_socket_address(QTestState *who, const char *parameter)
+{
+    QDict *rsp, *rsp_return;
+    char *result;
+    Error *local_err = NULL;
+    SocketAddressList *addrs;
+    Visitor *iv = NULL;
+    QObject *object;
+
+    rsp = wait_command(who, "{ 'execute': 'query-migrate-parameters' }");
+    rsp_return = qdict_get_qdict(rsp, "return");
+    object = qdict_get(rsp_return, parameter);
+
+    iv = qobject_input_visitor_new(object);
+    visit_type_SocketAddressList(iv, NULL, &addrs, &local_err);
+
+    /* we are only using a single address */
+    result = g_strdup_printf("%s", SocketAddress_to_str("", addrs->value,
+                                                        false, false));
+
+    qapi_free_SocketAddressList(addrs);
+    QDECREF(rsp);
+    return result;
+}
+
+static char *migrate_get_parameter(QTestState *who, const char *parameter)
 {
     QDict *rsp, *rsp_return;
     char *result;
@@ -287,9 +314,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,
@@ -666,6 +702,46 @@ static void test_xbzrle_unix(void)
     g_free(uri);
 }
 
+static void test_precopy_tcp(void)
+{
+    char *uri;
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, "tcp:127.0.0.1:0", false);
+
+    /* 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");
+
+    uri = migrate_get_socket_address(to, "socket-address");
+
+    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, true);
+    g_free(uri);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -689,6 +765,7 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/deprecated", test_deprecated);
     qtest_add_func("/migration/bad_dest", test_baddest);
     qtest_add_func("/migration/precopy/unix", test_precopy_unix);
+    qtest_add_func("/migration/precopy/tcp", test_precopy_tcp);
     qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
 
     ret = g_test_run();
-- 
2.14.3

[Qemu-devel] [PATCH v7 7/8] migration: Add multifd test

[Juan Quintela]

We set the x-multifd-page-count and x-multifd-channels.

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

diff --git a/tests/migration-test.c b/tests/migration-test.c
index 309ae4793e..873bb58200 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -742,6 +742,53 @@ static void test_precopy_tcp(void)
     g_free(uri);
 }
 
+static void test_multifd_tcp(void)
+{
+    char *uri;
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, "tcp:127.0.0.1:0", false);
+
+    /* 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");
+
+    migrate_set_parameter(from, "x-multifd-channels", "4");
+    migrate_set_parameter(to, "x-multifd-channels", "4");
+
+    migrate_set_parameter(from, "x-multifd-page-count", "64");
+    migrate_set_parameter(to, "x-multifd-page-count", "64");
+
+    migrate_set_capability(from, "x-multifd", "true");
+    migrate_set_capability(to, "x-multifd", "true");
+    /* Wait for the first serial output from the source */
+    wait_for_serial("src_serial");
+
+    uri = migrate_get_socket_address(to, "socket-address");
+
+    migrate(from, uri);
+
+    wait_for_migration_pass(from);
+
+    /* 300ms it 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, true);
+}
+
 int main(int argc, char **argv)
 {
     char template[] = "/tmp/migration-test-XXXXXX";
@@ -767,6 +814,7 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/precopy/unix", test_precopy_unix);
     qtest_add_func("/migration/precopy/tcp", test_precopy_tcp);
     qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
+    qtest_add_func("/migration/multifd/tcp", test_multifd_tcp);
 
     ret = g_test_run();
 
-- 
2.14.3

[Qemu-devel] [PATCH v7 8/8] [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 873bb58200..cf6cec11ef 100644
--- a/tests/migration-test.c
+++ b/tests/migration-test.c
@@ -789,6 +789,55 @@ static void test_multifd_tcp(void)
     test_migrate_end(from, to, true);
 }
 
+static void test_compress(const char *uri)
+{
+    QTestState *from, *to;
+
+    test_migrate_start(&from, &to, uri, false);
+
+    /* 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");
+
+    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 it 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, true);
+}
+
+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";
@@ -815,6 +864,9 @@ int main(int argc, char **argv)
     qtest_add_func("/migration/precopy/tcp", test_precopy_tcp);
     qtest_add_func("/migration/xbzrle/unix", test_xbzrle_unix);
     qtest_add_func("/migration/multifd/tcp", test_multifd_tcp);
+    if (0) {
+        qtest_add_func("/migration/compress/unix", test_compress_unix);
+    }
 
     ret = g_test_run();
 
-- 
2.14.3

Re: [Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> It will be used to store the uri parameters. We want this only for
> tcp, so we don't set it for other uris.  We need it to know what port
> is migration running.
> 
> 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.
> 
> This used to be a port parameter.  I was asked to move to
> SocketAddress, done.
> I also merged the setting of the migration tcp port in this one
> because now I need to free the address, and this makes it easier.
> This used to be x-socket-address with a single direction, now it is a
> list of addresses.

Is there a reason it's a parameter rather than just an entry in
MigrationInfo?

Dave

> ---
>  hmp.c                 | 14 ++++++++++++++
>  migration/migration.c | 25 +++++++++++++++++++++++++
>  migration/migration.h |  1 +
>  migration/socket.c    | 11 +++++++++++
>  qapi/migration.json   | 13 +++++++++++--
>  qapi/sockets.json     | 13 +++++++++++++
>  6 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index a25c7bd9a8..caf94345c9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -355,6 +355,20 @@ 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);
> +        if (params->has_socket_address) {
> +            SocketAddressList *addr;
> +
> +            monitor_printf(mon, "%s: [\n",
> +                MigrationParameter_str(MIGRATION_PARAMETER_SOCKET_ADDRESS));
> +
> +            for (addr = params->socket_address; addr; addr = addr->next) {
> +                char *s = SocketAddress_to_str("", addr->value,
> +                                               false, false);
> +                monitor_printf(mon, "\t%s\n", s);
> +            }
> +
> +            monitor_printf(mon, "]\n");
> +        }
>      }
>  
>      qapi_free_MigrationParameters(params);
> diff --git a/migration/migration.c b/migration/migration.c
> index 58bd382730..71fa4c8176 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -31,6 +31,8 @@
>  #include "migration/vmstate.h"
>  #include "block/block.h"
>  #include "qapi/error.h"
> +#include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-sockets.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-events-migration.h"
>  #include "qapi/qmp/qerror.h"
> @@ -277,6 +279,21 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
>      return migrate_send_rp_message(mis, msg_type, msglen, bufc);
>  }
>  
> +void migrate_set_address(SocketAddress *address)
> +{
> +    MigrationState *s = migrate_get_current();
> +    SocketAddressList *addrs;
> +
> +    addrs = g_new0(SocketAddressList, 1);
> +    addrs->next = s->parameters.socket_address;
> +    s->parameters.socket_address = addrs;
> +
> +    if (!s->parameters.has_socket_address) {
> +        s->parameters.has_socket_address = true;
> +    }
> +    addrs->value = QAPI_CLONE(SocketAddress, address);
> +}
> +
>  void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
> @@ -564,6 +581,11 @@ 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;
> +    if (s->parameters.socket_address) {
> +        params->has_socket_address = true;
> +        params->socket_address =
> +            QAPI_CLONE(SocketAddressList, s->parameters.socket_address);
> +    }
>  
>      return params;
>  }
> @@ -2571,6 +2593,9 @@ static void migration_instance_finalize(Object *obj)
>      qemu_mutex_destroy(&ms->error_mutex);
>      g_free(params->tls_hostname);
>      g_free(params->tls_creds);
> +    if (params->socket_address) {
> +        qapi_free_SocketAddressList(params->socket_address);
> +    }
>      qemu_sem_destroy(&ms->pause_sem);
>      error_free(ms->error);
>  }
> diff --git a/migration/migration.h b/migration/migration.h
> index 8d2f320c48..4774ee305f 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -241,5 +241,6 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname,
>  
>  void dirty_bitmap_mig_before_vm_start(void);
>  void init_dirty_bitmap_incoming_migration(void);
> +void migrate_set_address(SocketAddress *address);
>  
>  #endif
> diff --git a/migration/socket.c b/migration/socket.c
> index 122d8ccfbe..5195fd57c5 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"
> @@ -152,6 +153,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>                                              Error **errp)
>  {
>      QIONetListener *listener = qio_net_listener_new();
> +    int i;
>  
>      qio_net_listener_set_name(listener, "migration-socket-listener");
>  
> @@ -163,6 +165,15 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>      qio_net_listener_set_client_func(listener,
>                                       socket_accept_incoming_migration,
>                                       NULL, NULL);
> +
> +    for (i = 0; i < listener->nsioc; i++)  {
> +        SocketAddress *address =
> +            qio_channel_socket_get_local_address(listener->sioc[i], errp);
> +        if (address < 0) {
> +            return;
> +        }
> +        migrate_set_address(address);
> +    }
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9d0bf82cf4..c3aafaf8fe 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -6,6 +6,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @MigrationStats:
> @@ -494,6 +495,9 @@
>  #                     and a power of 2
>  #                     (Since 2.11)
>  #
> +# @socket-address: Only used for tcp, to know what the real port is
> +#                  (Since 2.13)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -502,7 +506,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', 'socket-address' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -671,6 +675,10 @@
>  #                     needs to be a multiple of the target page size
>  #                     and a power of 2
>  #                     (Since 2.11)
> +#
> +# @socket-address: Only used for tcp, to know what the real port is
> +#                  (Since 2.13)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -687,7 +695,8 @@
>              '*block-incremental': 'bool' ,
>              '*x-multifd-channels': 'uint8',
>              '*x-multifd-page-count': 'uint32',
> -            '*xbzrle-cache-size': 'size' } }
> +            '*xbzrle-cache-size': 'size',
> +            '*socket-address': ['SocketAddress'] } }
>  
>  ##
>  # @query-migrate-parameters:
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index fc81d8d5e8..f1ca09a927 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -152,3 +152,16 @@
>              'unix': 'UnixSocketAddress',
>              'vsock': 'VsockSocketAddress',
>              'fd': 'String' } }
> +
> +##
> +# @DummyStruct:
> +#
> +# Both block-core and migration needs SocketAddressList
> +# I am open to comments about how to share it
> +#
> +# @dummy-list: A dummy list
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'DummyStruct',
> +  'data': { 'dummy-list': ['SocketAddress'] } }
> -- 
> 2.14.3
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH v7 1/8] qemu-sockets: Export SocketAddress_to_str

[Daniel P. Berrangé]

On Wed, Apr 04, 2018 at 01:27:24PM +0200, Juan Quintela wrote:
> Migration code needs that function in hmp.c (so we need to export it),
> and it needs it on tests/migration-test.c, so we need to move it to a
> place where it is compiled into the test framework.
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  chardev/char-socket.c  | 29 -----------------------------
>  include/qemu/sockets.h |  3 +++
>  util/qemu-sockets.c    | 29 +++++++++++++++++++++++++++++
>  3 files changed, 32 insertions(+), 29 deletions(-)
> 
> diff --git a/chardev/char-socket.c b/chardev/char-socket.c
> index 159e69c3b1..3bbf3a37a0 100644
> --- a/chardev/char-socket.c
> +++ b/chardev/char-socket.c
> @@ -382,35 +382,6 @@ static void tcp_chr_free_connection(Chardev *chr)
>      s->connected = 0;
>  }
>  
> -static char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
> -                                  bool is_listen, bool is_telnet)
> -{
> -    switch (addr->type) {
> -    case SOCKET_ADDRESS_TYPE_INET:
> -        return g_strdup_printf("%s%s:%s:%s%s", prefix,
> -                               is_telnet ? "telnet" : "tcp",
> -                               addr->u.inet.host,
> -                               addr->u.inet.port,
> -                               is_listen ? ",server" : "");
> -        break;
> -    case SOCKET_ADDRESS_TYPE_UNIX:
> -        return g_strdup_printf("%sunix:%s%s", prefix,
> -                               addr->u.q_unix.path,
> -                               is_listen ? ",server" : "");
> -        break;
> -    case SOCKET_ADDRESS_TYPE_FD:
> -        return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
> -                               is_listen ? ",server" : "");
> -        break;
> -    case SOCKET_ADDRESS_TYPE_VSOCK:
> -        return g_strdup_printf("%svsock:%s:%s", prefix,
> -                               addr->u.vsock.cid,
> -                               addr->u.vsock.port);
> -    default:
> -        abort();
> -    }
> -}
> -
>  static void update_disconnected_filename(SocketChardev *s)
>  {
>      Chardev *chr = CHARDEV(s);
> diff --git a/include/qemu/sockets.h b/include/qemu/sockets.h
> index 8140fea685..efea0ea850 100644
> --- a/include/qemu/sockets.h
> +++ b/include/qemu/sockets.h
> @@ -110,4 +110,7 @@ SocketAddress *socket_remote_address(int fd, Error **errp);
>   */
>  SocketAddress *socket_address_flatten(SocketAddressLegacy *addr);
>  
> +char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
> +                           bool is_listen, bool is_telnet);
> +
>  #endif /* QEMU_SOCKETS_H */
> diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
> index 8bd8bb64eb..09f04bf76b 100644
> --- a/util/qemu-sockets.c
> +++ b/util/qemu-sockets.c
> @@ -1333,3 +1333,32 @@ SocketAddress *socket_address_flatten(SocketAddressLegacy *addr_legacy)
>  
>      return addr;
>  }
> +
> +char *SocketAddress_to_str(const char *prefix, SocketAddress *addr,
> +                                  bool is_listen, bool is_telnet)
> +{
> +    switch (addr->type) {
> +    case SOCKET_ADDRESS_TYPE_INET:
> +        return g_strdup_printf("%s%s:%s:%s%s", prefix,
> +                               is_telnet ? "telnet" : "tcp",
> +                               addr->u.inet.host,
> +                               addr->u.inet.port,
> +                               is_listen ? ",server" : "");
> +        break;
> +    case SOCKET_ADDRESS_TYPE_UNIX:
> +        return g_strdup_printf("%sunix:%s%s", prefix,
> +                               addr->u.q_unix.path,
> +                               is_listen ? ",server" : "");
> +        break;
> +    case SOCKET_ADDRESS_TYPE_FD:
> +        return g_strdup_printf("%sfd:%s%s", prefix, addr->u.fd.str,
> +                               is_listen ? ",server" : "");
> +        break;
> +    case SOCKET_ADDRESS_TYPE_VSOCK:
> +        return g_strdup_printf("%svsock:%s:%s", prefix,
> +                               addr->u.vsock.cid,
> +                               addr->u.vsock.port);
> +    default:
> +        abort();
> +    }
> +}

This code really does not belong in qemu-sockets.h - the syntax being
printed here is the chardev specific syntax, hence those is_listen and
is_telnet parameters. If we want a way to string-ify SocketAddress then
it should be a generic format, not the chardev format.

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Daniel P. Berrangé]

On Wed, Apr 04, 2018 at 01:27:27PM +0200, Juan Quintela wrote:
> It will be used to store the uri parameters. We want this only for
> tcp, so we don't set it for other uris.  We need it to know what port
> is migration running.
> 
> 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.
> 
> This used to be a port parameter.  I was asked to move to
> SocketAddress, done.
> I also merged the setting of the migration tcp port in this one
> because now I need to free the address, and this makes it easier.
> This used to be x-socket-address with a single direction, now it is a
> list of addresses.
> ---
>  hmp.c                 | 14 ++++++++++++++
>  migration/migration.c | 25 +++++++++++++++++++++++++
>  migration/migration.h |  1 +
>  migration/socket.c    | 11 +++++++++++
>  qapi/migration.json   | 13 +++++++++++--
>  qapi/sockets.json     | 13 +++++++++++++
>  6 files changed, 75 insertions(+), 2 deletions(-)
> 
> diff --git a/hmp.c b/hmp.c
> index a25c7bd9a8..caf94345c9 100644
> --- a/hmp.c
> +++ b/hmp.c
> @@ -355,6 +355,20 @@ 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);
> +        if (params->has_socket_address) {
> +            SocketAddressList *addr;
> +
> +            monitor_printf(mon, "%s: [\n",
> +                MigrationParameter_str(MIGRATION_PARAMETER_SOCKET_ADDRESS));
> +
> +            for (addr = params->socket_address; addr; addr = addr->next) {
> +                char *s = SocketAddress_to_str("", addr->value,
> +                                               false, false);
> +                monitor_printf(mon, "\t%s\n", s);
> +            }
> +
> +            monitor_printf(mon, "]\n");
> +        }
>      }
>  
>      qapi_free_MigrationParameters(params);
> diff --git a/migration/migration.c b/migration/migration.c
> index 58bd382730..71fa4c8176 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -31,6 +31,8 @@
>  #include "migration/vmstate.h"
>  #include "block/block.h"
>  #include "qapi/error.h"
> +#include "qapi/clone-visitor.h"
> +#include "qapi/qapi-visit-sockets.h"
>  #include "qapi/qapi-commands-migration.h"
>  #include "qapi/qapi-events-migration.h"
>  #include "qapi/qmp/qerror.h"
> @@ -277,6 +279,21 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
>      return migrate_send_rp_message(mis, msg_type, msglen, bufc);
>  }
>  
> +void migrate_set_address(SocketAddress *address)

s/set/add/ in the method name since you're expecting this to be called
multiple times, augmenting the value, not replacing it.

> +{
> +    MigrationState *s = migrate_get_current();
> +    SocketAddressList *addrs;
> +
> +    addrs = g_new0(SocketAddressList, 1);
> +    addrs->next = s->parameters.socket_address;
> +    s->parameters.socket_address = addrs;
> +
> +    if (!s->parameters.has_socket_address) {
> +        s->parameters.has_socket_address = true;
> +    }
> +    addrs->value = QAPI_CLONE(SocketAddress, address);
> +}
> +
>  void qemu_start_incoming_migration(const char *uri, Error **errp)
>  {
>      const char *p;
> @@ -564,6 +581,11 @@ 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;
> +    if (s->parameters.socket_address) {

nitpick, should really check  s->parameters.has_socket_address

> +        params->has_socket_address = true;
> +        params->socket_address =
> +            QAPI_CLONE(SocketAddressList, s->parameters.socket_address);
> +    }
>  
>      return params;
>  }
> @@ -2571,6 +2593,9 @@ static void migration_instance_finalize(Object *obj)
>      qemu_mutex_destroy(&ms->error_mutex);
>      g_free(params->tls_hostname);
>      g_free(params->tls_creds);
> +    if (params->socket_address) {
> +        qapi_free_SocketAddressList(params->socket_address);
> +    }
>      qemu_sem_destroy(&ms->pause_sem);
>      error_free(ms->error);
>  }
> diff --git a/migration/migration.h b/migration/migration.h
> index 8d2f320c48..4774ee305f 100644
> --- a/migration/migration.h
> +++ b/migration/migration.h
> @@ -241,5 +241,6 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char* rbname,
>  
>  void dirty_bitmap_mig_before_vm_start(void);
>  void init_dirty_bitmap_incoming_migration(void);
> +void migrate_set_address(SocketAddress *address);
>  
>  #endif
> diff --git a/migration/socket.c b/migration/socket.c
> index 122d8ccfbe..5195fd57c5 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"
> @@ -152,6 +153,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>                                              Error **errp)
>  {
>      QIONetListener *listener = qio_net_listener_new();
> +    int i;

Nitpick   size_t for array indexes

>  
>      qio_net_listener_set_name(listener, "migration-socket-listener");
>  
> @@ -163,6 +165,15 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>      qio_net_listener_set_client_func(listener,
>                                       socket_accept_incoming_migration,
>                                       NULL, NULL);
> +
> +    for (i = 0; i < listener->nsioc; i++)  {
> +        SocketAddress *address =
> +            qio_channel_socket_get_local_address(listener->sioc[i], errp);
> +        if (address < 0) {

'address' is a pointer, so comparing to ' < 0' is wrong - I'm surprised
the compiler isn't issuing a warning about this.

> +            return;
> +        }
> +        migrate_set_address(address);
> +    }
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 9d0bf82cf4..c3aafaf8fe 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -6,6 +6,7 @@
>  ##
>  
>  { 'include': 'common.json' }
> +{ 'include': 'sockets.json' }
>  
>  ##
>  # @MigrationStats:
> @@ -494,6 +495,9 @@
>  #                     and a power of 2
>  #                     (Since 2.11)
>  #
> +# @socket-address: Only used for tcp, to know what the real port is
> +#                  (Since 2.13)
> +#
>  # Since: 2.4
>  ##
>  { 'enum': 'MigrationParameter',
> @@ -502,7 +506,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', 'socket-address' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -671,6 +675,10 @@
>  #                     needs to be a multiple of the target page size
>  #                     and a power of 2
>  #                     (Since 2.11)
> +#
> +# @socket-address: Only used for tcp, to know what the real port is
> +#                  (Since 2.13)
> +#
>  # Since: 2.4
>  ##
>  { 'struct': 'MigrationParameters',
> @@ -687,7 +695,8 @@
>              '*block-incremental': 'bool' ,
>              '*x-multifd-channels': 'uint8',
>              '*x-multifd-page-count': 'uint32',
> -            '*xbzrle-cache-size': 'size' } }
> +            '*xbzrle-cache-size': 'size',
> +            '*socket-address': ['SocketAddress'] } }
>  
>  ##
>  # @query-migrate-parameters:
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index fc81d8d5e8..f1ca09a927 100644
> --- a/qapi/sockets.json
> +++ b/qapi/sockets.json
> @@ -152,3 +152,16 @@
>              'unix': 'UnixSocketAddress',
>              'vsock': 'VsockSocketAddress',
>              'fd': 'String' } }
> +
> +##
> +# @DummyStruct:
> +#
> +# Both block-core and migration needs SocketAddressList
> +# I am open to comments about how to share it

What's the actual problem you're addressing with this ?

> +#
> +# @dummy-list: A dummy list
> +#
> +# Since: 2.13
> +##
> +{ 'struct': 'DummyStruct',
> +  'data': { 'dummy-list': ['SocketAddress'] } }
> -- 
> 2.14.3
> 
> 

Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Daniel P. Berrangé]

On Thu, Apr 12, 2018 at 02:09:33PM +0100, Dr. David Alan Gilbert wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
> > It will be used to store the uri parameters. We want this only for
> > tcp, so we don't set it for other uris.  We need it to know what port
> > is migration running.
> > 
> > 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.
> > 
> > This used to be a port parameter.  I was asked to move to
> > SocketAddress, done.
> > I also merged the setting of the migration tcp port in this one
> > because now I need to free the address, and this makes it easier.
> > This used to be x-socket-address with a single direction, now it is a
> > list of addresses.
> 
> Is there a reason it's a parameter rather than just an entry in
> MigrationInfo?

I imagine it is a side-effect of earlier versions of this patch
which used the migration parameter setter method. Having it in
the MigrationInfo does seem reasonable to me since this is not
a tunable parameter.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

Re: [Qemu-devel] [PATCH v7 1/8] qemu-sockets: Export SocketAddress_to_str

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Wed, Apr 04, 2018 at 01:27:24PM +0200, Juan Quintela wrote:
>> Migration code needs that function in hmp.c (so we need to export it),
>> and it needs it on tests/migration-test.c, so we need to move it to a
>> place where it is compiled into the test framework.
>
> This code really does not belong in qemu-sockets.h - the syntax being
> printed here is the chardev specific syntax, hence those is_listen and
> is_telnet parameters. If we want a way to string-ify SocketAddress then
> it should be a generic format, not the chardev format.

Hi

Dropeed.  Don't make sense to try to generalize it for two users.  Just
copy to hmp.c one simplified copy.

Thanks, Juan.

Re: [Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Juan Quintela]

"Dr. David Alan Gilbert" <dgilbert@redhat.com> wrote:
> * Juan Quintela (quintela@redhat.com) wrote:
>> It will be used to store the uri parameters. We want this only for
>> tcp, so we don't set it for other uris.  We need it to know what port
>> is migration running.
>> 
>> 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.
>> 
>> This used to be a port parameter.  I was asked to move to
>> SocketAddress, done.
>> I also merged the setting of the migration tcp port in this one
>> because now I need to free the address, and this makes it easier.
>> This used to be x-socket-address with a single direction, now it is a
>> list of addresses.
>
> Is there a reason it's a parameter rather than just an entry in
> MigrationInfo?

Done.

I think that we *should* create a migration instance also on destination
side, it is so much different adding things one side than the other.

Later, Juan.

Re: [Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Thu, Apr 12, 2018 at 02:09:33PM +0100, Dr. David Alan Gilbert wrote:
>> * Juan Quintela (quintela@redhat.com) wrote:
>> > It will be used to store the uri parameters. We want this only for
>> > tcp, so we don't set it for other uris.  We need it to know what port
>> > is migration running.
>> > 
>> > 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.
>> > 
>> > This used to be a port parameter.  I was asked to move to
>> > SocketAddress, done.
>> > I also merged the setting of the migration tcp port in this one
>> > because now I need to free the address, and this makes it easier.
>> > This used to be x-socket-address with a single direction, now it is a
>> > list of addresses.
>> 
>> Is there a reason it's a parameter rather than just an entry in
>> MigrationInfo?
>
> I imagine it is a side-effect of earlier versions of this patch
> which used the migration parameter setter method. Having it in
> the MigrationInfo does seem reasonable to me since this is not
> a tunable parameter.

Yeap, that was the reason, as said on previous email, changed.

Thanks to both, Juan.

Re: [Qemu-devel] [PATCH v7 4/8] migration: Create socket-address parameter

[Juan Quintela]

Daniel P. Berrangé <berrange@redhat.com> wrote:
> On Wed, Apr 04, 2018 at 01:27:27PM +0200, Juan Quintela wrote:
>> It will be used to store the uri parameters. We want this only for
>> tcp, so we don't set it for other uris.  We need it to know what port
>> is migration running.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> @@ -277,6 +279,21 @@ int migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
>>      return migrate_send_rp_message(mis, msg_type, msglen, bufc);
>>  }
>>  
>> +void migrate_set_address(SocketAddress *address)
>
> s/set/add/ in the method name since you're expecting this to be called
> multiple times, augmenting the value, not replacing it.

Makes sense, done.

>> @@ -564,6 +581,11 @@ 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;
>> +    if (s->parameters.socket_address) {
>
> nitpick, should really check  s->parameters.has_socket_address

Changed to state, no has_socket_address on mis-> anymore (if
socket_address != NULL, we know there are some)


>> @@ -152,6 +153,7 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>>                                              Error **errp)
>>  {
>>      QIONetListener *listener = qio_net_listener_new();
>> +    int i;
>
> Nitpick   size_t for array indexes

changed.

>>  
>>      qio_net_listener_set_name(listener, "migration-socket-listener");
>>  
>> @@ -163,6 +165,15 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>>      qio_net_listener_set_client_func(listener,
>>                                       socket_accept_incoming_migration,
>>                                       NULL, NULL);
>> +
>> +    for (i = 0; i < listener->nsioc; i++)  {
>> +        SocketAddress *address =
>> +            qio_channel_socket_get_local_address(listener->sioc[i], errp);
>> +        if (address < 0) {
>
> 'address' is a pointer, so comparing to ' < 0' is wrong - I'm surprised
> the compiler isn't issuing a warning about this.

Oops, I guess a copy & paste from other error check.  No real clue why
compiler didn't detect it either.
>> +
>> +##
>> +# @DummyStruct:
>> +#
>> +# Both block-core and migration needs SocketAddressList
>> +# I am open to comments about how to share it
>
> What's the actual problem you're addressing with this ?

Until now, List of SocketAddress were onl used on block-core.  QAPI
generator is able to cope with that.  Now, we need to use them on both
migration.json and block-core.json.  QAPI generator is smart enough to
detect that SocketAddressList templates have already been generated, but
it is not able to use them on both sides.  if I put them in a common
place that are included from both .json, it is just generated correctly,
only once, and it compiles.

I haven't been able to find a way to convince qapi generator that I just
want it to generate the code here, without creating something that uses
it.

I am open to changes.

Thanks, Juan.

>
>> +#
>> +# @dummy-list: A dummy list
>> +#
>> +# Since: 2.13
>> +##
>> +{ 'struct': 'DummyStruct',
>> +  'data': { 'dummy-list': ['SocketAddress'] } }
>> -- 
>> 2.14.3
>> 
>> 
>
> Regards,
> Daniel