Re: [Qemu-devel] [PATCH v10 03/24] migration: Create tcp_port parameter

[Juan Quintela <quintela@redhat.com>]

Daniel P. Berrange <berrange@redhat.com> wrote:
> On Wed, Mar 07, 2018 at 11:59:49AM +0100, Juan Quintela wrote:
>> It will be used to store the uri tcp_port parameter.  This is the only
>> parameter than can change and we can need to be able to connect to it.
>> 
>> Signed-off-by: Juan Quintela <quintela@redhat.com>
>> 
>> --
>>  # TODO either fuse back into MigrationParameters, or make
>> @@ -584,7 +591,8 @@
>>              '*block-incremental': 'bool',
>>              '*x-multifd-channels': 'int',
>>              '*x-multifd-page-count': 'int',
>> -            '*xbzrle-cache-size': 'size' } }
>> +            '*xbzrle-cache-size': 'size',
>> +            '*x-tcp-port': 'uint16'} }
>
> This should not exist - this exposes this parameter in migate-set-parameters
> as a end user settable property, which is definitely not desirable.
>
> It is only something we should report with 'query-migrate' / 'info migrate'

Oops, my understanding was that the three places have to be in sync.
Now I stand corrected.  Thanks.


>
>>  # @migrate-set-parameters:
>> @@ -667,6 +675,10 @@
>>  #                     needs to be a multiple of the target page size
>>  #                     and a power of 2
>>  #                     (Since 2.11)
>> +#
>> +# @x-tcp-port: Only used for tcp, to know what the real port is
>> +#                     (Since 2.12)
>> +#
>>  # Since: 2.4
>>  ##
>>  { 'struct': 'MigrationParameters',
>> @@ -683,7 +695,8 @@
>>              '*block-incremental': 'bool' ,
>>              '*x-multifd-channels': 'uint8',
>>              '*x-multifd-page-count': 'uint32',
>> -            '*xbzrle-cache-size': 'size' } }
>> +            '*xbzrle-cache-size': 'size',
>> +            '*x-tcp-port': 'uint16'} }
>
> As mentioned in previous review, IMHO we should be reporting the full
> socket address, and allow an array of them, since we're going to have
> more than one address available. i.e.
>
>    '*socket-address': ['SocketAddress']

This is weird, really weird.  But was done.

- this needs to be copied, because it is a pointer, so we end needing
  QAPI_CLONE(), yes, I know.
- it is really weird that I have to:

    rsp_return = qdict_get_qdict(rsp, "return");
    object = qdict_get(rsp_return, parameter);

    iv = qobject_input_visitor_new(object);
    visit_type_SocketAddress(iv, NULL, &saddr, &local_err);
    result = g_strdup_printf("%s",
                             SocketAddress_to_str("", saddr, false, false));
    QDECREF(rsp);

  Remember, it is a *series* of strings in the ddict, we have code to
  _print_ qdicts, as info migrate works.  But I haven't found an easier
  way of getting from a qdict to an string than:
    * parsing it
    * convert it back to text
  sniff

> It doesn't cover non-socket based URIs, but that's fine, because for those
> the mgmt app already knows how the channel is setup. We just need the
> array of SocketAddress, because for socket URIs, the hostname, gets turned
> into an array of addresses, and the mgmt app can't discover them.

SocketAddress are a mess, there is not a single example that I can find
on how to use it on the whole tree.  I *think* that I did that right,
will see your comments on the next post.

Later, Juan.