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

[Markus Armbruster <armbru@redhat.com>]

Markus Armbruster <armbru@redhat.com> writes:

> 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/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.

Here's a way to avoid restricting modern to JSON.

Legacy URI is either "defer" or starts with "KEYWORD:", where KEYWORD is
one of a few well-known words.

As long as we don't support an implied key, a non-empty dotted keys
argument starts with "KEY=", where KEY cannot contain ':'.

This lets us distinguish legacy URI from dotted keys.  Say, if the
argument is "defer" or starts with letters followed by ':', assume URI.

>   How can a management application detect that -incoming supports
>   modern?
>
>   Sure overloading -incoming this way is a good idea?

It'll be a pain to document.

> * 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;