Re: [PATCH v4 2/8] migration: Converts uri parameter into 'MigrateAddress' struct

[Het Gala]

Just so that, there is a wider attention, I will try to address and 
discuss the comments from Daniel and Juan both here, as many of them 
seems to be overlapping. I hope that is fine with the maintainers.

On 15/05/23 3:42 pm, Daniel P. Berrangé wrote:
> On Fri, May 12, 2023 at 02:32:34PM +0000, Het Gala wrote:
> > This patch introduces code that can parse 'uri' string parameter and
> > spit out 'MigrateAddress' struct. All the required migration parameters
> > are stored in the struct.
> >
> > Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> > Signed-off-by: Het Gala <het.gala@nutanix.com>
> > ---
> >   migration/migration.c | 63 +++++++++++++++++++++++++++++++++++++++++--
> >   1 file changed, 61 insertions(+), 2 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index 0ee07802a5..a7e4e286aa 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -64,6 +64,7 @@
> >   #include "yank_functions.h"
> >   #include "sysemu/qtest.h"
> >   #include "options.h"
> > +#include "qemu/sockets.h"
> >   
> >   static NotifierList migration_state_notifiers =
> >       NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
> > @@ -408,13 +409,58 @@ void migrate_add_address(SocketAddress *address)
> >                         QAPI_CLONE(SocketAddress, address));
> >   }
> >   
> > +static bool migrate_uri_parse(const char *uri,
> > +                              MigrateAddress **channel,
> > +                              Error **errp)
> > +{
> > +    Error *local_err = NULL;
> > +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> > +    SocketAddress *saddr;
> > +    InetSocketAddress *isock = &addrs->u.rdma;
> > +    strList **tail = &addrs->u.exec.args;
> > +
> > +    if (strstart(uri, "exec:", NULL)) {
> > +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> > +        QAPI_LIST_APPEND(tail, g_strdup("/bin/sh"));
> > +        QAPI_LIST_APPEND(tail, g_strdup("-c"));
> > +        QAPI_LIST_APPEND(tail, g_strdup(uri + strlen("exec:")));
> > +    } else if (strstart(uri, "rdma:", NULL) &&
> > +               !inet_parse(isock, uri + strlen("rdma:"), errp)) {
> > +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> I would have this as
>
>      } else if (strstart(uri, "rdma:", NULL)) {
>          if (inet_parse(isock, uri + strlen("rdma:"), errp)) {
>              addrs->transport = MIGRATE_TRANSPORT_RDMA;
>         }
>
> as IMHO it is bad practice to have control pass to the next
> else if clause when inet_parse() fails, as we know this is
> only an RDMA addr
Ack. I will change in the next patch.
> Also you need to use '&local_err' not 'errp' in the inet_parse
> call, otherwise the later code block for cleanup won't run.

Yes, thanks for pointing it out Daniel. Will modify that.

Also, Juan is of the opinion that we could omit 'local_error' variable 
and try to address and free the memory there itself. For ex:

if (saddr == NULL) {
    qapi_free_MigrateAddress(addrs);
    return false;
}

Or, Daniel, can I also define here the variables like you suggested down 
in the patch ? or is it used in some special case or I am missing 
something ?

g_autoptr(MigrateAddress) addrs = g_new0(MigrateAddress, 1);

So we would not have to worry to free MigrateAddress struct.

> > +    } else if (strstart(uri, "tcp:", NULL) ||
> > +                strstart(uri, "unix:", NULL) ||
> > +                strstart(uri, "vsock:", NULL) ||
> > +                strstart(uri, "fd:", NULL)) {
> > +        addrs->transport = MIGRATE_TRANSPORT_SOCKET;
> > +        saddr = socket_parse(uri, &local_err);
> > +        addrs->u.socket = *saddr;
> Protect with
>
>     if (saddr != NULL) {
>         addrs->u.socket = *saddr;
>     }
>
> > +    }
> > +
> > +    if (local_err) {
> > +        qapi_free_MigrateAddress(addrs);
> > +        qapi_free_SocketAddress(saddr);
> > +        qapi_free_InetSocketAddress(isock);
> > +        error_propagate(errp, local_err);
> > +        return false;
> > +    }
> > +
> > +    *channel = addrs;
> > +    return true;
> > +}
> > +
> >   static void qemu_start_incoming_migration(const char *uri, Error **errp)
> >   {
> >       const char *p = NULL;
> > +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> Avoid the later 'out:' cleanup block by using:
>
>    g_autoptr(MigrateAddress) channel = g_new0(MigrateAddress, 1);
Ack. I think this also solves the doubt raised by Juan "I wish, I really 
wish, that there was a way to free things on error". Am I right ?
> >   
> >       /* URI is not suitable for migration? */
> >       if (!migration_channels_and_uri_compatible(uri, errp)) {
> > -        return;
> > +        goto out;
> > +    }
> > +
> > +    if (uri && !migrate_uri_parse(uri, &channel, errp)) {
> > +        error_setg(errp, "Error parsing uri");
> This error_setg() is overwriting the error migrate_uri_parse already
> set, so just drop it.

Okay, I got the point. I thought error_setg() would add error statement 
on top of errp, but I was incorrect.

Juan also made the same point I believe. I will remove error_setg() in 
the next version of patchset series.

> > +        goto out;
> >       }
> >   
> >       qapi_event_send_migration(MIGRATION_STATUS_SETUP);
> > @@ -433,6 +479,9 @@ static void qemu_start_incoming_migration(const char *uri, 
> > Error **errp)
> >       } else {
> >           error_setg(errp, "unknown migration protocol: %s", uri);
> >       }
> > +
> > +out:
> > +    qapi_free_MigrateAddress(channel);
> >   }
> >   
> >   static void process_incoming_migration_bh(void *opaque)
> > @@ -1638,10 +1687,16 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> > blk,
> >       Error *local_err = NULL;
> >       MigrationState *s = migrate_get_current();
> >       const char *p = NULL;
> > +    MigrateAddress *channel = g_new0(MigrateAddress, 1);
> Here too, use g_autoptr(MigrateAddress) and drop the  'out:' block
Ack.
> >   
> >       /* URI is not suitable for migration? */
> >       if (!migration_channels_and_uri_compatible(uri, errp)) {
> > -        return;
> > +        goto out;
> > +    }
> > +
> > +    if (!migrate_uri_parse(uri, &channel, &local_err)) {
> > +        error_setg(errp, "Error parsing uri");
> > +        goto out;
> >       }
> >   
> >       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> > @@ -1688,6 +1743,10 @@ void qmp_migrate(const char *uri, bool has_blk, bool blk,
> >           error_propagate(errp, local_err);
> >           return;
> >       }
> > +
> > +out:
> > +    qapi_free_MigrateAddress(channel);
> > +    return;
> >   }
> >   
> >   void qmp_migrate_cancel(Error **errp)
> > --
> > 2.22.3
> With regards,
> Daniel
Regards,
Het Gala