Re: [Qemu-devel] [PATCH v5 2/5] migration: Set the migration tcp port

[Peter Xu]

On Wed, Jan 31, 2018 at 01:35:33PM +0100, Juan Quintela wrote:
> Peter Xu <address@hidden> 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 <address@hidden>
> >> 
> >> --
> >> 
> >> 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?

How about set this directly? :)  Like:

  migrate_get_current()->parameters.x_tcp_port = xxx;

Maybe also add a comment showing that this is a special case.  I just
feel strange that if user can set it, but I'll follow your decision
even if you really want to keep the qmp_*() call since after all this
is for debugging, and QEMU itself won't use this value now.

Thanks,

-- 
Peter Xu