Re: [PATCH V4 09/19] migration: incoming channel

[Markus Armbruster <armbru@redhat.com>]

Steve Sistare <steven.sistare@oracle.com> writes:

> Extend the -incoming option to allow an @MigrationChannel to be specified.
> This allows channels other than 'main' to be described on the command
> line, which will be needed for CPR.
>
> Signed-off-by: Steve Sistare <steven.sistare@oracle.com>

[...]

> diff --git a/qemu-options.hx b/qemu-options.hx
> index 02b9118..fab50ce 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -4937,10 +4937,17 @@ DEF("incoming", HAS_ARG, QEMU_OPTION_incoming, \
>      "-incoming exec:cmdline\n" \
>      "                accept incoming migration on given file descriptor\n" \
>      "                or from given external command\n" \
> +    "-incoming @MigrationChannel\n" \
> +    "                accept incoming migration on the channel\n" \
>      "-incoming defer\n" \
>      "                wait for the URI to be specified via migrate_incoming\n",
>      QEMU_ARCH_ALL)
>  SRST
> +The -incoming option specifies the migration channel for an incoming
> +migration.  It may be used multiple times to specify multiple
> +migration channel types.

Really?  If I understand the code below correctly, the last -incoming
wins, and any previous ones are silently ignored.

>                            The channel type is specified in @MigrationChannel,
> +and is 'main' for all other forms of -incoming.
> +
>  ``-incoming tcp:[host]:port[,to=maxport][,ipv4=on|off][,ipv6=on|off]``
>    \ 
>  ``-incoming rdma:host:port[,ipv4=on|off][,ipv6=on|off]``
> @@ -4960,6 +4967,16 @@ SRST
>      Accept incoming migration as an output from specified external
>      command.
>  
> +``-incoming @MigrationChannel``
> +    Accept incoming migration on the channel.  See the QAPI documentation
> +    for the syntax of the @MigrationChannel data element.  For example:
> +    ::

I get what you're trying to express, but there's no precedence for
referring to QAPI types like @TypeName in option documentation.  But
let's ignore this until after we nailed down the actual interface, on
which I have questions below.

> +
> +        -incoming '{"channel-type": "main",
> +                    "addr": { "transport": "socket",
> +                              "type": "unix",
> +                              "path": "my.sock" }}'
> +
>  ``-incoming defer``
>      Wait for the URI to be specified via migrate\_incoming. The monitor
>      can be used to change settings (such as migration parameters) prior
> diff --git a/system/vl.c b/system/vl.c
> index 4151a79..2c24c60 100644
> --- a/system/vl.c
> +++ b/system/vl.c
> @@ -123,6 +123,7 @@
>  #include "qapi/qapi-visit-block-core.h"
>  #include "qapi/qapi-visit-compat.h"
>  #include "qapi/qapi-visit-machine.h"
> +#include "qapi/qapi-visit-migration.h"
>  #include "qapi/qapi-visit-ui.h"
>  #include "qapi/qapi-commands-block-core.h"
>  #include "qapi/qapi-commands-migration.h"
> @@ -159,6 +160,7 @@ typedef struct DeviceOption {
>  static const char *cpu_option;
>  static const char *mem_path;
>  static const char *incoming;
> +static MigrationChannelList *incoming_channels;
>  static const char *loadvm;
>  static const char *accelerators;
>  static bool have_custom_ram_size;
> @@ -1821,6 +1823,35 @@ static void object_option_add_visitor(Visitor *v)
>      QTAILQ_INSERT_TAIL(&object_opts, opt, next);
>  }
>  
> +static void incoming_option_parse(const char *str)
> +{
> +    MigrationChannel *channel;
> +
> +    if (str[0] == '{') {
> +        QObject *obj = qobject_from_json(str, &error_fatal);
> +        Visitor *v = qobject_input_visitor_new(obj);
> +
> +        qobject_unref(obj);
> +        visit_type_MigrationChannel(v, "channel", &channel, &error_fatal);
> +        visit_free(v);
> +    } else if (!strcmp(str, "defer")) {
> +        channel = NULL;
> +    } else {
> +        migrate_uri_parse(str, &channel, &error_fatal);
> +    }
> +
> +    /* New incoming spec replaces the previous */
> +
> +    if (incoming_channels) {
> +        qapi_free_MigrationChannelList(incoming_channels);
> +    }
> +    if (channel) {
> +        incoming_channels = g_new0(MigrationChannelList, 1);
> +        incoming_channels->value = channel;
> +    }
> +    incoming = str;
> +}

@incoming is set to @optarg.

@incoming_channels is set to a MigrationChannelList of exactly one
element, parsed from @incoming.  Except when @incoming is "defer", then
@incoming_channels is set to null.

@incoming is only ever used as a flag.  Turn it into a bool?

Oh, wait...  see my comment on the next hunk.

Option -incoming resembles QMP command migrate-incoming.  Differences:

* migrate-incoming keeps legacy URI and modern argument separate: there
  are two named arguments, and exactly one of them must be passed.
  -incoming overloads them: if @optarg starts with '{', it's modern,
  else legacy URI.

  Because of that, -incoming *only* supports JSON syntax for modern, not
  dotted keys.  Other JSON-capable arguments support both.

  How can a management application detect that -incoming supports
  modern?

  Sure overloading -incoming this way is a good idea?

* migrate-incoming takes a list of channels, currently restricted to a
  single channel.  -incoming takes a channel.  If we lift the
  restriction, -incoming syntax will become even messier: we'll have to
  additionally overload list of channel.

  Should -incoming take a list from the start, like migrate-incoming
  does?

> +
>  static void object_option_parse(const char *str)
>  {
>      QemuOpts *opts;
> @@ -2730,7 +2761,7 @@ void qmp_x_exit_preconfig(Error **errp)
>      if (incoming) {
>          Error *local_err = NULL;
>          if (strcmp(incoming, "defer") != 0) {
> -            qmp_migrate_incoming(incoming, false, NULL, true, true,
> +            qmp_migrate_incoming(NULL, true, incoming_channels, true, true,
>                                   &local_err);

You move the parsing of legacy URI from within qmp_migrate_incoming()
into incoming_option_parse().

The alternative is not to parse it in incoming_option_parse(), but pass
it to qmp_migrate_incoming() like this:

               qmp_migrate_incoming(incoming, !incoming, incoming_channels,
                                    true, true, &local_err);

>              if (local_err) {
>                  error_reportf_err(local_err, "-incoming %s: ", incoming);
> @@ -3477,7 +3508,7 @@ void qemu_init(int argc, char **argv)
>                  if (!incoming) {
>                      runstate_set(RUN_STATE_INMIGRATE);
>                  }
> -                incoming = optarg;
> +                incoming_option_parse(optarg);
>                  break;
>              case QEMU_OPTION_only_migratable:
>                  only_migratable = 1;