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(¶ms, &err)) {
+ error_report_err(err);
+ exit(1);
+ }
+}
+
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
Peter Xu <peterx@redhat.com> wrote: > Adding validity check for the migration parameters passed in via global > properties. > > Signed-off-by: Peter Xu <peterx@redhat.com> Reviewed-by: Juan Quintela <quintela@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(¶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.
(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
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 <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(¶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
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 <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(¶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.
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.
--
Eduardo
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 <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(¶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.
>
> 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
© 2016 - 2026 Red Hat, Inc.