Re: [Qemu-devel] [PATCH v13 2/3] migration: Create socket-address parameter

[Markus Armbruster <armbru@redhat.com>]

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.