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

[Juan Quintela <quintela@redhat.com>]

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