| Message ID | 20180307110010.2205-4-quintela@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Multifd | expand |
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> > > -- > > This used to be uri parameter, but it has so many troubles to > reproduce that it don't just make sense. > --- > hmp.c | 3 +++ > migration/migration.c | 8 ++++++++ > qapi/migration.json | 19 ++++++++++++++++--- > 3 files changed, 27 insertions(+), 3 deletions(-) > diff --git a/qapi/migration.json b/qapi/migration.json > index 7f465a1902..b6ef193f47 100644 > --- a/qapi/migration.json > +++ b/qapi/migration.json > @@ -490,6 +490,9 @@ > # 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 > ## > { 'enum': 'MigrationParameter', > @@ -498,7 +501,7 @@ > 'tls-creds', 'tls-hostname', 'max-bandwidth', > 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', > 'x-multifd-channels', 'x-multifd-page-count', > - 'xbzrle-cache-size' ] } > + 'xbzrle-cache-size', 'x-tcp-port' ] } > > ## > # @MigrateSetParameters: > @@ -566,6 +569,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 > ## > # 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' > # @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'] 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. Regards, Daniel
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.
diff --git a/hmp.c b/hmp.c index 016cb5c4f1..b37605d86a 100644 --- a/hmp.c +++ b/hmp.c @@ -355,6 +355,9 @@ void hmp_info_migrate_parameters(Monitor *mon, const QDict *qdict) monitor_printf(mon, "%s: %" PRIu64 "\n", MigrationParameter_str(MIGRATION_PARAMETER_XBZRLE_CACHE_SIZE), params->xbzrle_cache_size); + monitor_printf(mon, "%s: %d\n", + MigrationParameter_str(MIGRATION_PARAMETER_X_TCP_PORT), + params->x_tcp_port); } qapi_free_MigrationParameters(params); diff --git a/migration/migration.c b/migration/migration.c index e345d0cc7e..31b16a335b 100644 --- a/migration/migration.c +++ b/migration/migration.c @@ -545,6 +545,8 @@ 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; + params->has_x_tcp_port = true; + params->x_tcp_port = s->parameters.x_tcp_port; return params; } @@ -912,6 +914,9 @@ static void migrate_params_test_apply(MigrateSetParameters *params, if (params->has_xbzrle_cache_size) { dest->xbzrle_cache_size = params->xbzrle_cache_size; } + if (params->has_x_tcp_port) { + dest->x_tcp_port = params->x_tcp_port; + } } static void migrate_params_apply(MigrateSetParameters *params, Error **errp) @@ -984,6 +989,9 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp) s->parameters.xbzrle_cache_size = params->xbzrle_cache_size; xbzrle_cache_resize(params->xbzrle_cache_size, errp); } + if (params->has_x_tcp_port) { + s->parameters.x_tcp_port = params->x_tcp_port; + } } void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp) diff --git a/qapi/migration.json b/qapi/migration.json index 7f465a1902..b6ef193f47 100644 --- a/qapi/migration.json +++ b/qapi/migration.json @@ -490,6 +490,9 @@ # 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 ## { 'enum': 'MigrationParameter', @@ -498,7 +501,7 @@ 'tls-creds', 'tls-hostname', 'max-bandwidth', 'downtime-limit', 'x-checkpoint-delay', 'block-incremental', 'x-multifd-channels', 'x-multifd-page-count', - 'xbzrle-cache-size' ] } + 'xbzrle-cache-size', 'x-tcp-port' ] } ## # @MigrateSetParameters: @@ -566,6 +569,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 ## # 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'} } ## # @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'} } ## # @query-migrate-parameters:
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> -- This used to be uri parameter, but it has so many troubles to reproduce that it don't just make sense. --- hmp.c | 3 +++ migration/migration.c | 8 ++++++++ qapi/migration.json | 19 ++++++++++++++++--- 3 files changed, 27 insertions(+), 3 deletions(-)