Re: [PATCH 3/5] migration: Avoid multiple parsing of uri in migration code flow

[Het Gala]

On 09/01/23 7:44 pm, Daniel P. Berrangé wrote:
> On Mon, Dec 26, 2022 at 05:33:27AM +0000, Het Gala wrote:
> > From: Author Het Gala <het.gala@nutanix.com>
> >
> > Existing uri is encoded at multiple levels to extract the relevant
> > migration information.
> >
> > The modified QAPI design maps migration parameters into MigrateChannel
> > struct before, thus avoiding double-level uri encoding.
> >
> > socket_outgoing_migration() has been depricated as It's only purpose was
> > uri parsing.
> > Renamed socket_outgoing_migration_internal() as socket_outgoing_migration().
> > qemu_uri_parsing() has been introduced to parse uri string (backward
> > compatibility) and populate the MigrateChannel struct parameters. Note that
> > the function will no longer be needed once the 'uri' parameter is depricated.
> >
> >   
> >   
> > @@ -486,6 +487,39 @@ void migrate_add_address(SocketAddress *address)
> >                         QAPI_CLONE(SocketAddress, address));
> >   }
> >   
> > +static void qemu_uri_parsing(const char *uri,
> > +                             MigrateChannel **channel,
> > +                             Error **errp)
> Coding style would prefer 'bool' instad of 'void'...
>
> Also lets call this 'migrate_uri_parse'
Sure, Ack. Will change it in the upcoming patch.
> > +    if (strstart(uri, "exec:", &p)) {
> > +        addrs->transport = MIGRATE_TRANSPORT_EXEC;
> > +        addrs->u.exec.exec_str = g_strdup(p + strlen("exec:"));
> > +    } else if (strstart(uri, "rdma:", NULL)) {
> > +        addrs->transport = MIGRATE_TRANSPORT_RDMA;
> > +        addrs->u.rdma.rdma_str = g_strdup(p + strlen("rdma:"));
> > +    } 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.socket_type = saddr;
> > +    }
> > +    val->channeltype = MIGRATE_CHANNEL_TYPE_MAIN;
> > +    val->addr = addrs;
> > +    *channel = val;
> > +
> > +    if (local_err) {
> > +        error_propagate(errp, local_err);
>       ...   'return false';
> > +    }
>    ...  'return true;'
Ack.
> > +}
> > +
> >   static void qemu_start_incoming_migration(const char *uri, Error **errp)
> >   {
> >       const char *p = NULL;
> > @@ -2397,7 +2431,8 @@ void qmp_migrate(const char *uri, MigrateChannel 
> > *channel, bool has_blk,
> >   {
> >       Error *local_err = NULL;
> >       MigrationState *s = migrate_get_current();
> > -    const char *p = NULL;
> > +    MigrateAddress *addrs = g_new0(MigrateAddress, 1);
> > +    SocketAddress *saddr = g_new0(SocketAddress, 1);
> >   
> >       if (!migrate_prepare(s, has_blk && blk, has_inc && inc,
> >                            has_resume && resume, errp)) {
> > @@ -2411,20 +2446,35 @@ void qmp_migrate(const char *uri, MigrateChannel 
> > *channel, bool has_blk,
> >           }
> >       }
> >   
> > +    /*
> > +     * motive here is just to have checks and convert uri into
> > +     * socketaddress struct
> > +     */
> > +    if (uri && channel) {
> > +        error_setg(errp, "uri and channels options should be"
> > +                          "mutually exclusive");
> Needs a 'return' statement after reporting the error. ALso, this
> check should be moved to the earlier patch that introduced the
> 'channel' field.
Okay sure. Will introduce the check in 2nd patch itself.
> > +    } else if (uri) {
> > +        qemu_uri_parsing(uri, &channel, &local_err);
> Needs to 'return' on error, eg
>
>    } else if (uri && !qemu_uri_parsing(...))
>        return;
Ack.
> With regards,
> Daniel
Regards,
Het Gala