Re: [Qemu-devel] [PATCH v2 07/11] migration: check global params for validity

[Eduardo Habkost <ehabkost@redhat.com>]

On Mon, Jul 17, 2017 at 04:26:07PM +0800, Peter Xu wrote:
> Adding validity check for the migration parameters passed in via global
> properties.
> 
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 34 ++++++++++++++++++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 8c65054..5a7f22c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2109,6 +2109,39 @@ static void migration_instance_init(Object *obj)
>      ms->parameters.tls_hostname = g_strdup("");
>  }
>  
> +static void migration_instance_post_init(Object *obj)
> +{
> +    MigrationState *ms = (MigrationState *)obj;
> +    Error *err = NULL;
> +    MigrationParameters params = {
> +        .has_compress_level = true,
> +        .compress_level = ms->parameters.compress_level,
> +        .has_compress_threads = true,
> +        .compress_threads = ms->parameters.compress_threads,
> +        .has_decompress_threads = true,
> +        .decompress_threads = ms->parameters.decompress_threads,
> +        .has_cpu_throttle_initial = true,
> +        .cpu_throttle_initial = ms->parameters.cpu_throttle_initial,
> +        .has_cpu_throttle_increment = true,
> +        .cpu_throttle_increment = ms->parameters.cpu_throttle_increment,
> +        .has_max_bandwidth = true,
> +        .max_bandwidth = ms->parameters.max_bandwidth,
> +        .has_downtime_limit = true,
> +        .downtime_limit = ms->parameters.downtime_limit,
> +        .has_x_checkpoint_delay = true,
> +        .x_checkpoint_delay = ms->parameters.x_checkpoint_delay,
> +        .has_block_incremental = true,
> +        .block_incremental = ms->parameters.block_incremental,
> +    };
> +
> +    /* We have applied all the migration properties... */
> +
> +    if (!migrate_params_check(&params, &err)) {

Why not just:
  if (!migrate_params_check(&ms->parameters, &err))
?

If the ms->parameters.has_* fields are not set to true anywhere,
we can set them to true in instance_init.

(This would also also us to use QAPI_CLONE in
qmp_query_migrate_parameters() instead of manually copying each
field.


> +        error_report_err(err);
> +        exit(1);
> +    }
> +}

On real devices, this is normally done on realize (which has
proper error reporting).  We never realize the TYPE_MIGRATION
object because it's not a real device, though.

Doing error checks on post_init feels fragile, as the only way
errors can be handled is triggering exit(1) in the middle of an
object_new() call.

As we have only one place where the TYPE_MIGRATION object is
created, I suggest calling migrate_params_check() inside
migration_object_init().

> +
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
>      /*
> @@ -2124,6 +2157,7 @@ static const TypeInfo migration_type = {
>      .class_size = sizeof(MigrationClass),
>      .instance_size = sizeof(MigrationState),
>      .instance_init = migration_instance_init,
> +    .instance_post_init = migration_instance_post_init,
>  };
>  
>  static void register_migration_types(void)
> -- 
> 2.7.4
> 

-- 
Eduardo