Peter Xu <pet...@redhat.com> wrote:
> On Mon, Jan 29, 2018 at 01:17:51PM +0100, Juan Quintela wrote:
>> We can set the port parameter as zero. This patch lets us know what
>> port the system was choosen for us. Now we can migrate to this place.
>>
>> Signed-off-by: Juan Quintela <quint...@redhat.com>
>>
>> --
>>
>> This was migrate_set_uri(), but as we only need the tcp_port, change
>> to that one.
>> ---
>> migration/migration.c | 10 ++++++++++
>> migration/migration.h | 2 ++
>> migration/socket.c | 35 ++++++++++++++++++++++++++++++-----
>> 3 files changed, 42 insertions(+), 5 deletions(-)
>>
>> diff --git a/migration/migration.c b/migration/migration.c
>> index eb6958dcda..53818a87af 100644
>> --- a/migration/migration.c
>> +++ b/migration/migration.c
>> @@ -245,6 +245,16 @@ void migrate_send_rp_req_pages(MigrationIncomingState
>> *mis, const char *rbname,
>> }
>> }
>>
>> +void migrate_set_port(const uint16_t port, Error **errp)
>> +{
>> + MigrateSetParameters p = {
>> + .has_x_tcp_port = true,
>> + .x_tcp_port = port,
>> + };
>> +
>> + qmp_migrate_set_parameters(&p, errp);
>
> If we are calling qmp_migrate_set_parameters() here, does it mean that
> user can also set this parameter via QMP?
Yeap. We do that, or we invent yet another mechanism to update the
tcp_port parameter :-(
You can't have both.
if the user modifies it, it just shots itself it its feet, no?
>> "migration-socket-listener");
>>
>> if (qio_channel_socket_listen_sync(listen_ioc, saddr, errp) < 0) {
>> object_unref(OBJECT(listen_ioc));
>> - return;
>> + return NULL;
>> + }
>> +
>> + address = qio_channel_socket_get_local_address(listen_ioc, errp);
>
> [1]
>
>> + if (address < 0) {
>> + object_unref(OBJECT(listen_ioc));
>> + return NULL;
>> }
>>
>> qio_channel_add_watch(QIO_CHANNEL(listen_ioc),
>
> [2]
>
>> @@ -178,14 +186,28 @@ static void
>> socket_start_incoming_migration(SocketAddress *saddr,
>> socket_accept_incoming_migration,
>> listen_ioc,
>> (GDestroyNotify)object_unref);
>> + return address;
>> }
>>
>> void tcp_start_incoming_migration(const char *host_port, Error **errp)
>> {
>> Error *err = NULL;
>> SocketAddress *saddr = tcp_build_address(host_port, &err);
>> +
>> if (!err) {
>> - socket_start_incoming_migration(saddr, &err);
>> + SocketAddress *address = socket_start_incoming_migration(saddr,
>> &err);
>> +
>> + if (address) {
>> + unsigned long long port;
>> +
>> + if (parse_uint_full(address->u.inet.port, &port, 10) < 0) {
>> + error_setg(errp, "error parsing port in '%s'",
>> + address->u.inet.port);
>> + } else {
>> + migrate_set_port(port, errp);
>> + }
>
> If *errp is non-NULL when reach here (I don't know when this will
> happen, though), would there be a dangling incoming task in main
> thread (which was created during socket_start_incoming_migration at
> [2])?
>
> Not sure whether it can be fixed by moving the set port logic to [1]
> above. Then the watch won't be created only if set port succeeded.
Ok, will take a look at reorganizing the code.
Thanks for the review.
Later, Juan.
