Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat

[Eduardo Habkost <ehabkost@redhat.com>]

On Tue, Jun 27, 2017 at 12:10:18PM +0800, Peter Xu wrote:
> These two parameters:
> 
> - MachineState::enforce_config_section
> - MigrationState::send_configuration
> 
> are playing similar role here. This patch merges the first one into
> second, then we'll have a single place to reference whether we need to
> send the configuration section.
> 
> I didn't remove the MachineState.enforce_config_section field since when
> applying that machine property (in machine_set_property()) we haven't
> yet initialized global properties and migration object. Then, it's
> still not easy to pass that boolean to MigrationState at such an early
> time.
> 
> A natural benefit for current patch is that now we kept the meaning of
> "enforce-config-section" since it'll still have the highest
> priority (that's what "enforce" mean I guess).
> 
> Reviewed-by: Juan Quintela <quintela@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 12 ++++++++++++
>  migration/savevm.c    | 12 ++----------
>  2 files changed, 14 insertions(+), 10 deletions(-)
> 
> diff --git a/migration/migration.c b/migration/migration.c
> index 96c6412..e7e6cf3 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -42,6 +42,7 @@
>  #include "exec/target_page.h"
>  #include "io/channel-buffer.h"
>  #include "migration/colo.h"
> +#include "hw/boards.h"
>  
>  #define MAX_THROTTLE  (32 << 20)      /* Migration transfer speed throttling */
>  
> @@ -102,9 +103,20 @@ static MigrationState *current_migration;
>  
>  void migration_object_init(void)
>  {
> +    MachineState *ms = MACHINE(qdev_get_machine());
> +
>      /* This can only be called once. */
>      assert(!current_migration);
>      current_migration = MIGRATION_OBJ(object_new(TYPE_MIGRATION));
> +
> +    /*
> +     * We cannot really do this in migration_instance_init() since at
> +     * that time global properties are not yet applied, then this
> +     * value will be definitely replaced by something else.
> +     */
> +    if (ms->enforce_config_section) {
> +        current_migration->send_configuration = true;
> +    }

So, this is a case where a user-provided config option (-machine
enforce-config-section) should trigger a different default in another
class (migration.send-configuration).

Also, the new default triggered by -machine has a very specific
priority:

* AccelClass::global_props must not override "-machine enforce-config-section=on"
* MachineClass::compat_props must not override
  "-machine enforce-config-section=on"

We must also decide in advance what should be result of:
* "-machine enforce-config-section=on -global migration.send-configuration=off"
* "-machine enforce-config-section=off -global migration.send-configuration=on"
* "-global migration.send-configuration=off -machine enforce-config-section=off"
* "-global migration.send-configuration=on -machine enforce-config-section=on"

I'm not sure what we should decide about these 4 cases above, but I
believe it would be safer to encode that decision at the same place we
handle the priority between accel/machine/user globals:
register_global_properties() at vl.c.


Or maybe this extra complexity is a sign that we shouldn't try to add
extra magic to make -machine affect the "migration" object properties,
and keep the existing machine->enforce_config_section check in the
migration code?  I'm not sure.

>  }
>  
>  /* For outgoing */
> diff --git a/migration/savevm.c b/migration/savevm.c
> index 7b39fb9..be3f885 100644
> --- a/migration/savevm.c
> +++ b/migration/savevm.c
> @@ -943,20 +943,13 @@ bool qemu_savevm_state_blocked(Error **errp)
>      return false;
>  }
>  
> -static bool enforce_config_section(void)
> -{
> -    MachineState *machine = MACHINE(qdev_get_machine());
> -    return machine->enforce_config_section;
> -}
> -
>  void qemu_savevm_state_header(QEMUFile *f)
>  {
>      trace_savevm_state_header();
>      qemu_put_be32(f, QEMU_VM_FILE_MAGIC);
>      qemu_put_be32(f, QEMU_VM_FILE_VERSION);
>  
> -    if (migrate_get_current()->send_configuration ||
> -        enforce_config_section()) {
> +    if (migrate_get_current()->send_configuration) {
>          qemu_put_byte(f, QEMU_VM_CONFIGURATION);
>          vmstate_save_state(f, &vmstate_configuration, &savevm_state, 0);
>      }
> @@ -1980,8 +1973,7 @@ int qemu_loadvm_state(QEMUFile *f)
>          return -ENOTSUP;
>      }
>  
> -    if (migrate_get_current()->send_configuration ||
> -        enforce_config_section()) {
> +    if (migrate_get_current()->send_configuration) {
>          if (qemu_get_byte(f) != QEMU_VM_CONFIGURATION) {
>              error_report("Configuration section missing");
>              return -EINVAL;
> -- 
> 2.7.4
> 

-- 
Eduardo