Re: [Qemu-devel] [PATCH 2/5] migration: Create block capability

[Markus Armbruster]

Juan Quintela <address@hidden> writes:

> Create one capability for block migration and one parameter for
> incremental block migration.
>
> Signed-off-by: Juan Quintela <address@hidden>
[...]
> diff --git a/include/migration/block.h b/include/migration/block.h
> index 41a1ac8..5225af9 100644
> --- a/include/migration/block.h
> +++ b/include/migration/block.h
> @@ -20,4 +20,6 @@ uint64_t blk_mig_bytes_transferred(void);
>  uint64_t blk_mig_bytes_remaining(void);
>  uint64_t blk_mig_bytes_total(void);
>  
> +void migrate_set_block_enabled(bool value, Error **errp);
> +
>  #endif /* MIGRATION_BLOCK_H */
> diff --git a/include/migration/migration.h b/include/migration/migration.h
> index 49ec501..024a048 100644
> --- a/include/migration/migration.h
> +++ b/include/migration/migration.h
> @@ -153,6 +153,9 @@ struct MigrationState
>  
>      /* The last error that occurred */
>      Error *error;
> +    /* Do we have to clean up -b/-i from old migrate parameters */

The sentence is a question, so it should end with a '?'.

> +    /* This feature is deprecated and will be removed */
> +    bool must_remove_block_options;
>  };
>  
>  void migrate_set_state(int *state, int old_state, int new_state);
> @@ -265,6 +268,9 @@ bool migrate_colo_enabled(void);
>  
>  int64_t xbzrle_cache_resize(int64_t new_size);
>  
> +bool migrate_use_block(void);
> +bool migrate_use_block_incremental(void);
> +
>  bool migrate_use_compression(void);
>  int migrate_compress_level(void);
>  int migrate_compress_threads(void);
> diff --git a/migration/migration.c b/migration/migration.c
> index 0304c01..c13c0a2 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
[...]
> @@ -1207,6 +1242,24 @@ void qmp_migrate(const char *uri, bool has_blk, bool 
> blk,
>          return;
>      }
>  
> +    if ((has_blk && blk) || (has_inc && inc)) {
> +        if (migrate_use_block() || migrate_use_block_incremental()) {
> +            error_setg(errp, "Command options are incompatible with "
> +                       "current migration capabilities");
> +            return;
> +        }
> +        migrate_set_block_enabled(true, &local_err);
> +        if (local_err) {
> +            error_propagate(errp, local_err);
> +            return;
> +        }
> +        s->must_remove_block_options = true;
> +    }
> +
> +    if (has_inc && inc) {
> +        migrate_set_block_incremental(s, true);
> +    }
> +

Putting this within the previous conditional might be clearer.  Your
choice.

>      s = migrate_init(&params);
>  
>      if (strstart(uri, "tcp:", &p)) {
[...]

Only nitpicks, so
Reviewed-by: Markus Armbruster <address@hidden>