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 <quint...@redhat.com>
> Signed-off-by: Peter Xu <pet...@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
