On Mon, Jul 17, 2017 at 03:25:05PM -0300, Eduardo Habkost wrote: > 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 <pet...@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(¶ms, &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.
Sure. > > (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. Hmm... > > 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(). ... How about I just provide a realize() for it? Then I can drop the QOM patch 4, also, I can keep the checks along with the object itself. Thanks, -- Peter Xu