diff mbox series

[v13,2/3] migration: Create socket-address parameter

Message ID 20190220092214.2384-3-quintela@redhat.com
State New
Headers show
Series Add make check tests for Migration | expand

Commit Message

Juan Quintela Feb. 20, 2019, 9:22 a.m. UTC 
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
---
 hmp.c                 | 33 +++++++++++++++++++++++++++++++++
 migration/migration.c | 24 ++++++++++++++++++++++++
 migration/migration.h |  4 ++++
 migration/socket.c    | 11 +++++++++++
 qapi/migration.json   |  6 +++++-
 qapi/sockets.json     | 13 +++++++++++++
 6 files changed, 90 insertions(+), 1 deletion(-)

Comments

Eric Blake Feb. 20, 2019, 2:03 p.m. UTC | #1 
On 2/20/19 3:22 AM, 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.
> 
> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
> Signed-off-by: Juan Quintela <quintela@redhat.com>
> 
> --

> +++ 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'] } }
>  

Okay, I actually tried compiling your patch with the following hunk:

>  ##
>  # @query-migrate:
> diff --git a/qapi/sockets.json b/qapi/sockets.json
> index fc81d8d5e8..d7f77984af 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: 3.1

4.0

> +##
> +{ 'struct': 'DummyStruct',
> +  'data': { 'dummy-list': ['SocketAddress'] } }
> 

removed, and got:

In file included from qapi/qapi-types-migration.c:15:
qapi/qapi-types-migration.h:267:5: error: unknown type name
‘SocketAddressList’
     SocketAddressList *socket_address;
     ^~~~~~~~~~~~~~~~~
make: *** [/home/eblake/qemu/rules.mak:69: qapi/qapi-types-migration.o]
Error 1

That says we have a bug in our QAPI generator - the type
SocketAddressList should be auto-generated at the point where it is
needed in migration.json. Markus, any ideas why we are not properly
auto-generating an array type when the only use of that array comes from
a different module than where the original type was declared?
Markus Armbruster Feb. 21, 2019, 1:12 p.m. UTC | #2 
Eric Blake <eblake@redhat.com> writes:

> On 2/20/19 3:22 AM, 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.
>> 
>> Reviewed-by: Dr. David Alan Gilbert <dgilbert@redhat.com>
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> --
>
>> +++ 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'] } }
>>  
>
> Okay, I actually tried compiling your patch with the following hunk:
>
>>  ##
>>  # @query-migrate:
>> diff --git a/qapi/sockets.json b/qapi/sockets.json
>> index fc81d8d5e8..d7f77984af 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: 3.1
>
> 4.0
>
>> +##
>> +{ 'struct': 'DummyStruct',
>> +  'data': { 'dummy-list': ['SocketAddress'] } }
>> 
>
> removed, and got:
>
> In file included from qapi/qapi-types-migration.c:15:
> qapi/qapi-types-migration.h:267:5: error: unknown type name
> ‘SocketAddressList’
>      SocketAddressList *socket_address;
>      ^~~~~~~~~~~~~~~~~
> make: *** [/home/eblake/qemu/rules.mak:69: qapi/qapi-types-migration.o]
> Error 1
>
> That says we have a bug in our QAPI generator - the type
> SocketAddressList should be auto-generated at the point where it is
> needed in migration.json. Markus, any ideas why we are not properly
> auto-generating an array type when the only use of that array comes from
> a different module than where the original type was declared?

With the DummyStruct, we generate SocketAddressList stuff into
qapi-{types,visit}-sockets.[ch].

Without it, we it ends up in qapi-{types,visit}-block-core.[ch].  How
come?

We create non-builtin array types on demand, i.e. on first use, by
calling ._make_array_type().

Like any other non-builtin type, an array type takes an info tuple
describing where it's defined.  We pass it the info of whatever
triggered its creation.  That's not as silly as it may sound; it can be
viewed as a definition.

However, it falls apart when we use the info to set the type's module,
in ._def_entity().  The module dicates where the generated code goes.
The array type's module becomes whatever module uses it first.  It
should be its element type's module instead.

Needs fixing.

If you don't want to wait for the fix, commit the DummyStruct
work-around with a big fat FIXME comment.
Markus Armbruster March 1, 2019, 3:42 p.m. UTC | #3 
Markus Armbruster <armbru@redhat.com> writes:

> Eric Blake <eblake@redhat.com> writes:
[...]
>> That says we have a bug in our QAPI generator - the type
>> SocketAddressList should be auto-generated at the point where it is
>> needed in migration.json. Markus, any ideas why we are not properly
>> auto-generating an array type when the only use of that array comes from
>> a different module than where the original type was declared?
>
> With the DummyStruct, we generate SocketAddressList stuff into
> qapi-{types,visit}-sockets.[ch].
>
> Without it, we it ends up in qapi-{types,visit}-block-core.[ch].  How
> come?
>
> We create non-builtin array types on demand, i.e. on first use, by
> calling ._make_array_type().
>
> Like any other non-builtin type, an array type takes an info tuple
> describing where it's defined.  We pass it the info of whatever
> triggered its creation.  That's not as silly as it may sound; it can be
> viewed as a definition.
>
> However, it falls apart when we use the info to set the type's module,
> in ._def_entity().  The module dicates where the generated code goes.
> The array type's module becomes whatever module uses it first.  It
> should be its element type's module instead.
>
> Needs fixing.
>
> If you don't want to wait for the fix, commit the DummyStruct
> work-around with a big fat FIXME comment.

Proposed fix:
Subject: [PATCH 0/7] qapi: Code generation fixes
Message-Id: <20190301154051.23317-1-armbru@redhat.com>
diff mbox series

Patch

diff --git a/hmp.c b/hmp.c
index 1e006eeb49..8e283153b5 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 37e06b76dc..5ecf0978ac 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"
@@ -197,6 +199,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)
@@ -312,6 +319,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;
@@ -966,6 +984,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 dcd05d9f87..837709d8a1 100644
--- a/migration/migration.h
+++ b/migration/migration.h
@@ -80,6 +80,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);
@@ -300,6 +303,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);
 
 #define qemu_ram_foreach_block \
   #warning "Use qemu_ram_foreach_block_migratable in migration code"
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 7a795ecc16..b62947791f 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..d7f77984af 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: 3.1
+##
+{ 'struct': 'DummyStruct',
+  'data': { 'dummy-list': ['SocketAddress'] } }