Re: [PATCH v5 16/19] qapi: add interface for local TAP migration

[Fabiano Rosas <farosas@suse.de>]

Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru> writes:

Hi Vladimir, as usual with "qapi:" patches, some comments about
language:

> To migrate virtio-net TAP device backend (including open fds) locally,
> user should simply set migration parameter
>
>    fds = [virtio-net]
>
> Why not simple boolean? To simplify migration to further versions,
> when more devices will support fds migration.
>
> Alternatively, we may add per-device option to disable fds-migration,
> but still:
>
> 1. It's more comfortable to set same capabilities/parameters on both
> source and target QEMU, than care about each device.
>
> 2. To not break the design, that machine-type + device options +
> migration capabilites and parameters are fully define the resulting
> migration stream. We'll break this if add in future more fds-passing
> support in devices under same fds=true parameter.
>

These arguments look convincing to me.

> Signed-off-by: Vladimir Sementsov-Ogievskiy <vsementsov@yandex-team.ru>
> ---
>  include/qapi/util.h | 17 +++++++++++++++++
>  migration/options.c | 30 +++++++++++++++++++++++++++++
>  migration/options.h |  2 ++
>  qapi/migration.json | 46 ++++++++++++++++++++++++++++++++++++---------
>  4 files changed, 86 insertions(+), 9 deletions(-)
>
> diff --git a/include/qapi/util.h b/include/qapi/util.h
> index 29bc4eb865..b953402416 100644
> --- a/include/qapi/util.h
> +++ b/include/qapi/util.h
> @@ -69,4 +69,21 @@ int parse_qapi_name(const char *name, bool complete);
>          _len;                                                       \
>      })
>  
> +/*
> + * For any GenericList @list, return true if it contains specified
> + * element.
> + */
> +#define QAPI_LIST_CONTAINS(list, el)                                \
> +    ({                                                              \
> +        bool _found = false;                                        \
> +        typeof_strip_qual(list) _tail;                              \
> +        for (_tail = list; _tail != NULL; _tail = _tail->next) {    \
> +            if (_tail->value == el) {                               \
> +                _found = true;                                      \
> +                break;                                              \
> +            }                                                       \
> +        }                                                           \
> +        _found;                                                     \
> +    })
> +
>  #endif
> diff --git a/migration/options.c b/migration/options.c
> index 4e923a2e07..061a1b8eaf 100644
> --- a/migration/options.c
> +++ b/migration/options.c
> @@ -13,6 +13,7 @@
>  
>  #include "qemu/osdep.h"
>  #include "qemu/error-report.h"
> +#include "qapi/util.h"
>  #include "exec/target_page.h"
>  #include "qapi/clone-visitor.h"
>  #include "qapi/error.h"
> @@ -262,6 +263,13 @@ bool migrate_mapped_ram(void)
>      return s->capabilities[MIGRATION_CAPABILITY_MAPPED_RAM];
>  }
>  
> +bool migrate_fds_virtio_net(void)
> +{
> +    MigrationState *s = migrate_get_current();
> +
> +    return QAPI_LIST_CONTAINS(s->parameters.fds, FDS_TARGET_VIRTIO_NET);
> +}
> +
>  bool migrate_ignore_shared(void)
>  {
>      MigrationState *s = migrate_get_current();
> @@ -960,6 +968,11 @@ MigrationParameters *qmp_query_migrate_parameters(Error **errp)
>      params->has_direct_io = true;
>      params->direct_io = s->parameters.direct_io;
>  
> +    if (s->parameters.has_fds) {
> +        params->has_fds = true;
> +        params->fds = QAPI_CLONE(FdsTargetList, s->parameters.fds);
> +    }
> +
>      return params;
>  }
>  
> @@ -1179,6 +1192,11 @@ bool migrate_params_check(MigrationParameters *params, Error **errp)
>          return false;
>      }
>  
> +    if (params->has_fds) {
> +        error_setg(errp, "Not implemented");
> +        return false;
> +    }
> +
>      return true;
>  }
>  
> @@ -1297,6 +1315,11 @@ static void migrate_params_test_apply(MigrateSetParameters *params,
>      if (params->has_direct_io) {
>          dest->direct_io = params->direct_io;
>      }
> +
> +    if (params->has_fds) {
> +        dest->has_fds = true;

I think what you want is to set this in
migrate_params_init(). block_bitmap_mapping is a bit of an outlier in
that it takes an empty *input* as valid.

> +        dest->fds = params->fds;
> +    }
>  }
>  
>  static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
> @@ -1429,6 +1452,13 @@ static void migrate_params_apply(MigrateSetParameters *params, Error **errp)
>      if (params->has_direct_io) {
>          s->parameters.direct_io = params->direct_io;
>      }
> +
> +    if (params->has_fds) {
> +        qapi_free_FdsTargetList(s->parameters.fds);
> +
> +        s->parameters.has_fds = true;
> +        s->parameters.fds = QAPI_CLONE(FdsTargetList, params->fds);

Same here, has_fds should always be true in s->parameters.

> +    }
>  }
>  
>  void qmp_migrate_set_parameters(MigrateSetParameters *params, Error **errp)
> diff --git a/migration/options.h b/migration/options.h
> index 82d839709e..a79472a235 100644
> --- a/migration/options.h
> +++ b/migration/options.h
> @@ -87,6 +87,8 @@ const char *migrate_tls_hostname(void);
>  uint64_t migrate_xbzrle_cache_size(void);
>  ZeroPageDetection migrate_zero_page_detection(void);
>  
> +bool migrate_fds_virtio_net(void);
> +
>  /* parameters helpers */
>  
>  bool migrate_params_check(MigrationParameters *params, Error **errp);
> diff --git a/qapi/migration.json b/qapi/migration.json
> index 2387c21e9c..6ef9629c6d 100644
> --- a/qapi/migration.json
> +++ b/qapi/migration.json
> @@ -747,6 +747,19 @@
>        '*transform': 'BitmapMigrationBitmapAliasTransform'
>    } }
>  
> +##
> +# @FdsTarget:
> +#
> +# @virtio-net: Enable live backend migration for virtio-net.

So you're assuming normal migration is "non-live backend migration"
because the backend is stopped and started again on the destination. I
think it makes sense.

> +#     The only supported backend is TAP device. When enabled, TAP fds
> +#     and all related state is passed to target QEMU through migration
> +#     channel (which should be unix socket).
> +#
> +# Since: 10.2
> +##
> +{ 'enum': 'FdsTarget',
> +  'data': [ 'virtio-net' ] }
> +
>  ##
>  # @BitmapMigrationNodeAlias:
>  #
> @@ -924,10 +937,14 @@
>  #     only has effect if the @mapped-ram capability is enabled.
>  #     (Since 9.1)
>  #
> +# @fds: List of targets to enable live-backend migration for. This
> +#     requires migration channel to be a unix socket (to pass fds
> +#     through). (Since 10.2)

I think I prefer live-backend as written here rather than the non
hyphenated version above. This clears up the ambiguity and can be used
in variable names, as a name for the feature and ... _thinks really
hard_ ... as the parameter name! (maybe)

On that note, fds is just too broad, I'm sure you know that, but to be
specific, we already have fd migration, multifd, fdset (as argument of
file: migration), etc. Talking just about the user interface here, of
course.

Also: "@fds: List of targets..." is super confusing.

And although "to pass fds through" is helping clarify the meaning of
@fds, it exposes implementation details on the QAPI, I don't think it's
relevant in the parameter description.

> +#
>  # Features:
>  #
> -# @unstable: Members @x-checkpoint-delay and
> -#     @x-vcpu-dirty-limit-period are experimental.
> +# @unstable: Members @x-checkpoint-delay,
> +#     @x-vcpu-dirty-limit-period and @fds are experimental.
>  #
>  # Since: 2.4
>  ##
> @@ -950,7 +967,8 @@
>             'vcpu-dirty-limit',
>             'mode',
>             'zero-page-detection',
> -           'direct-io'] }
> +           'direct-io',
> +           'fds' ] }
>  
>  ##
>  # @MigrateSetParameters:
> @@ -1105,10 +1123,14 @@
>  #     only has effect if the @mapped-ram capability is enabled.
>  #     (Since 9.1)
>  #
> +# @fds: List of targets to enable live-backend migration for. This
> +#     requires migration channel to be a unix socket (to pass fds
> +#     through). (Since 10.2)
> +#
>  # Features:
>  #
> -# @unstable: Members @x-checkpoint-delay and
> -#     @x-vcpu-dirty-limit-period are experimental.
> +# @unstable: Members @x-checkpoint-delay,
> +#     @x-vcpu-dirty-limit-period and @fds are experimental.
>  #
>  # TODO: either fuse back into `MigrationParameters`, or make
>  #     `MigrationParameters` members mandatory
> @@ -1146,7 +1168,8 @@
>              '*vcpu-dirty-limit': 'uint64',
>              '*mode': 'MigMode',
>              '*zero-page-detection': 'ZeroPageDetection',
> -            '*direct-io': 'bool' } }
> +            '*direct-io': 'bool',
> +            '*fds': { 'type': [ 'FdsTarget' ], 'features': [ 'unstable' ] } } }
>  
>  ##
>  # @migrate-set-parameters:
> @@ -1315,10 +1338,14 @@
>  #     only has effect if the @mapped-ram capability is enabled.
>  #     (Since 9.1)
>  #
> +# @fds: List of targets to enable live-backend migration for. This
> +#     requires migration channel to be a unix socket (to pass fds
> +#     through). (Since 10.2)
> +#
>  # Features:
>  #
> -# @unstable: Members @x-checkpoint-delay and
> -#     @x-vcpu-dirty-limit-period are experimental.
> +# @unstable: Members @x-checkpoint-delay,
> +#     @x-vcpu-dirty-limit-period and @fds are experimental.
>  #
>  # Since: 2.4
>  ##
> @@ -1353,7 +1380,8 @@
>              '*vcpu-dirty-limit': 'uint64',
>              '*mode': 'MigMode',
>              '*zero-page-detection': 'ZeroPageDetection',
> -            '*direct-io': 'bool' } }
> +            '*direct-io': 'bool',
> +            '*fds': { 'type': [ 'FdsTarget' ], 'features': [ 'unstable' ] } } }
>  
>  ##
>  # @query-migrate-parameters: