[Qemu-devel] [PATCH 0/6] Improve info migrate output on destination

[Qemu-devel] [PATCH 0/6] Improve info migrate output on destination

[Juan Quintela]

Hi

This series (on top of my last pull requset) do:

- Make update port for address work for port 0.
  Right now we only update the _port_ that we ask for, not the one that we receive.
- Print global options as everything else (i.e. one by line, and on/off)
- Move addr cleanup to the place where we created it
- Create an uri migration parameter
- Fill the migrate parameter and set it.
- make the uri parameter optional for migrate

Please review.

Later, Juan.

Juan Quintela (6):
  qio: Make port 0 work for qio
  migration: print features as on off
  migration: free addr in the same function that we created it
  migration: Create uri parameter
  migration: Now set the migration uri
  migration: make migrate uri parameter optional

 hmp-commands.hx       |  4 ++--
 hmp.c                 | 12 +++++++++--
 migration/migration.c | 59 ++++++++++++++++++++++++++++++++++++++++-----------
 migration/migration.h |  2 ++
 migration/socket.c    | 11 ++++++++--
 qapi/migration.json   | 20 +++++++++++++----
 util/qemu-sockets.c   | 11 ++++++----
 7 files changed, 93 insertions(+), 26 deletions(-)

-- 
2.13.6

[Qemu-devel] [PATCH 1/6] qio: Make port 0 work for qio

[Juan Quintela]

For tcp sockets we read back what is the socket/address.  So we know
what is the port that we are listening into.

Looked all callers of the function, and they just create the addr, use
it, and drop it, so no problem that we always update the port in the
address.

Signed-off-by: Juan Quintela <quintela@redhat.com>
CC: Daniel P. Berrange <berrange@redhat.com>
---
 util/qemu-sockets.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/util/qemu-sockets.c b/util/qemu-sockets.c
index b47fb45885..b099c88dd1 100644
--- a/util/qemu-sockets.c
+++ b/util/qemu-sockets.c
@@ -322,13 +322,16 @@ listen_failed:
 
 listen_ok:
     if (update_addr) {
+        SocketAddress *addr;
+
+        addr = socket_local_address(slisten, errp);
         g_free(saddr->host);
-        saddr->host = g_strdup(uaddr);
+        saddr->host = g_strdup(addr->u.inet.host);
         g_free(saddr->port);
-        saddr->port = g_strdup_printf("%d",
-                                      inet_getport(e) - port_offset);
+        saddr->port = g_strdup(addr->u.inet.port);
         saddr->has_ipv6 = saddr->ipv6 = e->ai_family == PF_INET6;
         saddr->has_ipv4 = saddr->ipv4 = e->ai_family != PF_INET6;
+        qapi_free_SocketAddress(addr);
     }
     freeaddrinfo(res);
     return slisten;
@@ -1047,7 +1050,7 @@ int socket_listen(SocketAddress *addr, Error **errp)
 
     switch (addr->type) {
     case SOCKET_ADDRESS_TYPE_INET:
-        fd = inet_listen_saddr(&addr->u.inet, 0, false, errp);
+        fd = inet_listen_saddr(&addr->u.inet, 0, true, errp);
         break;
 
     case SOCKET_ADDRESS_TYPE_UNIX:
-- 
2.13.6

[Qemu-devel] [PATCH 2/6] migration: print features as on off

[Juan Quintela]

Once there, do one thing for line

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 13 +++++++++----
 1 file changed, 9 insertions(+), 4 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 4de3b551fe..706d2407ec 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2375,10 +2375,15 @@ void migration_global_dump(Monitor *mon)
 {
     MigrationState *ms = migrate_get_current();
 
-    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
-                   "send-configuration=%d, send-section-footer=%d\n",
-                   ms->store_global_state, ms->only_migratable,
-                   ms->send_configuration, ms->send_section_footer);
+    monitor_printf(mon, "globals:\n");
+    monitor_printf(mon, "store-global-state: %s\n",
+                   ms->store_global_state ? "on" : "off");
+    monitor_printf(mon, "only-migratable: %s\n",
+                   ms->only_migratable ? "on" : "off");
+    monitor_printf(mon, "send-configuration: %s\n",
+                   ms->send_configuration ? "on" : "off");
+    monitor_printf(mon, "send-section-footer: %s\n",
+                   ms->send_section_footer ? "on" : "off");
 }
 
 #define DEFINE_PROP_MIG_CAP(name, x)             \
-- 
2.13.6

[Qemu-devel] [PATCH 3/6] migration: free addr in the same function that we created it

[Juan Quintela]

Otherwise, we can't use it after calling socket_start_incoming_migration

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/socket.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/migration/socket.c b/migration/socket.c
index dee869044a..3a8232dd2d 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -172,7 +172,6 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
 
     if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
         object_unref(OBJECT(listen_ioc));
-        qapi_free_SocketAddress(saddr);
         return;
     }
 
@@ -181,7 +180,6 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
                           socket_accept_incoming_migration,
                           listen_ioc,
                           (GDestroyNotify)object_unref);
-    qapi_free_SocketAddress(saddr);
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
@@ -191,6 +189,7 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
     if (!err) {
         socket_start_incoming_migration(saddr, &err);
     }
+    qapi_free_SocketAddress(saddr);
     error_propagate(errp, err);
 }
 
@@ -198,4 +197,5 @@ void unix_start_incoming_migration(const char *path, Error **errp)
 {
     SocketAddress *saddr = unix_build_address(path);
     socket_start_incoming_migration(saddr, errp);
+    qapi_free_SocketAddress(saddr);
 }
-- 
2.13.6

[Qemu-devel] [PATCH 4/6] migration: Create uri parameter

[Juan Quintela]

It will be used to store the uri migration parameter.  Right now it is
just the parameter code.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp.c                 |  7 +++++++
 migration/migration.c | 13 +++++++++++++
 qapi/migration.json   | 18 +++++++++++++++---
 3 files changed, 35 insertions(+), 3 deletions(-)

diff --git a/hmp.c b/hmp.c
index a01be50daa..9c544560bb 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: %" PRId64 "\n",
             MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE),
             params->xbzrle_cache_size);
+        monitor_printf(mon, "%s: %s\n",
+            MigrationParameter_str(MIGRATION_PARAMETER_URI),
+            params->uri);
     }
 
     qapi_free_MigrationParameters(params);
@@ -1667,6 +1670,10 @@ void hmp_migrate_set_parameter(Monitor *mon, const QDict *qdict)
         }
         p->xbzrle_cache_size = cache_size;
         break;
+    case MIGRATION_PARAMETER_URI:
+        p->has_uri = true;
+        visit_type_str(v, param, &p->uri, &err);
+        break;
     default:
         assert(0);
     }
diff --git a/migration/migration.c b/migration/migration.c
index 706d2407ec..3e48d37880 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -517,6 +517,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_uri = true;
+    params->uri = g_strdup(s->parameters.uri);
 
     return params;
 }
@@ -893,6 +895,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_uri) {
+        dest->uri = g_strdup(params->uri);
+    }
 }
 
 static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
@@ -965,6 +970,10 @@ 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_uri) {
+        g_free(s->parameters.uri);
+        s->parameters.uri = g_strdup(params->uri);
+    }
 }
 
 void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
@@ -2431,6 +2440,8 @@ static Property migration_properties[] = {
     DEFINE_PROP_SIZE("xbzrle-cache-size", MigrationState,
                       parameters.xbzrle_cache_size,
                       DEFAULT_MIGRATE_XBZRLE_CACHE_SIZE),
+    DEFINE_PROP_STRING("x-uri", MigrationState,
+                       parameters.uri),
 
     /* Migration capabilities */
     DEFINE_PROP_MIG_CAP("x-xbzrle", MIGRATION_CAPABILITY_XBZRLE),
@@ -2465,6 +2476,7 @@ static void migration_instance_finalize(Object *obj)
     qemu_mutex_destroy(&ms->error_mutex);
     g_free(params->tls_hostname);
     g_free(params->tls_creds);
+    g_free(params->uri);
     qemu_sem_destroy(&ms->pause_sem);
 }
 
@@ -2480,6 +2492,7 @@ static void migration_instance_init(Object *obj)
 
     params->tls_hostname = g_strdup("");
     params->tls_creds = g_strdup("");
+    params->uri = g_strdup("");
 
     /* Set has_* up only for parameter checks */
     params->has_compress_level = true;
diff --git a/qapi/migration.json b/qapi/migration.json
index bbc4671ded..ebde890604 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -488,6 +488,9 @@
 #                     and a power of 2
 #                     (Since 2.11)
 #
+# @uri: defines the method we are susing to migrate
+#                     (Since 2.11)
+#
 # 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', 'uri' ] }
 
 ##
 # @MigrateSetParameters:
@@ -564,6 +567,10 @@
 #                     needs to be a multiple of the target page size
 #                     and a power of 2
 #                     (Since 2.11)
+#
+# @uri: defines the method we are susing to migrate
+#                     (Since 2.11)
+#
 # 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' ,
+            '*uri': 'str'} }
 
 ##
 # @migrate-set-parameters:
@@ -665,6 +673,9 @@
 #                     needs to be a multiple of the target page size
 #                     and a power of 2
 #                     (Since 2.11)
+#
+# @uri: defines the method we are susing to migrate
+#                     (Since 2.11)
 # Since: 2.4
 ##
 { 'struct': 'MigrationParameters',
@@ -681,7 +692,8 @@
             '*block-incremental': 'bool' ,
             '*x-multifd-channels': 'int',
             '*x-multifd-page-count': 'int',
-            '*xbzrle-cache-size': 'size' } }
+            '*xbzrle-cache-size': 'size' ,
+            '*uri': 'str'} }
 
 ##
 # @query-migrate-parameters:
-- 
2.13.6

[Qemu-devel] [PATCH 5/6] migration: Now set the migration uri

[Juan Quintela]

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 migration/migration.c | 25 ++++++++++++++++++-------
 migration/migration.h |  2 ++
 migration/socket.c    |  7 +++++++
 3 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index 3e48d37880..507226907b 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -240,10 +240,21 @@ void migrate_send_rp_req_pages(MigrationIncomingState *mis, const char *rbname,
     }
 }
 
+void migrate_set_uri(const char *uri, Error **errp)
+{
+    MigrateSetParameters p = {
+        .has_uri = true,
+        .uri = (char *)uri,
+    };
+
+    qmp_migrate_set_parameters(&p, errp);
+}
+
 void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
 
+    migrate_set_uri(uri, errp);
     qapi_event_send_migration(MIGRATION_STATUS_SETUP, &error_abort);
     if (!strcmp(uri, "defer")) {
         deferred_incoming_migration(errp);
@@ -1363,7 +1374,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         error_setg(errp, "Guest is waiting for an incoming migration");
         return;
     }
-
+    migrate_set_uri(uri, errp);
     if (migration_is_blocked(errp)) {
         return;
     }
@@ -1388,20 +1399,20 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
 
     s = migrate_init();
 
-    if (strstart(uri, "tcp:", &p)) {
+    if (strstart(s->parameters.uri, "tcp:", &p)) {
         tcp_start_outgoing_migration(s, p, &local_err);
 #ifdef CONFIG_RDMA
-    } else if (strstart(uri, "rdma:", &p)) {
+    } else if (strstart(s->parameters.uri, "rdma:", &p)) {
         rdma_start_outgoing_migration(s, p, &local_err);
 #endif
-    } else if (strstart(uri, "exec:", &p)) {
+    } else if (strstart(s->parameters.uri, "exec:", &p)) {
         exec_start_outgoing_migration(s, p, &local_err);
-    } else if (strstart(uri, "unix:", &p)) {
+    } else if (strstart(s->parameters.uri, "unix:", &p)) {
         unix_start_outgoing_migration(s, p, &local_err);
-    } else if (strstart(uri, "fd:", &p)) {
+    } else if (strstart(s->parameters.uri, "fd:", &p)) {
         fd_start_outgoing_migration(s, p, &local_err);
     } else {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "uri",
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "s->parameters.uri",
                    "a valid migration protocol");
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_FAILED);
diff --git a/migration/migration.h b/migration/migration.h
index 663415fe48..cb0ab4807a 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -210,4 +210,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_uri(const char *uri, Error **errp);
+
 #endif
diff --git a/migration/socket.c b/migration/socket.c
index 3a8232dd2d..c3ab81d1fb 100644
--- a/migration/socket.c
+++ b/migration/socket.c
@@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
     Error *err = NULL;
     SocketAddress *saddr = tcp_build_address(host_port, &err);
     if (!err) {
+        char *new_uri;
         socket_start_incoming_migration(saddr, &err);
+        if (!err) {
+            new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host,
+                                      saddr->u.inet.port);
+            migrate_set_uri(new_uri, &err);
+            g_free(new_uri);
+        }
     }
     qapi_free_SocketAddress(saddr);
     error_propagate(errp, err);
-- 
2.13.6

[Qemu-devel] [PATCH 6/6] migration: make migrate uri parameter optional

[Juan Quintela]

Now that we have set an uri migration parameter, we can make it
optional in the command line.

Signed-off-by: Juan Quintela <quintela@redhat.com>
---
 hmp-commands.hx       |  4 ++--
 hmp.c                 |  5 +++--
 migration/migration.c | 12 +++++++++---
 qapi/migration.json   |  2 +-
 4 files changed, 15 insertions(+), 8 deletions(-)

diff --git a/hmp-commands.hx b/hmp-commands.hx
index 4afd57cf5f..5be4e3934d 100644
--- a/hmp-commands.hx
+++ b/hmp-commands.hx
@@ -928,8 +928,8 @@ ETEXI
 
     {
         .name       = "migrate",
-        .args_type  = "detach:-d,blk:-b,inc:-i,uri:s",
-        .params     = "[-d] [-b] [-i] uri",
+        .args_type  = "detach:-d,blk:-b,inc:-i,uri:-s",
+        .params     = "[-d] [-b] [-i] [uri]",
         .help       = "migrate to URI (using -d to not wait for completion)"
 		      "\n\t\t\t -b for migration without shared storage with"
 		      " full copy of disk\n\t\t\t -i for migration without "
diff --git a/hmp.c b/hmp.c
index 9c544560bb..f8fae740e8 100644
--- a/hmp.c
+++ b/hmp.c
@@ -1938,10 +1938,11 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
     bool detach = qdict_get_try_bool(qdict, "detach", false);
     bool blk = qdict_get_try_bool(qdict, "blk", false);
     bool inc = qdict_get_try_bool(qdict, "inc", false);
-    const char *uri = qdict_get_str(qdict, "uri");
+    bool has_uri = qdict_get_try_bool(qdict, "uri", false);
+    const char *uri = qdict_get_try_str(qdict, "uri");
     Error *err = NULL;
 
-    qmp_migrate(uri, !!blk, blk, !!inc, inc, false, false, &err);
+    qmp_migrate(!!has_uri, uri, !!blk, blk, !!inc, inc, false, false, &err);
     if (err) {
         error_report_err(err);
         return;
diff --git a/migration/migration.c b/migration/migration.c
index 507226907b..3818a83489 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -1356,7 +1356,7 @@ bool migration_is_blocked(Error **errp)
     return false;
 }
 
-void qmp_migrate(const char *uri, bool has_blk, bool blk,
+void qmp_migrate(bool has_uri, const char *uri, bool has_blk, bool blk,
                  bool has_inc, bool inc, bool has_detach, bool detach,
                  Error **errp)
 {
@@ -1374,7 +1374,13 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
         error_setg(errp, "Guest is waiting for an incoming migration");
         return;
     }
-    migrate_set_uri(uri, errp);
+    if (has_uri && uri) {
+        migrate_set_uri(uri, errp);
+    }
+    if (!s->parameters.uri) {
+        error_setg(errp, "Migration uri needs to be set");
+        return;
+    }
     if (migration_is_blocked(errp)) {
         return;
     }
@@ -1412,7 +1418,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
     } else if (strstart(s->parameters.uri, "fd:", &p)) {
         fd_start_outgoing_migration(s, p, &local_err);
     } else {
-        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "s->parameters.uri",
+        error_setg(errp, QERR_INVALID_PARAMETER_VALUE, "migration uri",
                    "a valid migration protocol");
         migrate_set_state(&s->state, MIGRATION_STATUS_SETUP,
                           MIGRATION_STATUS_FAILED);
diff --git a/qapi/migration.json b/qapi/migration.json
index ebde890604..6b9758ab99 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -1042,7 +1042,7 @@
 #
 ##
 { 'command': 'migrate',
-  'data': {'uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
+  'data': {'*uri': 'str', '*blk': 'bool', '*inc': 'bool', '*detach': 'bool' } }
 
 ##
 # @migrate-incoming:
-- 
2.13.6

Re: [Qemu-devel] [PATCH 0/6] Improve info migrate output on destination

[Fam Zheng]

On Mon, 10/30 12:21, Juan Quintela wrote:
> Hi
> 
> This series (on top of my last pull requset) do:

For patchew,

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

Re: [Qemu-devel] [PATCH 1/6] qio: Make port 0 work for qio

[Daniel P. Berrange]

On Mon, Oct 30, 2017 at 12:21:07PM +0100, Juan Quintela wrote:
> For tcp sockets we read back what is the socket/address.  So we know
> what is the port that we are listening into.
> 
> Looked all callers of the function, and they just create the addr, use
> it, and drop it, so no problem that we always update the port in the
> address.

Can you explain more why you need this ?

Nothing should be using the socket_listen() method directly any more IIRC,
and for the QIOChannelSocket classes, you should not rely on the address that
you pass in, being the same as the one that ultimately gets passed into the
socket_listen() method.

Patches that I have pending change things so that listening happens in two
phases. First we take the SocketAddress and do DNS resolution to create
mutliple new SocketAddress structs. These are then passed into the lowlevel
socket_listen() method. So with that happening, even if you update the address
in socket_listen() that info won't get back upto the caller.

If you have a QIOChannelSocket instance, and you want to know what port it
ended up listening on, you should call qio_channel_socket_get_local_address()
method instead, which returns a dynamically popualted SocketAddress struct.
This should mean socket_listen() never needs to update the address that it
binds on.

IOW, I think this patch is redundant.

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 1/6] qio: Make port 0 work for qio

[Juan Quintela]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Oct 30, 2017 at 12:21:07PM +0100, Juan Quintela wrote:
>> For tcp sockets we read back what is the socket/address.  So we know
>> what is the port that we are listening into.
>> 
>> Looked all callers of the function, and they just create the addr, use
>> it, and drop it, so no problem that we always update the port in the
>> address.
>
> Can you explain more why you need this ?

I need to get back somewhere the port that bind gave me.
I can do it later by hand.  But that function has one parameter that
says that it update the sockaddr that gets passed, and it don't do it.

Auditing all the users of it, they don't care (basically all of them
just free the addr after returning for this function).

But if you preffer that I do this directly on the migration code, I have
no problem with that.

> Nothing should be using the socket_listen() method directly any more IIRC,
> and for the QIOChannelSocket classes, you should not rely on the address that
> you pass in, being the same as the one that ultimately gets passed into the
> socket_listen() method.

Ok, I will change to do this directly on the migration code.

> Patches that I have pending change things so that listening happens in two
> phases. First we take the SocketAddress and do DNS resolution to create
> mutliple new SocketAddress structs. These are then passed into the lowlevel
> socket_listen() method. So with that happening, even if you update the address
> in socket_listen() that info won't get back upto the caller.
>
> If you have a QIOChannelSocket instance, and you want to know what port it
> ended up listening on, you should call qio_channel_socket_get_local_address()
> method instead, which returns a dynamically popualted SocketAddress struct.
> This should mean socket_listen() never needs to update the address that it
> binds on.

Ah, yet another function O:-)

Ok, I will use this one.  Sorry for the disturbance.

> IOW, I think this patch is redundant.

Later, Juan.

Re: [Qemu-devel] [PATCH 5/6] migration: Now set the migration uri

[Daniel P. Berrange]

On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote:
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> ---
>  migration/migration.c | 25 ++++++++++++++++++-------
>  migration/migration.h |  2 ++
>  migration/socket.c    |  7 +++++++
>  3 files changed, 27 insertions(+), 7 deletions(-)


> diff --git a/migration/socket.c b/migration/socket.c
> index 3a8232dd2d..c3ab81d1fb 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
>      Error *err = NULL;
>      SocketAddress *saddr = tcp_build_address(host_port, &err);
>      if (!err) {
> +        char *new_uri;
>          socket_start_incoming_migration(saddr, &err);
> +        if (!err) {
> +            new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host,
> +                                      saddr->u.inet.port);

This is bad as it is throwing away data that the original URI had. In particular
you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the
original URI for later, then why not just keep the 'host_port' parameter that
was passed into this function instead of trying to reverse engineeer the URI ?


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 2/6] migration: print features as on off

[Dr. David Alan Gilbert]

* Juan Quintela (quintela@redhat.com) wrote:
> Once there, do one thing for line
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

> ---
>  migration/migration.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4de3b551fe..706d2407ec 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2375,10 +2375,15 @@ void migration_global_dump(Monitor *mon)
>  {
>      MigrationState *ms = migrate_get_current();
>  
> -    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
> -                   "send-configuration=%d, send-section-footer=%d\n",
> -                   ms->store_global_state, ms->only_migratable,
> -                   ms->send_configuration, ms->send_section_footer);
> +    monitor_printf(mon, "globals:\n");
> +    monitor_printf(mon, "store-global-state: %s\n",
> +                   ms->store_global_state ? "on" : "off");
> +    monitor_printf(mon, "only-migratable: %s\n",
> +                   ms->only_migratable ? "on" : "off");
> +    monitor_printf(mon, "send-configuration: %s\n",
> +                   ms->send_configuration ? "on" : "off");
> +    monitor_printf(mon, "send-section-footer: %s\n",
> +                   ms->send_section_footer ? "on" : "off");
>  }
>  
>  #define DEFINE_PROP_MIG_CAP(name, x)             \
> -- 
> 2.13.6
> 
--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK

Re: [Qemu-devel] [PATCH 2/6] migration: print features as on off

[Peter Xu]

On Mon, Oct 30, 2017 at 12:21:08PM +0100, Juan Quintela wrote:
> Once there, do one thing for line
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

> ---
>  migration/migration.c | 13 +++++++++----
>  1 file changed, 9 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 4de3b551fe..706d2407ec 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2375,10 +2375,15 @@ void migration_global_dump(Monitor *mon)
>  {
>      MigrationState *ms = migrate_get_current();
>  
> -    monitor_printf(mon, "globals: store-global-state=%d, only_migratable=%d, "
> -                   "send-configuration=%d, send-section-footer=%d\n",
> -                   ms->store_global_state, ms->only_migratable,
> -                   ms->send_configuration, ms->send_section_footer);
> +    monitor_printf(mon, "globals:\n");
> +    monitor_printf(mon, "store-global-state: %s\n",
> +                   ms->store_global_state ? "on" : "off");
> +    monitor_printf(mon, "only-migratable: %s\n",
> +                   ms->only_migratable ? "on" : "off");
> +    monitor_printf(mon, "send-configuration: %s\n",
> +                   ms->send_configuration ? "on" : "off");
> +    monitor_printf(mon, "send-section-footer: %s\n",
> +                   ms->send_section_footer ? "on" : "off");
>  }
>  
>  #define DEFINE_PROP_MIG_CAP(name, x)             \
> -- 
> 2.13.6
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/6] migration: free addr in the same function that we created it

[Peter Xu]

On Mon, Oct 30, 2017 at 12:21:09PM +0100, Juan Quintela wrote:
> Otherwise, we can't use it after calling socket_start_incoming_migration
> 
> Signed-off-by: Juan Quintela <quintela@redhat.com>

Reviewed-by: Peter Xu <peterx@redhat.com>

http://lists.gnu.org/archive/html/qemu-devel/2017-11/msg01305.html

So I got a same patch. Let's see which one will go in earlier. I think
it should be this one. :)

> ---
>  migration/socket.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/migration/socket.c b/migration/socket.c
> index dee869044a..3a8232dd2d 100644
> --- a/migration/socket.c
> +++ b/migration/socket.c
> @@ -172,7 +172,6 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>  
>      if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
>          object_unref(OBJECT(listen_ioc));
> -        qapi_free_SocketAddress(saddr);
>          return;
>      }
>  
> @@ -181,7 +180,6 @@ static void socket_start_incoming_migration(SocketAddress *saddr,
>                            socket_accept_incoming_migration,
>                            listen_ioc,
>                            (GDestroyNotify)object_unref);
> -    qapi_free_SocketAddress(saddr);
>  }
>  
>  void tcp_start_incoming_migration(const char *host_port, Error **errp)
> @@ -191,6 +189,7 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
>      if (!err) {
>          socket_start_incoming_migration(saddr, &err);
>      }
> +    qapi_free_SocketAddress(saddr);
>      error_propagate(errp, err);
>  }
>  
> @@ -198,4 +197,5 @@ void unix_start_incoming_migration(const char *path, Error **errp)
>  {
>      SocketAddress *saddr = unix_build_address(path);
>      socket_start_incoming_migration(saddr, errp);
> +    qapi_free_SocketAddress(saddr);
>  }
> -- 
> 2.13.6
> 

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 5/6] migration: Now set the migration uri

[Juan Quintela]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote:
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> ---
>>  migration/migration.c | 25 ++++++++++++++++++-------
>>  migration/migration.h |  2 ++
>>  migration/socket.c    |  7 +++++++
>>  3 files changed, 27 insertions(+), 7 deletions(-)
>
>
>> diff --git a/migration/socket.c b/migration/socket.c
>> index 3a8232dd2d..c3ab81d1fb 100644
>> --- a/migration/socket.c
>> +++ b/migration/socket.c
>> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
>>      Error *err = NULL;
>>      SocketAddress *saddr = tcp_build_address(host_port, &err);
>>      if (!err) {
>> +        char *new_uri;
>>          socket_start_incoming_migration(saddr, &err);
>> +        if (!err) {
>> +            new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host,
>> +                                      saddr->u.inet.port);
>
> This is bad as it is throwing away data that the original URI had. In particular
> you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the
> original URI for later, then why not just keep the 'host_port' parameter that
> was passed into this function instead of trying to reverse engineeer the URI ?

I don't need the original uri anymore, this is the incoming side of
migration, and we can only set that once, if migration fails, we need to
restart qemu anymore.

I changed it to this:

            new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host,
                                      address->u.inet.port,
                                      iaddr->has_ipv4 ? ",ipv4" : "",
                                      iaddr->has_ipv6 ? ",ipv6" : "");


Clearly, we don't care about the to= parameter.

The whole point of this exercise is that in destination, we use

-incoming tcp:0:0

and with the output of info migrate_parameters (uri) we are able to
migrate to that place.  For rest of migration posibilities, there is no
changes at all.


Thanks, Juan.

Re: [Qemu-devel] [PATCH 5/6] migration: Now set the migration uri

[Daniel P. Berrange]

On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote:
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote:
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >> ---
> >>  migration/migration.c | 25 ++++++++++++++++++-------
> >>  migration/migration.h |  2 ++
> >>  migration/socket.c    |  7 +++++++
> >>  3 files changed, 27 insertions(+), 7 deletions(-)
> >
> >
> >> diff --git a/migration/socket.c b/migration/socket.c
> >> index 3a8232dd2d..c3ab81d1fb 100644
> >> --- a/migration/socket.c
> >> +++ b/migration/socket.c
> >> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
> >>      Error *err = NULL;
> >>      SocketAddress *saddr = tcp_build_address(host_port, &err);
> >>      if (!err) {
> >> +        char *new_uri;
> >>          socket_start_incoming_migration(saddr, &err);
> >> +        if (!err) {
> >> +            new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host,
> >> +                                      saddr->u.inet.port);
> >
> > This is bad as it is throwing away data that the original URI had. In particular
> > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the
> > original URI for later, then why not just keep the 'host_port' parameter that
> > was passed into this function instead of trying to reverse engineeer the URI ?
> 
> I don't need the original uri anymore, this is the incoming side of
> migration, and we can only set that once, if migration fails, we need to
> restart qemu anymore.
> 
> I changed it to this:
> 
>             new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host,
>                                       address->u.inet.port,
>                                       iaddr->has_ipv4 ? ",ipv4" : "",
>                                       iaddr->has_ipv6 ? ",ipv6" : "");
> 
> 
> Clearly, we don't care about the to= parameter.
> 
> The whole point of this exercise is that in destination, we use
> 
> -incoming tcp:0:0
> 
> and with the output of info migrate_parameters (uri) we are able to
> migrate to that place.  For rest of migration posibilities, there is no
> changes at all.

That doesn't work typically. When the incoming QEMU listens for connections,
by default it will listen on 0.0.0.0, or ::, so that's what the address
you're creating will contain.  You can't use 0.0.0.0 or :: in a connect()
call on the other side as that's a wildcard address that refers to "any 
valid IP address on the current host".

When you connect to the listening QEMU you need the actual IP address
of one (of the possibly many) NICs on the target host.  IOW, the only
time the listen address is useful is if you have told QEMU to listen on
a specific NIC's IP address, instead of the wildcard address.

This is the core reason why libvirt calls 'gethostname()' on the target
host to identify the primary IP address of the host, and uses that on the
source host to establish the connection.

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 5/6] migration: Now set the migration uri

[Daniel P. Berrange]

On Wed, Nov 22, 2017 at 12:54:58PM +0000, Daniel P. Berrange wrote:
> On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote:
> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote:
> > >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> > >> ---
> > >>  migration/migration.c | 25 ++++++++++++++++++-------
> > >>  migration/migration.h |  2 ++
> > >>  migration/socket.c    |  7 +++++++
> > >>  3 files changed, 27 insertions(+), 7 deletions(-)
> > >
> > >
> > >> diff --git a/migration/socket.c b/migration/socket.c
> > >> index 3a8232dd2d..c3ab81d1fb 100644
> > >> --- a/migration/socket.c
> > >> +++ b/migration/socket.c
> > >> @@ -187,7 +187,14 @@ void tcp_start_incoming_migration(const char *host_port, Error **errp)
> > >>      Error *err = NULL;
> > >>      SocketAddress *saddr = tcp_build_address(host_port, &err);
> > >>      if (!err) {
> > >> +        char *new_uri;
> > >>          socket_start_incoming_migration(saddr, &err);
> > >> +        if (!err) {
> > >> +            new_uri = g_strdup_printf("tcp:%s:%s", saddr->u.inet.host,
> > >> +                                      saddr->u.inet.port);
> > >
> > > This is bad as it is throwing away data that the original URI had. In particular
> > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the
> > > original URI for later, then why not just keep the 'host_port' parameter that
> > > was passed into this function instead of trying to reverse engineeer the URI ?
> > 
> > I don't need the original uri anymore, this is the incoming side of
> > migration, and we can only set that once, if migration fails, we need to
> > restart qemu anymore.
> > 
> > I changed it to this:
> > 
> >             new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host,
> >                                       address->u.inet.port,
> >                                       iaddr->has_ipv4 ? ",ipv4" : "",
> >                                       iaddr->has_ipv6 ? ",ipv6" : "");
> > 
> > 
> > Clearly, we don't care about the to= parameter.
> > 
> > The whole point of this exercise is that in destination, we use
> > 
> > -incoming tcp:0:0
> > 
> > and with the output of info migrate_parameters (uri) we are able to
> > migrate to that place.  For rest of migration posibilities, there is no
> > changes at all.
> 
> That doesn't work typically. When the incoming QEMU listens for connections,
> by default it will listen on 0.0.0.0, or ::, so that's what the address
> you're creating will contain.  You can't use 0.0.0.0 or :: in a connect()
> call on the other side as that's a wildcard address that refers to "any 
> valid IP address on the current host".
> 
> When you connect to the listening QEMU you need the actual IP address
> of one (of the possibly many) NICs on the target host.  IOW, the only
> time the listen address is useful is if you have told QEMU to listen on
> a specific NIC's IP address, instead of the wildcard address.
> 
> This is the core reason why libvirt calls 'gethostname()' on the target
> host to identify the primary IP address of the host, and uses that on the
> source host to establish the connection.

I should have said the port number info will still be useful (if you're
using the dynamic port allocation), it is just the IP address that's not
usable in general.

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 5/6] migration: Now set the migration uri

[Juan Quintela]

"Daniel P. Berrange" <berrange@redhat.com> wrote:
> On Wed, Nov 22, 2017 at 12:54:58PM +0000, Daniel P. Berrange wrote:
>> On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote:
>> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
>> > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote:

> > > This is bad as it is throwing away data that the original URI
>> > > had. In particular
>> > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the
>> > > original URI for later, then why not just keep the 'host_port' parameter that
>> > > was passed into this function instead of trying to reverse
>> > > engineeer the URI ?
>> > 
>> > I don't need the original uri anymore, this is the incoming side of
>> > migration, and we can only set that once, if migration fails, we need to
>> > restart qemu anymore.
>> > 
>> > I changed it to this:
>> > 
>> >             new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host,
>> >                                       address->u.inet.port,
>> >                                       iaddr->has_ipv4 ? ",ipv4" : "",
>> >                                       iaddr->has_ipv6 ? ",ipv6" : "");
>> > 
>> > 
>> > Clearly, we don't care about the to= parameter.
>> > 
>> > The whole point of this exercise is that in destination, we use
>> > 
>> > -incoming tcp:0:0
>> > 
>> > and with the output of info migrate_parameters (uri) we are able to
>> > migrate to that place.  For rest of migration posibilities, there is no
>> > changes at all.
>> 
>> That doesn't work typically. When the incoming QEMU listens for connections,
>> by default it will listen on 0.0.0.0, or ::, so that's what the address
>> you're creating will contain.  You can't use 0.0.0.0 or :: in a connect()
>> call on the other side as that's a wildcard address that refers to "any 
>> valid IP address on the current host".
>> 
>> When you connect to the listening QEMU you need the actual IP address
>> of one (of the possibly many) NICs on the target host.  IOW, the only
>> time the listen address is useful is if you have told QEMU to listen on
>> a specific NIC's IP address, instead of the wildcard address.
>> 
>> This is the core reason why libvirt calls 'gethostname()' on the target
>> host to identify the primary IP address of the host, and uses that on the
>> source host to establish the connection.
>
> I should have said the port number info will still be useful (if you're
> using the dynamic port allocation), it is just the IP address that's not
> usable in general.

I gave up O:-)

I introduced tcp_port parameter, that is really what I wanted to know.
The test use case (the one that I am interested right now) is that I
do:

qemu-system-x86_64 ..... --incomping tcp:127.0.0.1:0

And I want to know the port that the destination gets to connect to it.
For migration, it don't really matters if we _also_ set the
address/ip/whatever it gets translated to, because it is not possible
right now to restart the migration incoming side (you need to restart
qemu for long and boring historic reasons).

So, I ended just adding:

+#
+# @tcp-port: Only used for tcp, to know what is the real port
+#                     (Since 2.12)
 #

And all the boilerplate code to access/setup this one.

BTW, I am not sure how  well the current code will work with a range of
ports, because we don't have a way to tell the source which one the
kernel/qemu decided to use O:-)

Later, Juan.

Re: [Qemu-devel] [PATCH 5/6] migration: Now set the migration uri

[Daniel P. Berrange]

On Wed, Nov 29, 2017 at 05:43:35PM +0100, Juan Quintela wrote:
> "Daniel P. Berrange" <berrange@redhat.com> wrote:
> > On Wed, Nov 22, 2017 at 12:54:58PM +0000, Daniel P. Berrange wrote:
> >> On Wed, Nov 22, 2017 at 01:29:57PM +0100, Juan Quintela wrote:
> >> > "Daniel P. Berrange" <berrange@redhat.com> wrote:
> >> > > On Mon, Oct 30, 2017 at 12:21:11PM +0100, Juan Quintela wrote:
> 
> > > > This is bad as it is throwing away data that the original URI
> >> > > had. In particular
> >> > > you loose the 'ipv4=on|off' and 'ipv6=on|off' flags. If you need to keep the
> >> > > original URI for later, then why not just keep the 'host_port' parameter that
> >> > > was passed into this function instead of trying to reverse
> >> > > engineeer the URI ?
> >> > 
> >> > I don't need the original uri anymore, this is the incoming side of
> >> > migration, and we can only set that once, if migration fails, we need to
> >> > restart qemu anymore.
> >> > 
> >> > I changed it to this:
> >> > 
> >> >             new_uri = g_strdup_printf("tcp:%s:%s%s%s", address->u.inet.host,
> >> >                                       address->u.inet.port,
> >> >                                       iaddr->has_ipv4 ? ",ipv4" : "",
> >> >                                       iaddr->has_ipv6 ? ",ipv6" : "");
> >> > 
> >> > 
> >> > Clearly, we don't care about the to= parameter.
> >> > 
> >> > The whole point of this exercise is that in destination, we use
> >> > 
> >> > -incoming tcp:0:0
> >> > 
> >> > and with the output of info migrate_parameters (uri) we are able to
> >> > migrate to that place.  For rest of migration posibilities, there is no
> >> > changes at all.
> >> 
> >> That doesn't work typically. When the incoming QEMU listens for connections,
> >> by default it will listen on 0.0.0.0, or ::, so that's what the address
> >> you're creating will contain.  You can't use 0.0.0.0 or :: in a connect()
> >> call on the other side as that's a wildcard address that refers to "any 
> >> valid IP address on the current host".
> >> 
> >> When you connect to the listening QEMU you need the actual IP address
> >> of one (of the possibly many) NICs on the target host.  IOW, the only
> >> time the listen address is useful is if you have told QEMU to listen on
> >> a specific NIC's IP address, instead of the wildcard address.
> >> 
> >> This is the core reason why libvirt calls 'gethostname()' on the target
> >> host to identify the primary IP address of the host, and uses that on the
> >> source host to establish the connection.
> >
> > I should have said the port number info will still be useful (if you're
> > using the dynamic port allocation), it is just the IP address that's not
> > usable in general.
> 
> I gave up O:-)
> 
> I introduced tcp_port parameter, that is really what I wanted to know.
> The test use case (the one that I am interested right now) is that I
> do:
> 
> qemu-system-x86_64 ..... --incomping tcp:127.0.0.1:0
> 
> And I want to know the port that the destination gets to connect to it.
> For migration, it don't really matters if we _also_ set the
> address/ip/whatever it gets translated to, because it is not possible
> right now to restart the migration incoming side (you need to restart
> qemu for long and boring historic reasons).
> 
> So, I ended just adding:
> 
> +#
> +# @tcp-port: Only used for tcp, to know what is the real port
> +#                     (Since 2.12)
>  #
> 
> And all the boilerplate code to access/setup this one.
> 
> BTW, I am not sure how  well the current code will work with a range of
> ports, because we don't have a way to tell the source which one the
> kernel/qemu decided to use O:-)

Ultimately I think we need to be able to report a full list of
SocketAddress structs representing the listening addresses, because
we will eventually be listening on multiple distinct sockets. Currently
if a hostname resolves to multiple IP addrs, we only listen on the first
successful IP, but I have patches that fix this so we listen on multiple
IPs....

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 :|