From: "Daniel P. Berrangé" <berrange@redhat.com>
To: Prasad Pandit <ppandit@redhat.com>
Cc: Fabiano Rosas <farosas@suse.de>,
qemu-devel@nongnu.org, peterx@redhat.com
Subject: Re: [PATCH v3 04/25] migration: Cleanup TLS handshake hostname passing
Date: Tue, 20 Jan 2026 11:11:02 +0000 [thread overview]
Message-ID: <aW9ixg59R8a90FIs@redhat.com> (raw)
In-Reply-To: <CAE8KmOw66B0Za=KA=TyxyWMPYSjFK=GBq7XhREevkCO4Vs+HYQ@mail.gmail.com>
On Tue, Jan 20, 2026 at 04:32:55PM +0530, Prasad Pandit wrote:
> On Fri, 9 Jan 2026 at 18:18, Fabiano Rosas <farosas@suse.de> wrote:
> > The TLS hostname is doing a tour around the world just to be cached
> > into s->hostname. We're already abusing MigrationState by doing that,
> > so incorporate the s->hostname into migration_tls_hostname() and stop
> > passing the string around.
>
> * I get the idea of not passing 's->hostname' around as a parameter, but:
> - migrate_tls_hostname() already returns '->tls_hostname->u.s',
> why are we making it return 's->hostname' too?
> - How are 'tls_hostname->u.s' and 's->hostname' different?
> + virsh(1) migrate has:
> --tls-destination <string> override the destination host name
> used for TLS verification
>
> * IIUC, when --tls-destination is supplied, s->hostname and
> tls_hostname are different, otherwise they are the same? If that is
> the case, could we set tls_hostname = s->hostname when
> params->tls_hostname is not defined below?
Normal default behaviour is that the hostname in the TLS certificate
MUST be validated against the hostname that is specified by the user
(or mgmt app) in the migration URI.
The 'tls-hostname' migration parameter is an optional override that
replaces the hostname from the migration URI, to be used in scenarios
such as:
* The URI is a UNIX socket. eg the UNIX socket points to a proxy
that is transporting data to the dest in some manner. There is
no way to know the hostname from the URI, so 'tls-hostname' must
be provided
* The URI is pointing to a proxy that forwards to the real host.
eg perhaps you setup an SSH tunnel from localhost, to the real
target. QEMU must NOT validate the remote TLS cert against
'localhost', so 'tls-hostname' must be provided by the user
Whatever is usd to validate the TLS hostname, must be data that is
provided by the user in some manner.
TL:DR: always use 'tls-hostname' if it is provided by the user,
otherwise always use the hostname from the migration URI,
otherwise do not provide a hostname.
> ===
> static void migrate_params_test_apply(MigrationParameters *params,
> MigrationParameters *dest)
> {
> if (params->tls_hostname) {
> dest->tls_hostname = QAPI_CLONE(StrOrNull, params->tls_hostname);
> } else {
> /* clear the reference, it's owned by s->parameters */
> dest->tls_hostname = NULL;
> }
> ...
> static void migrate_params_apply(MigrationParameters *params)
> {
> ...
> if (params->tls_hostname) {
> qapi_free_StrOrNull(s->parameters.tls_hostname);
> s->parameters.tls_hostname = QAPI_CLONE(StrOrNull,
> params->tls_hostname);
> }
> ...
> }
> ===
>
> > Reviewed-by: Peter Xu <peterx@redhat.com>
> > Signed-off-by: Fabiano Rosas <farosas@suse.de>
> > ---
> > migration/channel.c | 6 ++----
> > migration/channel.h | 1 -
> > migration/exec.c | 2 +-
> > migration/fd.c | 2 +-
> > migration/file.c | 2 +-
> > migration/multifd.c | 9 +++------
> > migration/options.c | 5 +++++
> > migration/postcopy-ram.c | 2 +-
> > migration/socket.c | 9 +++------
> > migration/tls.c | 17 ++++-------------
> > migration/tls.h | 2 --
> > migration/trace-events | 10 +++++-----
> > 12 files changed, 26 insertions(+), 41 deletions(-)
> >
> > diff --git a/migration/channel.c b/migration/channel.c
> > index b4ab676048..ba14f66d85 100644
> > --- a/migration/channel.c
> > +++ b/migration/channel.c
> > @@ -60,20 +60,18 @@ void migration_channel_process_incoming(QIOChannel *ioc)
> > *
> > * @s: Current migration state
> > * @ioc: Channel to which we are connecting
> > - * @hostname: Where we want to connect
> > * @error: Error indicating failure to connect, free'd here
> > */
> > void migration_channel_connect(MigrationState *s,
> > QIOChannel *ioc,
> > - const char *hostname,
> > Error *error)
> > {
> > trace_migration_set_outgoing_channel(
> > - ioc, object_get_typename(OBJECT(ioc)), hostname, error);
> > + ioc, object_get_typename(OBJECT(ioc)), error);
> >
> > if (!error) {
> > if (migrate_channel_requires_tls_upgrade(ioc)) {
> > - migration_tls_channel_connect(s, ioc, hostname, &error);
> > + migration_tls_channel_connect(s, ioc, &error);
> >
> > if (!error) {
> > /* tls_channel_connect will call back to this
> > diff --git a/migration/channel.h b/migration/channel.h
> > index 5bdb8208a7..2215091323 100644
> > --- a/migration/channel.h
> > +++ b/migration/channel.h
> > @@ -22,7 +22,6 @@ void migration_channel_process_incoming(QIOChannel *ioc);
> >
> > void migration_channel_connect(MigrationState *s,
> > QIOChannel *ioc,
> > - const char *hostname,
> > Error *error_in);
> >
> > int migration_channel_read_peek(QIOChannel *ioc,
> > diff --git a/migration/exec.c b/migration/exec.c
> > index 20e6cccf8c..78fe0fff13 100644
> > --- a/migration/exec.c
> > +++ b/migration/exec.c
> > @@ -55,7 +55,7 @@ void exec_start_outgoing_migration(MigrationState *s, strList *command,
> > }
> >
> > qio_channel_set_name(ioc, "migration-exec-outgoing");
> > - migration_channel_connect(s, ioc, NULL, NULL);
> > + migration_channel_connect(s, ioc, NULL);
> > object_unref(OBJECT(ioc));
> > }
> >
> > diff --git a/migration/fd.c b/migration/fd.c
> > index 9bf9be6acb..c956b260a4 100644
> > --- a/migration/fd.c
> > +++ b/migration/fd.c
> > @@ -70,7 +70,7 @@ void fd_start_outgoing_migration(MigrationState *s, const char *fdname, Error **
> > }
> >
> > qio_channel_set_name(ioc, "migration-fd-outgoing");
> > - migration_channel_connect(s, ioc, NULL, NULL);
> > + migration_channel_connect(s, ioc, NULL);
> > object_unref(OBJECT(ioc));
> > }
> >
> > diff --git a/migration/file.c b/migration/file.c
> > index bb8031e3c7..c490f2b219 100644
> > --- a/migration/file.c
> > +++ b/migration/file.c
> > @@ -122,7 +122,7 @@ void file_start_outgoing_migration(MigrationState *s,
> > return;
> > }
> > qio_channel_set_name(ioc, "migration-file-outgoing");
> > - migration_channel_connect(s, ioc, NULL, NULL);
> > + migration_channel_connect(s, ioc, NULL);
> > }
> >
> > static gboolean file_accept_incoming_migration(QIOChannel *ioc,
> > diff --git a/migration/multifd.c b/migration/multifd.c
> > index bf6da85af8..3fb1a07ba9 100644
> > --- a/migration/multifd.c
> > +++ b/migration/multifd.c
> > @@ -814,12 +814,10 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> > QIOChannel *ioc,
> > Error **errp)
> > {
> > - MigrationState *s = migrate_get_current();
> > - const char *hostname = s->hostname;
> > MultiFDTLSThreadArgs *args;
> > QIOChannelTLS *tioc;
> >
> > - tioc = migration_tls_client_create(ioc, hostname, errp);
> > + tioc = migration_tls_client_create(ioc, errp);
> > if (!tioc) {
> > return false;
> > }
> > @@ -829,7 +827,7 @@ static bool multifd_tls_channel_connect(MultiFDSendParams *p,
> > * created TLS channel, which has already taken a reference.
> > */
> > object_unref(OBJECT(ioc));
> > - trace_multifd_tls_outgoing_handshake_start(ioc, tioc, hostname);
> > + trace_multifd_tls_outgoing_handshake_start(ioc, tioc);
> > qio_channel_set_name(QIO_CHANNEL(tioc), "multifd-tls-outgoing");
> >
> > args = g_new0(MultiFDTLSThreadArgs, 1);
> > @@ -876,8 +874,7 @@ static void multifd_new_send_channel_async(QIOTask *task, gpointer opaque)
> > goto out;
> > }
> >
> > - trace_multifd_set_outgoing_channel(ioc, object_get_typename(OBJECT(ioc)),
> > - migrate_get_current()->hostname);
> > + trace_multifd_set_outgoing_channel(ioc, object_get_typename(OBJECT(ioc)));
> >
> > if (migrate_channel_requires_tls_upgrade(ioc)) {
> > ret = multifd_tls_channel_connect(p, ioc, &local_err);
> > diff --git a/migration/options.c b/migration/options.c
> > index 9a5a39c886..881034c289 100644
> > --- a/migration/options.c
> > +++ b/migration/options.c
> > @@ -956,6 +956,11 @@ const char *migrate_tls_hostname(void)
> > return s->parameters.tls_hostname->u.s;
> > }
> >
> > + /* hostname saved from a previously connected channel */
> > + if (s->hostname) {
> > + return s->hostname;
> > + }
> > +
>
> * Maybe this return 's->hostname' can be avoided by setting
> tls_hostname = s->hostname at the initialisation stage. If
> 'tls_hostname' and 's->hostname' are different, will the
> migrate_tls_hostname() callers work correctly with s->hostname?
>
> > return NULL;
> > }
> >
> > diff --git a/migration/postcopy-ram.c b/migration/postcopy-ram.c
> > index 98a98138be..7afb42bd27 100644
> > --- a/migration/postcopy-ram.c
> > +++ b/migration/postcopy-ram.c
> > @@ -1966,7 +1966,7 @@ postcopy_preempt_send_channel_new(QIOTask *task, gpointer opaque)
> > }
> >
> > if (migrate_channel_requires_tls_upgrade(ioc)) {
> > - tioc = migration_tls_client_create(ioc, s->hostname, &local_err);
> > + tioc = migration_tls_client_create(ioc, &local_err);
> > if (!tioc) {
> > goto out;
> > }
> > diff --git a/migration/socket.c b/migration/socket.c
> > index 9e379bf56f..426f363b99 100644
> > --- a/migration/socket.c
> > +++ b/migration/socket.c
> > @@ -44,7 +44,6 @@ void socket_send_channel_create(QIOTaskFunc f, void *data)
> >
> > struct SocketConnectData {
> > MigrationState *s;
> > - char *hostname;
> > };
> >
> > static void socket_connect_data_free(void *opaque)
> > @@ -53,7 +52,6 @@ static void socket_connect_data_free(void *opaque)
> > if (!data) {
> > return;
> > }
> > - g_free(data->hostname);
> > g_free(data);
> > }
> >
> > @@ -69,7 +67,7 @@ static void socket_outgoing_migration(QIOTask *task,
> > goto out;
> > }
> >
> > - trace_migration_socket_outgoing_connected(data->hostname);
> > + trace_migration_socket_outgoing_connected();
> >
> > if (migrate_zero_copy_send() &&
> > !qio_channel_has_feature(sioc, QIO_CHANNEL_FEATURE_WRITE_ZERO_COPY)) {
> > @@ -77,7 +75,7 @@ static void socket_outgoing_migration(QIOTask *task,
> > }
> >
> > out:
> > - migration_channel_connect(data->s, sioc, data->hostname, err);
> > + migration_channel_connect(data->s, sioc, err);
> > object_unref(OBJECT(sioc));
> > }
> >
> > @@ -96,7 +94,7 @@ void socket_start_outgoing_migration(MigrationState *s,
> > outgoing_args.saddr = addr;
> >
> > if (saddr->type == SOCKET_ADDRESS_TYPE_INET) {
> > - data->hostname = g_strdup(saddr->u.inet.host);
> > + s->hostname = g_strdup(saddr->u.inet.host);
> > }
> >
> > qio_channel_set_name(QIO_CHANNEL(sioc), "migration-socket-outgoing");
> > @@ -180,4 +178,3 @@ void socket_start_incoming_migration(SocketAddress *saddr,
> > qapi_free_SocketAddress(address);
> > }
> > }
> > -
> > diff --git a/migration/tls.c b/migration/tls.c
> > index 1df31bdcbb..82f58cbc78 100644
> > --- a/migration/tls.c
> > +++ b/migration/tls.c
> > @@ -112,12 +112,11 @@ static void migration_tls_outgoing_handshake(QIOTask *task,
> > } else {
> > trace_migration_tls_outgoing_handshake_complete();
> > }
> > - migration_channel_connect(s, ioc, NULL, err);
> > + migration_channel_connect(s, ioc, err);
> > object_unref(OBJECT(ioc));
> > }
> >
> > QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc,
> > - const char *hostname,
> > Error **errp)
> > {
> > QCryptoTLSCreds *creds;
> > @@ -127,29 +126,21 @@ QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc,
> > return NULL;
> > }
> >
> > - const char *tls_hostname = migrate_tls_hostname();
> > - if (tls_hostname) {
> > - hostname = tls_hostname;
> > - }
> > -
> > - return qio_channel_tls_new_client(ioc, creds, hostname, errp);
> > + return qio_channel_tls_new_client(ioc, creds, migrate_tls_hostname(), errp);
> > }
> >
> > void migration_tls_channel_connect(MigrationState *s,
> > QIOChannel *ioc,
> > - const char *hostname,
> > Error **errp)
> > {
> > QIOChannelTLS *tioc;
> >
> > - tioc = migration_tls_client_create(ioc, hostname, errp);
> > + tioc = migration_tls_client_create(ioc, errp);
> > if (!tioc) {
> > return;
> > }
> >
> > - /* Save hostname into MigrationState for handshake */
> > - s->hostname = g_strdup(hostname);
> > - trace_migration_tls_outgoing_handshake_start(hostname);
> > + trace_migration_tls_outgoing_handshake_start();
> > qio_channel_set_name(QIO_CHANNEL(tioc), "migration-tls-outgoing");
> >
> > if (migrate_postcopy_ram() || migrate_return_path()) {
> > diff --git a/migration/tls.h b/migration/tls.h
> > index 7607cfe803..7cd9c76013 100644
> > --- a/migration/tls.h
> > +++ b/migration/tls.h
> > @@ -27,12 +27,10 @@
> > void migration_tls_channel_process_incoming(QIOChannel *ioc, Error **errp);
> >
> > QIOChannelTLS *migration_tls_client_create(QIOChannel *ioc,
> > - const char *hostname,
> > Error **errp);
> >
> > void migration_tls_channel_connect(MigrationState *s,
> > QIOChannel *ioc,
> > - const char *hostname,
> > Error **errp);
> > void migration_tls_channel_end(QIOChannel *ioc, Error **errp);
> > /* Whether the QIO channel requires further TLS handshake? */
> > diff --git a/migration/trace-events b/migration/trace-events
> > index bf11b62b17..da8f909cac 100644
> > --- a/migration/trace-events
> > +++ b/migration/trace-events
> > @@ -149,10 +149,10 @@ multifd_send_sync_main_wait(uint8_t id) "channel %u"
> > multifd_send_terminate_threads(void) ""
> > multifd_send_thread_end(uint8_t id, uint64_t packets) "channel %u packets %" PRIu64
> > multifd_send_thread_start(uint8_t id) "%u"
> > -multifd_tls_outgoing_handshake_start(void *ioc, void *tioc, const char *hostname) "ioc=%p tioc=%p hostname=%s"
> > +multifd_tls_outgoing_handshake_start(void *ioc, void *tioc) "ioc=%p tioc=%p"
> > multifd_tls_outgoing_handshake_error(void *ioc, const char *err) "ioc=%p err=%s"
> > multifd_tls_outgoing_handshake_complete(void *ioc) "ioc=%p"
> > -multifd_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostname) "ioc=%p ioctype=%s hostname=%s"
> > +multifd_set_outgoing_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
> >
> > # migration.c
> > migrate_set_state(const char *new_state) "new state %s"
> > @@ -204,7 +204,7 @@ migration_transferred_bytes(uint64_t qemu_file, uint64_t multifd, uint64_t rdma)
> >
> > # channel.c
> > migration_set_incoming_channel(void *ioc, const char *ioctype) "ioc=%p ioctype=%s"
> > -migration_set_outgoing_channel(void *ioc, const char *ioctype, const char *hostname, void *err) "ioc=%p ioctype=%s hostname=%s err=%p"
> > +migration_set_outgoing_channel(void *ioc, const char *ioctype, void *err) "ioc=%p ioctype=%s err=%p"
> >
> > # global_state.c
> > migrate_state_too_big(void) ""
> > @@ -328,11 +328,11 @@ migration_file_incoming(const char *filename) "filename=%s"
> >
> > # socket.c
> > migration_socket_incoming_accepted(void) ""
> > -migration_socket_outgoing_connected(const char *hostname) "hostname=%s"
> > +migration_socket_outgoing_connected(void) ""
> > migration_socket_outgoing_error(const char *err) "error=%s"
> >
> > # tls.c
> > -migration_tls_outgoing_handshake_start(const char *hostname) "hostname=%s"
> > +migration_tls_outgoing_handshake_start(void) ""
> > migration_tls_outgoing_handshake_error(const char *err) "err=%s"
> > migration_tls_outgoing_handshake_complete(void) ""
> > migration_tls_incoming_handshake_start(void) ""
> > --
>
> * Otherwise change looks okay.
> Reviewed-by: Prasad Pandit <pjp@fedoraproject.org>
>
> Thank you.
> ---
> - Prasad
>
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
next prev parent reply other threads:[~2026-01-20 11:11 UTC|newest]
Thread overview: 58+ messages / expand[flat|nested] mbox.gz Atom feed top
2026-01-09 12:40 [PATCH v3 00/25] migration: Cleanup early connection code Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 01/25] migration: Remove redundant state change Fabiano Rosas
2026-01-13 12:33 ` Prasad Pandit
2026-01-13 13:25 ` Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 02/25] migration: Fix state change at migration_channel_process_incoming Fabiano Rosas
2026-01-13 12:39 ` Prasad Pandit
2026-01-13 13:27 ` Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 03/25] migration/tls: Remove unused parameter Fabiano Rosas
2026-01-19 12:37 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 04/25] migration: Cleanup TLS handshake hostname passing Fabiano Rosas
2026-01-20 11:02 ` Prasad Pandit
2026-01-20 11:11 ` Daniel P. Berrangé [this message]
2026-01-20 11:37 ` Prasad Pandit
2026-01-20 14:51 ` Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 05/25] migration: Move postcopy_try_recover into migration_incoming_process Fabiano Rosas
2026-01-19 11:38 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 06/25] migration: Use migrate_mode() to query for cpr-transfer Fabiano Rosas
2026-01-19 12:06 ` Prasad Pandit
2026-01-20 17:52 ` Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 07/25] migration: Free the error earlier in the resume case Fabiano Rosas
2026-01-15 11:54 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 08/25] migration: Move error reporting out of migration_cleanup Fabiano Rosas
2026-01-19 12:32 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 09/25] migration: Expand migration_connect_error_propagate to cover cancelling Fabiano Rosas
2026-01-20 9:15 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 10/25] migration: yank: Move register instance earlier Fabiano Rosas
2026-01-20 9:01 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 11/25] migration: Fold migration_cleanup() into migration_connect_error_propagate() Fabiano Rosas
2026-01-16 12:25 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 12/25] migration: Handle error in the early async paths Fabiano Rosas
2026-01-16 11:17 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 13/25] migration: Move setting of QEMUFile into migration_outgoing|incoming_setup Fabiano Rosas
2026-01-19 12:22 ` Prasad Pandit
2026-01-20 18:01 ` Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 14/25] migration/rdma: Use common connection paths Fabiano Rosas
2026-01-19 12:27 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 15/25] migration: Start incoming from channel.c Fabiano Rosas
2026-01-19 12:24 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 16/25] migration/channel: Rename migration_channel_connect Fabiano Rosas
2026-01-20 11:10 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 17/25] migration: Rename instances of start Fabiano Rosas
2026-01-20 11:21 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 18/25] migration: Move channel code to channel.c Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 19/25] migration: Move transport connection code into channel.c Fabiano Rosas
2026-01-20 9:40 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 20/25] migration: Move channel parsing to channel.c Fabiano Rosas
2026-01-20 10:15 ` Prasad Pandit
2026-01-20 18:18 ` Fabiano Rosas
2026-01-09 12:40 ` [PATCH v3 21/25] migration: Move URI " Fabiano Rosas
2026-01-20 10:20 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 22/25] migration: Free cpr-transfer MigrationAddress along with gsource Fabiano Rosas
2026-01-20 11:17 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 23/25] migration: Move CPR HUP watch to cpr-transfer.c Fabiano Rosas
2026-01-20 11:24 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 24/25] migration: Remove qmp_migrate_finish Fabiano Rosas
2026-01-20 11:07 ` Prasad Pandit
2026-01-09 12:40 ` [PATCH v3 25/25] migration/channel: Centralize calling migration_channel_connect_outgoing Fabiano Rosas
2026-01-19 11:28 ` Prasad Pandit
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=aW9ixg59R8a90FIs@redhat.com \
--to=berrange@redhat.com \
--cc=farosas@suse.de \
--cc=peterx@redhat.com \
--cc=ppandit@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox