Steve Sistare <steven.sist...@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.sist...@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;
