diff mbox series

[PULL,10/21] migration: Create socket-address parameter

Message ID 20190305181602.9051-11-dgilbert@redhat.com
State New
Headers show
Series [PULL,01/21] migration: Fix cancel state | expand

Commit Message

Dr. David Alan Gilbert March 5, 2019, 6:15 p.m. UTC 
From: Juan Quintela <quintela@redhat.com>

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.

Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
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.
Move SocketAddress_to_str here.  I used to try to generalize the one
in chardev/char-socket.c, but it is not worth it.

Free string (eric)
Handle VSOCK address nicely (not that migration can use them yet).
Remove useless breaks (dave)
rename socket_address to socket_address_list to avoid confusion
Update to 4.0 (eric)
Put a comment indicating that there is a problem on the qapi
generator (markus).
Message-Id: <20190227105128.1655-3-quintela@redhat.com>

Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
---
 hmp.c                 | 33 +++++++++++++++++++++++++++++++++
 migration/migration.c | 24 ++++++++++++++++++++++++
 migration/migration.h |  4 ++++
 migration/socket.c    | 11 +++++++++++
 qapi/migration.json   |  6 +++++-
 qapi/sockets.json     | 18 ++++++++++++++++++
 6 files changed, 95 insertions(+), 1 deletion(-)

Comments

Eric Blake March 5, 2019, 6:47 p.m. UTC | #1 
On 3/5/19 12:15 PM, Dr. David Alan Gilbert (git) wrote:
> From: Juan Quintela <quintela@redhat.com>
> 
> 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.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> --
> 

This was not the usual '---' divider, and hence:

> 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.
> Move SocketAddress_to_str here.  I used to try to generalize the one
> in chardev/char-socket.c, but it is not worth it.
> 
> Free string (eric)
> Handle VSOCK address nicely (not that migration can use them yet).
> Remove useless breaks (dave)
> rename socket_address to socket_address_list to avoid confusion
> Update to 4.0 (eric)
> Put a comment indicating that there is a problem on the qapi
> generator (markus).

...all this got included, even if it was perhaps not intended.

> Message-Id: <20190227105128.1655-3-quintela@redhat.com>
> 
> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>

At any rate, it certainly looks odd to split *-by: tags by so much text.

> +++ b/qapi/sockets.json
> @@ -152,3 +152,21 @@
>               '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
> +#
> +# FIXME: This shouldn't be needed, but this struct has two users, and
> +# current qapi generator generates it on the 1st place that uses it,
> +# so the second user don't see it.  Putting it here it is seen in both
> +# sides.

If Markus' pull request lands first, we don't need this.
https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg01185.html
Markus Armbruster March 6, 2019, 5:21 a.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 3/5/19 12:15 PM, Dr. David Alan Gilbert (git) wrote:
>> From: Juan Quintela <quintela@redhat.com>
>>
>> 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.
>>
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>>
>> --
>>
>
> This was not the usual '---' divider, and hence:
>
>> 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.
>> Move SocketAddress_to_str here.  I used to try to generalize the one
>> in chardev/char-socket.c, but it is not worth it.
>>
>> Free string (eric)
>> Handle VSOCK address nicely (not that migration can use them yet).
>> Remove useless breaks (dave)
>> rename socket_address to socket_address_list to avoid confusion
>> Update to 4.0 (eric)
>> Put a comment indicating that there is a problem on the qapi
>> generator (markus).
>
> ...all this got included, even if it was perhaps not intended.
>
>> Message-Id: <20190227105128.1655-3-quintela@redhat.com>
>>
>> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>
> At any rate, it certainly looks odd to split *-by: tags by so much text.
>
>> +++ b/qapi/sockets.json
>> @@ -152,3 +152,21 @@
>>               '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
>> +#
>> +# FIXME: This shouldn't be needed, but this struct has two users, and
>> +# current qapi generator generates it on the 1st place that uses it,
>> +# so the second user don't see it.  Putting it here it is seen in both
>> +# sides.
>
> If Markus' pull request lands first, we don't need this.
> https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg01185.html

It did: merge commit a3e3b0a7bd5.  Please drop this hunk, either in a
respin or in a follow-up patch.

A respin would give you a chance to clean up the commit message.
Dr. David Alan Gilbert March 6, 2019, 10:40 a.m. UTC | #3 
* Markus Armbruster (armbru@redhat.com) wrote:
> Eric Blake <eblake@redhat.com> writes:
> 
> > On 3/5/19 12:15 PM, Dr. David Alan Gilbert (git) wrote:
> >> From: Juan Quintela <quintela@redhat.com>
> >>
> >> 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.
> >>
> >> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >> Signed-off-by: Juan Quintela <quintela@redhat.com>
> >>
> >> --
> >>
> >
> > This was not the usual '---' divider, and hence:
> >
> >> 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.
> >> Move SocketAddress_to_str here.  I used to try to generalize the one
> >> in chardev/char-socket.c, but it is not worth it.
> >>
> >> Free string (eric)
> >> Handle VSOCK address nicely (not that migration can use them yet).
> >> Remove useless breaks (dave)
> >> rename socket_address to socket_address_list to avoid confusion
> >> Update to 4.0 (eric)
> >> Put a comment indicating that there is a problem on the qapi
> >> generator (markus).
> >
> > ...all this got included, even if it was perhaps not intended.
> >
> >> Message-Id: <20190227105128.1655-3-quintela@redhat.com>
> >>
> >> Signed-off-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> >
> > At any rate, it certainly looks odd to split *-by: tags by so much text.
> >
> >> +++ b/qapi/sockets.json
> >> @@ -152,3 +152,21 @@
> >>               '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
> >> +#
> >> +# FIXME: This shouldn't be needed, but this struct has two users, and
> >> +# current qapi generator generates it on the 1st place that uses it,
> >> +# so the second user don't see it.  Putting it here it is seen in both
> >> +# sides.
> >
> > If Markus' pull request lands first, we don't need this.
> > https://lists.gnu.org/archive/html/qemu-devel/2019-03/msg01185.html
> 
> It did: merge commit a3e3b0a7bd5.  Please drop this hunk, either in a
> respin or in a follow-up patch.
> 
> A respin would give you a chance to clean up the commit message.

OK, respin coming up.

Dave

--
Dr. David Alan Gilbert / dgilbert@redhat.com / Manchester, UK
diff mbox series

Patch

diff --git a/hmp.c b/hmp.c
index 5f13b16e24..c2ad3f8251 100644
--- a/hmp.c
+++ b/hmp.c
@@ -166,6 +166,27 @@  void hmp_info_mice(Monitor *mon, const QDict *qdict)
     qapi_free_MouseInfoList(mice_list);
 }
 
+static char *SocketAddress_to_str(SocketAddress *addr)
+{
+    switch (addr->type) {
+    case SOCKET_ADDRESS_TYPE_INET:
+        return g_strdup_printf("tcp:%s:%s",
+                               addr->u.inet.host,
+                               addr->u.inet.port);
+    case SOCKET_ADDRESS_TYPE_UNIX:
+        return g_strdup_printf("unix:%s",
+                               addr->u.q_unix.path);
+    case SOCKET_ADDRESS_TYPE_FD:
+        return g_strdup_printf("fd:%s", addr->u.fd.str);
+    case SOCKET_ADDRESS_TYPE_VSOCK:
+        return g_strdup_printf("tcp:%s:%s",
+                               addr->u.vsock.cid,
+                               addr->u.vsock.port);
+    default:
+        return g_strdup("unknown address type");
+    }
+}
+
 void hmp_info_migrate(Monitor *mon, const QDict *qdict)
 {
     MigrationInfo *info;
@@ -306,6 +327,18 @@  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
         g_free(str);
         visit_free(v);
     }
+    if (info->has_socket_address) {
+        SocketAddressList *addr;
+
+        monitor_printf(mon, "socket address: [\n");
+
+        for (addr = info->socket_address; addr; addr = addr->next) {
+            char *s = SocketAddress_to_str(addr->value);
+            monitor_printf(mon, "\t%s\n", s);
+            g_free(s);
+        }
+        monitor_printf(mon, "]\n");
+    }
     qapi_free_MigrationInfo(info);
     qapi_free_MigrationCapabilityStatusList(caps);
 }
diff --git a/migration/migration.c b/migration/migration.c
index 4ff195c901..501319aa14 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"
@@ -207,6 +209,11 @@  void migration_incoming_state_destroy(void)
     }
 
     qemu_event_reset(&mis->main_thread_load_event);
+
+    if (mis->socket_address_list) {
+        qapi_free_SocketAddressList(mis->socket_address_list);
+        mis->socket_address_list = NULL;
+    }
 }
 
 static void migrate_generate_event(int new_state)
@@ -322,6 +329,17 @@  void migration_incoming_enable_colo(void)
     migration_colo_enabled = true;
 }
 
+void migrate_add_address(SocketAddress *address)
+{
+    MigrationIncomingState *mis = migration_incoming_get_current();
+    SocketAddressList *addrs;
+
+    addrs = g_new0(SocketAddressList, 1);
+    addrs->next = mis->socket_address_list;
+    mis->socket_address_list = addrs;
+    addrs->value = QAPI_CLONE(SocketAddress, address);
+}
+
 void qemu_start_incoming_migration(const char *uri, Error **errp)
 {
     const char *p;
@@ -1003,6 +1021,12 @@  static void fill_destination_migration_info(MigrationInfo *info)
 {
     MigrationIncomingState *mis = migration_incoming_get_current();
 
+    if (mis->socket_address_list) {
+        info->has_socket_address = true;
+        info->socket_address =
+            QAPI_CLONE(SocketAddressList, mis->socket_address_list);
+    }
+
     switch (mis->state) {
     case MIGRATION_STATUS_NONE:
         return;
diff --git a/migration/migration.h b/migration/migration.h
index 81c261941d..99e99e56bd 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -84,6 +84,9 @@  struct MigrationIncomingState {
     bool postcopy_recover_triggered;
     QemuSemaphore postcopy_pause_sem_dst;
     QemuSemaphore postcopy_pause_sem_fault;
+
+    /* List of listening socket addresses  */
+    SocketAddressList *socket_address_list;
 };
 
 MigrationIncomingState *migration_incoming_get_current(void);
@@ -305,6 +308,7 @@  void migrate_send_rp_resume_ack(MigrationIncomingState *mis, uint32_t value);
 
 void dirty_bitmap_mig_before_vm_start(void);
 void init_dirty_bitmap_incoming_migration(void);
+void migrate_add_address(SocketAddress *address);
 
 int foreach_not_ignored_block(RAMBlockIterFunc func, void *opaque);
 
diff --git a/migration/socket.c b/migration/socket.c
index f4c8174400..239527fb1f 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"
@@ -177,6 +178,7 @@  static void socket_start_incoming_migration(SocketAddress *saddr,
                                             Error **errp)
 {
     QIONetListener *listener = qio_net_listener_new();
+    size_t i;
 
     qio_net_listener_set_name(listener, "migration-socket-listener");
 
@@ -189,6 +191,15 @@  static void socket_start_incoming_migration(SocketAddress *saddr,
                                           socket_accept_incoming_migration,
                                           NULL, NULL,
                                           g_main_context_get_thread_default());
+
+    for (i = 0; i < listener->nsioc; i++)  {
+        SocketAddress *address =
+            qio_channel_socket_get_local_address(listener->sioc[i], errp);
+        if (!address) {
+            return;
+        }
+        migrate_add_address(address);
+    }
 }
 
 void tcp_start_incoming_migration(const char *host_port, Error **errp)
diff --git a/qapi/migration.json b/qapi/migration.json
index eab87340b2..6bd7fd3f1a 100644
--- a/qapi/migration.json
+++ b/qapi/migration.json
@@ -6,6 +6,7 @@ 
 ##
 
 { 'include': 'common.json' }
+{ 'include': 'sockets.json' }
 
 ##
 # @MigrationStats:
@@ -199,6 +200,8 @@ 
 # @compression: migration compression statistics, only returned if compression
 #           feature is on and status is 'active' or 'completed' (Since 3.1)
 #
+# @socket-address: Only used for tcp, to know what the real port is (Since 4.0)
+#
 # Since: 0.14.0
 ##
 { 'struct': 'MigrationInfo',
@@ -213,7 +216,8 @@ 
            '*error-desc': 'str',
            '*postcopy-blocktime' : 'uint32',
            '*postcopy-vcpu-blocktime': ['uint32'],
-           '*compression': 'CompressionStats'} }
+           '*compression': 'CompressionStats',
+           '*socket-address': ['SocketAddress'] } }
 
 ##
 # @query-migrate:
diff --git a/qapi/sockets.json b/qapi/sockets.json
index fc81d8d5e8..6eeda4a9d6 100644
--- a/qapi/sockets.json
+++ b/qapi/sockets.json
@@ -152,3 +152,21 @@ 
             '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
+#
+# FIXME: This shouldn't be needed, but this struct has two users, and
+# current qapi generator generates it on the 1st place that uses it,
+# so the second user don't see it.  Putting it here it is seen in both
+# sides.
+#
+# Since: 4.0
+##
+{ 'struct': 'DummyStruct',
+  'data': { 'dummy-list': ['SocketAddress'] } }