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

[Peter Xu]

On Mon, Jul 17, 2017 at 11:33:55PM -0300, Eduardo Habkost wrote:
> On Tue, Jul 18, 2017 at 09:56:06AM +0800, Peter Xu wrote:
> > 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 <address@hidden>
> > > > ---
> > > >  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.
> > 
> > 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.
> 
> qdev does lots of other stuff at realize time (e.g. it adds the
> device to the device tree), and I don't think we want to trigger
> that.

Ok. Then let me take your suggestion to move this to
migration_object_init().  Thanks,

-- 
Peter Xu