Re: [PATCH v2 3/6] migration: HMP side changes for modified 'migrate' QAPI design

[Daniel P . Berrangé]

On Wed, Feb 08, 2023 at 09:35:57AM +0000, Het Gala wrote:
> hmp_migrate() stores modified QAPI 'migrate' arguments from qdict
> into well defined MigrateChannel struct with help of
> migrate_channel_from_qdict().
> hmp_migrate() also accepts uri string as modified QAPI a 'migrate'
> argument (for backward compatibility).
> 
> Suggested-by: Daniel P. Berrange <berrange@redhat.com>
> Suggested-by: Manish Mishra <manish.mishra@nutanix.com>
> Suggested-by: Aravind Retnakaran <aravind.retnakaran@nutanix.com>
> Signed-off-by: Het Gala <het.gala@nutanix.com>
> ---
>  migration/migration-hmp-cmds.c | 105 ++++++++++++++++++++++++++++++++-
>  migration/migration.c          |  15 ++++-
>  2 files changed, 116 insertions(+), 4 deletions(-)
> 
> diff --git a/migration/migration-hmp-cmds.c b/migration/migration-hmp-cmds.c
> index ef25bc8929..a9f65ded7a 100644
> --- a/migration/migration-hmp-cmds.c
> +++ b/migration/migration-hmp-cmds.c
> @@ -32,6 +32,101 @@
>  #include "sysemu/runstate.h"
>  #include "ui/qemu-spice.h"
>  
> +static bool
> +migrate_channel_from_qdict(MigrateChannel **channel,
> +                           const QDict *qdict, Error **errp)
> +{
> +    Error *err = NULL;
> +    const char *channeltype  = qdict_get_try_str(qdict, "channeltype");
> +    const char *transport_str = qdict_get_try_str(qdict, "transport");
> +    const char *socketaddr_type = qdict_get_try_str(qdict, "type");
> +    const char *inet_host = qdict_get_try_str(qdict, "host");
> +    const char *inet_port = qdict_get_try_str(qdict, "port");
> +    const char *unix_path = qdict_get_try_str(qdict, "path");
> +    const char *vsock_cid = qdict_get_try_str(qdict, "cid");
> +    const char *vsock_port = qdict_get_try_str(qdict, "port");
> +    const char *fd = qdict_get_try_str(qdict, "str");
> +    QList *exec = qdict_get_qlist(qdict, "exec");
> +    MigrateChannel *val = g_new0(MigrateChannel, 1);
> +    MigrateChannelType channel_type;
> +    MigrateTransport transport;
> +    MigrateAddress *addr = g_new0(MigrateAddress, 1);
> +    SocketAddress *saddr = g_new(SocketAddress, 1);
> +    SocketAddressType type;
> +    InetSocketAddress *isock = g_new0(InetSocketAddress, 1);
> +
> +    channel_type = qapi_enum_parse(&MigrateChannelType_lookup,
> +                                   channeltype, -1, &err);
> +    if (channel_type < 0) {
> +        goto end;
> +    }
> +
> +    transport = qapi_enum_parse(&MigrateTransport_lookup,
> +                                transport_str, -1, &err);
> +    if (transport < 0) {
> +        goto end;
> +    }
> +
> +    type = qapi_enum_parse(&SocketAddressType_lookup,
> +                           socketaddr_type, -1, &err);
> +    if (type < 0) {
> +        goto end;
> +    }
> +
> +    addr->transport = transport;
> +
> +    switch (transport) {
> +    case MIGRATE_TRANSPORT_SOCKET:
> +        saddr->type = type;
> +
> +        switch (type) {
> +        case SOCKET_ADDRESS_TYPE_INET:
> +            saddr->u.inet.host = (char *)inet_host;
> +            saddr->u.inet.port = (char *)inet_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_UNIX:
> +            saddr->u.q_unix.path = (char *)unix_path;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_VSOCK:
> +            saddr->u.vsock.cid = (char *)vsock_cid;
> +            saddr->u.vsock.port = (char *)vsock_port;
> +            break;
> +        case SOCKET_ADDRESS_TYPE_FD:
> +            saddr->u.fd.str = (char *)fd;
> +            break;
> +        default:
> +            error_setg(errp, "%s: Unknown socket type %d",
> +                       __func__, saddr->type);
> +            break;
> +        }
> +
> +        addr->u.socket.socket_type = saddr;
> +        break;
> +    case MIGRATE_TRANSPORT_EXEC:
> +        addr->u.exec.data = (strList *)exec;
> +         break;
> +    case MIGRATE_TRANSPORT_RDMA:
> +        isock->host = (char *)inet_host;
> +        isock->port = (char *)inet_port;
> +        addr->u.rdma.data = isock;
> +        break;
> +    default:
> +        error_setg(errp, "%s: Unknown migrate transport type %d",
> +                   __func__, addr->transport);
> +        break;
> +    }
> +
> +    val->channeltype = channel_type;
> +    val->addr = addr;
> +    *channel = val;
> +    return true;
> +
> +end:
> +    error_propagate(errp, err);
> +    return false;
> +}
> +
> +
>  void hmp_info_migrate(Monitor *mon, const QDict *qdict)
>  {
>      MigrationInfo *info;
> @@ -701,8 +796,16 @@ void hmp_migrate(Monitor *mon, const QDict *qdict)
>      const char *uri = qdict_get_str(qdict, "uri");
>      Error *err = NULL;
>  
> -    qmp_migrate(uri, !!blk, blk, !!inc, inc,
> +    MigrateChannel *channel = g_new0(MigrateChannel, 1);
> +
> +    if (!migrate_channel_from_qdict(&channel, qdict, &err)) {
> +        error_setg(&err, "error in retrieving channel from qdict");
> +        return;
> +    }
> +
> +    qmp_migrate(uri, channel, !!blk, blk, !!inc, inc,
>                  false, false, true, resume, &err);
> +    qapi_free_MigrateChannel(channel);
>      if (hmp_handle_error(mon, err)) {
>          return;
>      }
> diff --git a/migration/migration.c b/migration/migration.c
> index 7a14aa98d8..f6dd8dbb03 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2463,9 +2463,9 @@ static bool migrate_prepare(MigrationState *s, bool 
> blk, bool blk_inc,
>      return true;
>  }
>  
> -void qmp_migrate(const char *uri, bool has_blk, bool blk,
> -                 bool has_inc, bool inc, bool has_detach, bool detach,
> -                 bool has_resume, bool resume, Error **errp)
> +void qmp_migrate(const char *uri, MigrateChannel *channel, bool has_blk,
> +                 bool blk, bool has_inc, bool inc, bool has_detach,
> +                 bool detach, bool has_resume, bool resume, Error **errp)
>  {
>      Error *local_err = NULL;
>      MigrationState *s = migrate_get_current();
> @@ -2483,6 +2483,15 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> blk,
>          }
>      }
>  
> +    /*
> +     * Having preliminary checks for uri and channel
> +     */
> +    if (uri && channel) {
> +        error_setg(errp, "uri and channels options should be"

s/should be/are/, also best to quote parameter names, eg

    error_setg(errp, 
               "'uri' and 'channels' options are mutually exclusive");

> +                          "mutually exclusive");
> +        return;
> +    }
> +

This change for qmp_migrate will need to be in patch 2.

QEMU needs to compile & pass tests successfully after each individual
patch. Currently it'll fail on patch 2, because the code generator
wil emit the new qmp_migrate API declaration signature, but the change
to the implementation signature is here in patch 3.

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 :|