From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:42270) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1dQIIE-0000dG-Jk for qemu-devel@nongnu.org; Wed, 28 Jun 2017 15:06:27 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1dQII9-0002Ta-L1 for qemu-devel@nongnu.org; Wed, 28 Jun 2017 15:06:26 -0400 Received: from mx1.redhat.com ([209.132.183.28]:44020) by eggs.gnu.org with esmtps (TLS1.0:DHE_RSA_AES_256_CBC_SHA1:32) (Exim 4.71) (envelope-from ) id 1dQII9-0002TB-BC for qemu-devel@nongnu.org; Wed, 28 Jun 2017 15:06:21 -0400 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.phx2.redhat.com [10.5.11.16]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mx1.redhat.com (Postfix) with ESMTPS id 4A4E58E766 for ; Wed, 28 Jun 2017 19:06:20 +0000 (UTC) Date: Wed, 28 Jun 2017 16:06:16 -0300 From: Eduardo Habkost Message-ID: <20170628190616.GN12152@localhost.localdomain> References: <1498536619-14548-1-git-send-email-peterx@redhat.com> <1498536619-14548-10-git-send-email-peterx@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <1498536619-14548-10-git-send-email-peterx@redhat.com> Subject: Re: [Qemu-devel] [PATCH v6 09/10] migration: merge enforce_config_section somewhat List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Xu Cc: qemu-devel@nongnu.org, Laurent Vivier , Eric Blake , Markus Armbruster , Juan Quintela , "Dr . David Alan Gilbert" 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 > Signed-off-by: Peter Xu > --- > 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