Re: [PATCH v4 3/8] migration: converts socket backend to accept MigrateAddress struct

[Het Gala]

On 15/05/23 3:47 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:35PM +0000, Het Gala wrote:
> > Socket transport backend for 'migrate'/'migrate-incoming' QAPIs accept
> > new wire protocol of MigrateAddress struct.
> >
> > It is achived by parsing 'uri' string and storing migration parameters
> > required for socket connection into well defined SocketAddress struct.
> >
> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >   migration/exec.c      |  4 ++--
> >   migration/exec.h      |  4 ++++
> >   migration/migration.c | 44 +++++++++++++++++++++++++++++++------------
> >   migration/socket.c    | 34 +++++----------------------------
> >   migration/socket.h    |  7 ++++---
> >   5 files changed, 47 insertions(+), 46 deletions(-)
> >
> > diff --git a/migration/exec.c b/migration/exec.c
> > index 2bf882bbe1..c4a3293246 100644
> > --- a/migration/exec.c
> > +++ b/migration/exec.c
> > @@ -27,7 +27,6 @@
> >   #include "qemu/cutils.h"
> >   
> >   #ifdef WIN32
> > -const char *exec_get_cmd_path(void);
> >   const char *exec_get_cmd_path(void)
Even this, I will shift it to the 2nd patch, where I need to move 
exec_get_cmd_path() out accross other file (migration.c).
> >   {
> >       g_autofree char *detected_path = g_new(char, MAX_PATH);
> > @@ -40,7 +39,8 @@ const char *exec_get_cmd_path(void)
> >   }
> >   #endif
> >   
> > -void exec_start_outgoing_migration(MigrationState *s, const char *command, Error **errp)
> > +void exec_start_outgoing_migration(MigrationState *s, const char *command,
> > +                                   Error **errp)
> >   {
> >       QIOChannel *ioc;
Sure, Juan. Will change this in the 2nd patch itself instead of here. I 
am not very convinved why should we have a different patch all together 
for this, because we are just using this code outside this file in my 
opinion? But if you still think so, I can make a different patch for that.
> >   
> > diff --git a/migration/exec.h b/migration/exec.h
> > index b210ffde7a..736cd71028 100644
> > --- a/migration/exec.h
> > +++ b/migration/exec.h
> > @@ -19,6 +19,10 @@
> >   
> >   #ifndef QEMU_MIGRATION_EXEC_H
> >   #define QEMU_MIGRATION_EXEC_H
> > +
> > +#ifdef WIN32
> > +const char *exec_get_cmd_path(void);
> > +#endif
> >   void exec_start_incoming_migration(const char *host_port, Error **errp);
> >   
> >   void exec_start_outgoing_migration(MigrationState *s, const char *host_port,
> > diff --git a/migration/migration.c b/migration/migration.c
> > index a7e4e286aa..61f52d2f90 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -421,7 +421,11 @@ static bool migrate_uri_parse(const char *uri,
> >   
> >       if (strstart(uri, "exec:", NULL)) {
> >           addrs->transport = MIGRATE_TRANSPORT_EXEC;
> > +#ifdef WIN32
> > +        QAPI_LIST_APPEND(tail, g_strdup(exec_get_cmd_path()));
> > +#else
> >           QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> > +#endif
> This windows portability code should have been in the previous patch
> I think.
Ack, yes, it could have also been added in the 2nd patch. Will 
accomodate windows portability change there itself.
> >           QAPI_LIST_APPEND(tail, g_strdup("-c"));
> >           QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> >       } else if (strstart(uri, "rdma:", NULL) &&
> > @@ -450,8 +454,10 @@ static bool migrate_uri_parse(const char *uri,
> >   
> >   static void qemu_start_incoming_migration(const char *uri, Error **errp)
> >   {
> > +    Error *local_err = NULL;
> >       const char *p = NULL;
> >       MigrateAddress *channel = g_new0(MigrateAddress, 1);
> > +    SocketAddress *saddr;
> >   
> >       /* URI is not suitable for migration? */
> >       if (!migration_channels_and_uri_compatible(uri, errp)) {
> > @@ -463,23 +469,32 @@ static void qemu_start_incoming_migration(const char 
> > *uri, Error **errp)
> >           goto out;
> >       }
> >   
> > +    saddr = &channel->u.socket;
> Accessing u.socket before checkout transport == SOCKET is bad
> practice, even though this is technically safe.

Ok, Ack. I understand your point, before defining whether transport 
could be a socket or not, it is not a good way representing and defining 
it earlier.

Thanks for pointing it out Daniel. I will change this in all the places, 
where I have accessed it before confirming if the transport == socket.

> >       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> > -    if (strstart(uri, "tcp:", &p) ||
> > -        strstart(uri, "unix:", NULL) ||
> > -        strstart(uri, "vsock:", NULL)) {
> > -        socket_start_incoming_migration(p ? p : uri, errp);
> > +    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> THis should have
>
>      SocketAddress *saddr = &channe->u.socket
>
> so that 'saddr' is limited in scope to where we've validated
> transport == SOCKET
Yes, Ack. Makes sense.
> > +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> > +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> > +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> > +            socket_start_incoming_migration(saddr, &local_err);
> > +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> > +            fd_start_incoming_migration(saddr->u.fd.str, &local_err);
> > +        }
> >   #ifdef CONFIG_RDMA
> >       } else if (strstart(uri, "rdma:", &p)) {
> >           rdma_start_incoming_migration(p, errp);
> >   #endif
> >       } else if (strstart(uri, "exec:", &p)) {
> >           exec_start_incoming_migration(p, errp);
> > -    } else if (strstart(uri, "fd:", &p)) {
> > -        fd_start_incoming_migration(p, errp);
> >       } else {
> >           error_setg(errp, "unknown migration protocol: %s", uri);
> >       }
> >   
> > +    if (local_err) {
> > +        qapi_free_SocketAddress(saddr);
> > +        error_propagate(errp, local_err);
> > +        return;
Juan, I get your point. But I think, we won't be needing local_err at 
all, if I use g_autoptr for 'channel' and 'saddr' is a part of 
'channel'. Let me have a v2 patchset and if it is still not convinving, 
we can have a discussion on this.
> THis leaks 'channel', and free's 'saddr' which actually  belongs
> to channel.
>
> With my comments on the previous patch suggesting g_autoptr for
> 'channel', we don't need any free calls for 'saddr' or 'channel'.

Right. With g_autoptr used for freeing 'channel' in last patch, we wont 
have to worry about freeing 'saddr' at all. Thanks Daniel

if (local_err) {
    qapi_free_SocketAddress(saddr);
    error_propagate(errp, local_err);
    return;
}
And after changing the position for assigning 'saddr' and using 
g_autoptr for 'channel' I believe we can get rid of 'local_error' 
variable too and replace it with 'errp'. Please suggest if I am missing 
something here. TIA!

> > +    }
> > +
> >   out:
> >       qapi_free_MigrateAddress(channel);
> >   }
> > @@ -1688,6 +1703,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >       MigrationState *s = migrate_get_current();
> >       const char *p = NULL;
> >       MigrateAddress *channel = g_new0(MigrateAddress, 1);
> > +    SocketAddress *saddr;
> >   
> >       /* URI is not suitable for migration? */
> >       if (!migration_channels_and_uri_compatible(uri, errp)) {
> > @@ -1711,18 +1727,21 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> > blk,
> >           }
> >       }
> >   
> > -    if (strstart(uri, "tcp:", &p) ||
> > -        strstart(uri, "unix:", NULL) ||
> > -        strstart(uri, "vsock:", NULL)) {
> > -        socket_start_outgoing_migration(s, p ? p : uri, &local_err);
> > +    saddr = &channel->u.socket;
> Again, put this *after*  checking transport == SOCKET
Ack.
> > +    if (channel->transport == MIGRATE_TRANSPORT_SOCKET) {
> > +        if (saddr->type == SOCKET_ADDRESS_TYPE_INET ||
> > +            saddr->type == SOCKET_ADDRESS_TYPE_UNIX ||
> > +            saddr->type == SOCKET_ADDRESS_TYPE_VSOCK) {
> > +            socket_start_outgoing_migration(s, saddr, &local_err);
> > +        } else if (saddr->type == SOCKET_ADDRESS_TYPE_FD) {
> > +            fd_start_outgoing_migration(s, saddr->u.fd.str, &local_err);
> > +        }
> >   #ifdef CONFIG_RDMA
> >       } else if (strstart(uri, "rdma:", &p)) {
> >           rdma_start_outgoing_migration(s, p, &local_err);
> >   #endif
> >       } else if (strstart(uri, "exec:", &p)) {
> >           exec_start_outgoing_migration(s, p, &local_err);
> > -    } else if (strstart(uri, "fd:", &p)) {
> > -        fd_start_outgoing_migration(s, p, &local_err);
> >       } else {
> >           if (!(has_resume && resume)) {
> >               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> > @@ -1739,6 +1758,7 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >           if (!(has_resume && resume)) {
> >               yank_unregister_instance(MIGRATION_YANK_INSTANCE);
> >           }
> > +        qapi_free_SocketAddress(saddr);
> This saddr pointer belongs to 'channel' which must be freed.
Again here, with your comment in last patch to use g_autoptr, we need 
not to explicitly free 'channel' or 'saddr' right.
> >           migrate_fd_error(s, local_err);
> >           error_propagate(errp, local_err);
> >           return;
> > diff --git a/migration/socket.c b/migration/socket.c
> > index 1b6f5baefb..8e7430b266 100644
> > --- a/migration/socket.c
> > +++ b/migration/socket.c
> > @@ -108,10 +108,9 @@ out:
> >       object_unref(OBJECT(sioc));
> >   }
> >   
> > -static void
> > -socket_start_outgoing_migration_internal(MigrationState *s,
> > -                                         SocketAddress *saddr,
> > -                                         Error **errp)
> > +void socket_start_outgoing_migration(MigrationState *s,
> > +                                     SocketAddress *saddr,
> > +                                     Error **errp)
> >   {
> >       QIOChannelSocket *sioc = qio_channel_socket_new();
> >       struct SocketConnectData *data = g_new0(struct SocketConnectData, 1);
> > @@ -135,18 +134,6 @@ socket_start_outgoing_migration_internal(MigrationState *s,
> >                                        NULL);
> >   }
> >   
> > -void socket_start_outgoing_migration(MigrationState *s,
> > -                                     const char *str,
> > -                                     Error **errp)
> > -{
> > -    Error *err = NULL;
> > -    SocketAddress *saddr = socket_parse(str, &err);
> > -    if (!err) {
> > -        socket_start_outgoing_migration_internal(s, saddr, &err);
> > -    }
> > -    error_propagate(errp, err);
> > -}
> > -
Actually Juan. I don't need this function at all, because parsing of uri 
into socketAddress using socket_parse is already done. So there is no 
use of having this function in the first place, so I decided to delete 
this fucntion all together. Same with incoming function.
> >   static void socket_accept_incoming_migration(QIONetListener *listener,
> >                                                QIOChannelSocket *cioc,
> >                                                gpointer opaque)
> > @@ -172,9 +159,8 @@ socket_incoming_migration_end(void *opaque)
> >       object_unref(OBJECT(listener));
> >   }
> >   
> > -static void
> > -socket_start_incoming_migration_internal(SocketAddress *saddr,
> > -                                         Error **errp)
> > +void socket_start_incoming_migration(SocketAddress *saddr,
> > +                                     Error **errp)
> >   {
> >       QIONetListener *listener = qio_net_listener_new();
> >       MigrationIncomingState *mis = migration_incoming_get_current();
> > @@ -213,13 +199,3 @@ socket_start_incoming_migration_internal(SocketAddress 
> > *saddr,
> >       }
> >   }
> >   
> > -void socket_start_incoming_migration(const char *str, Error **errp)
> > -{
> > -    Error *err = NULL;
> > -    SocketAddress *saddr = socket_parse(str, &err);
> > -    if (!err) {
> > -        socket_start_incoming_migration_internal(saddr, &err);
> > -    }
> > -    qapi_free_SocketAddress(saddr);
> > -    error_propagate(errp, err);
> > -}
> > diff --git a/migration/socket.h b/migration/socket.h
> > index dc54df4e6c..5e4c33b8ea 100644
> > --- a/migration/socket.h
> > +++ b/migration/socket.h
> > @@ -19,13 +19,14 @@
> >   
> >   #include "io/channel.h"
> >   #include "io/task.h"
> > +#include "qemu/sockets.h"
> >   
> >   void socket_send_channel_create(QIOTaskFunc f, void *data);
> >   QIOChannel *socket_send_channel_create_sync(Error **errp);
> >   int socket_send_channel_destroy(QIOChannel *send);
> >   
> > -void socket_start_incoming_migration(const char *str, Error **errp);
> > +void socket_start_incoming_migration(SocketAddress *saddr, Error **errp);
> >   
> > -void socket_start_outgoing_migration(MigrationState *s, const char *str,
> > -                                     Error **errp);
> > +void socket_start_outgoing_migration(MigrationState *s,
> > +                                     SocketAddress *saddr, Error **errp);
> >   #endif
> > --
> > 2.22.3
> With regards,
> Daniel