Re: [PATCH RFC] migration: warn about non-migratable configurations unless '--no-migration' was specified

[Markus Armbruster <armbru@redhat.com>]

Vitaly Kuznetsov <vkuznets@redhat.com> writes:

> When a migration blocker is added nothing is reported to the user,
> inability to migrate such guest may come as a late surprise. As a bare
> minimum, we can print a warning. To not pollute the output for those, who
> have no intention to migrate their guests, introduce '--no-migration'
> option which both block the migration and eliminates warning from

Sure this justifies its own command line option?  Can we make it a
parameter of an existing option?  We have too many options, and options
starting "no-" are often awkward.

> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
>  include/qapi/qmp/qerror.h |  3 +++
>  include/sysemu/sysemu.h   |  1 +
>  migration/migration.c     | 18 +++++++++++++++++-
>  qemu-options.hx           |  7 +++++++
>  softmmu/globals.c         |  1 +
>  softmmu/vl.c              |  3 +++
>  6 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/include/qapi/qmp/qerror.h b/include/qapi/qmp/qerror.h
> index 596fce0c54e7..2e1563c72f83 100644
> --- a/include/qapi/qmp/qerror.h
> +++ b/include/qapi/qmp/qerror.h
> @@ -50,6 +50,9 @@
   /*
    * These macros will go away, please don't use in new code, and do not
    * add new ones!
    */
   ...
>  #define QERR_MISSING_PARAMETER \
>      "Parameter '%s' is missing"
>  
> +#define QERR_NO_MIGRATION \
> +    "Guest is not migratable ('--no-migration' used)"
> +
>  #define QERR_PERMISSION_DENIED \
>      "Insufficient permission to perform this operation"
>  

Please don't add new macros here.

Looks like you use QERR_NO_MIGRATION only in migration/migration.c.  You
can add a macro there.  Or simply duplicate the error string, which I'd
find easier to read.  Up to you.

> diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
> index 8fae667172ac..c65cd5d5a336 100644
> --- a/include/sysemu/sysemu.h
> +++ b/include/sysemu/sysemu.h
> @@ -9,6 +9,7 @@
>  /* vl.c */
>  
>  extern int only_migratable;
> +extern int no_migration;
>  extern const char *qemu_name;
>  extern QemuUUID qemu_uuid;
>  extern bool qemu_uuid_set;
> diff --git a/migration/migration.c b/migration/migration.c
> index ca8b97baa5ac..29a8480ced54 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -1077,7 +1077,9 @@ static void fill_source_migration_info(MigrationInfo *info)
>      info->blocked = migration_is_blocked(NULL);
>      info->has_blocked_reasons = info->blocked;
>      info->blocked_reasons = NULL;
> -    if (info->blocked) {
> +    if (no_migration) {
> +        QAPI_LIST_PREPEND(info->blocked_reasons, g_strdup(QERR_NO_MIGRATION));
> +    } else if (info->blocked) {
>          GSList *cur_blocker = migration_blockers;
>  
>          /*

Apropos blocked-reasons.  migration.json has:

    # @blocked: True if outgoing migration is blocked (since 6.0)
    #
    # @blocked-reasons: A list of reasons an outgoing migration is blocked (since 6.0)
    [...]
              'blocked': 'bool',
              '*blocked-reasons': ['str'],

Can "blocked-reasons" be absent or empty when "blocked" is true?

If not, then "blocked" is redundant, and should be dropped before we
release 6.0.

Else, the documentation should spell it out.  No need to rush that.

The patch was not cc'ed to me.  I might have caught it earlier...

[...]