Re: [PATCH v2 04/11] qapi/migration: Introduce x-vcpu-dirty-limit-period parameter

[Markus Armbruster]

huangy81@chinatelecom.cn writes:

> From: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
>
> Introduce "x-vcpu-dirty-limit-period" migration experimental
> parameter, which is in the range of 1 to 1000ms and used to
> make dirtyrate calculation period configurable.
>
> Signed-off-by: Hyman Huang(黄勇) <huangy81@chinatelecom.cn>
> ---
>  migration/migration.c | 26 ++++++++++++++++++++++++++
>  monitor/hmp-cmds.c    |  8 ++++++++
>  qapi/migration.json   | 34 +++++++++++++++++++++++++++-------
>  3 files changed, 61 insertions(+), 7 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index 739bb68..701267c 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -116,6 +116,8 @@
>  #define DEFAULT_MIGRATE_ANNOUNCE_ROUNDS    5
>  #define DEFAULT_MIGRATE_ANNOUNCE_STEP    100
>  
> +#define DEFAULT_MIGRATE_VCPU_DIRTY_LIMIT_PERIOD     500     /* ms */
> +
>  static NotifierList migration_state_notifiers =
>      NOTIFIER_LIST_INITIALIZER(migration_state_notifiers);
>  
> @@ -963,6 +965,9 @@ MigrationParameters *qmp_query_migrate_parameters(Error 
> **errp)
>                         s->parameters.block_bitmap_mapping);
>      }
>  
> +    params->has_x_vcpu_dirty_limit_period = true;
> +    params->x_vcpu_dirty_limit_period = 
> s->parameters.x_vcpu_dirty_limit_period;
> +
>      return params;
>  }
>  
> @@ -1564,6 +1569,15 @@ static bool migrate_params_check(MigrationParameters 
> *params, Error **errp)
>      }
>  #endif
>  
> +    if (params->has_x_vcpu_dirty_limit_period &&
> +        (params->x_vcpu_dirty_limit_period < 1 ||
> +         params->x_vcpu_dirty_limit_period > 1000)) {
> +        error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
> +                   "x_vcpu_dirty_limit_period",
> +                   "is invalid, it must be in the range of 1 to 1000 ms");

Two mistakes:

1. The parameter is called "x-vcpu-dirty-limit-period".

2. The error message is bad:

    (qemu) migrate_set_parameter x-vcpu-dirty-limit-period 100000
    Error: Parameter 'x_vcpu_dirty_limit_period' expects is invalid, it must be 
in the range of 1 to 1000 ms

   Use something like

           error_setg(errp, QERR_INVALID_PARAMETER_VALUE,
                      "x-vcpu-dirty-limit-period",
                      "a value between 1 and 1000");

Always, always, always test your error paths!

> +        return false;
> +    }
> +
>      return true;
>  }
>  

[...]