[Qemu-devel] [PATCH 0/4] migration: fix iotest 055, only-migratable break

[Qemu-devel] [PATCH 0/4] migration: fix iotest 055, only-migratable break

[Peter Xu]

Two breakage introduced during the migration objectify series: one for
--only-migratable, another one for iotest 055.

First two patches fixes the breakages. Latter two are documentation
updates suggested by Eduardo. Please review. Thanks.

Peter Xu (4):
  migration: fix handling for --only-migratable
  vl: move global property, migrate init earlier
  doc: add item for "-M enforce-config-section"
  doc: update TYPE_MIGRATION documents

 include/migration/misc.h |  1 -
 migration/migration.c    | 20 +++++++++-----------
 qemu-options.hx          |  8 ++++++++
 vl.c                     | 26 +++++++++++++-------------
 4 files changed, 30 insertions(+), 25 deletions(-)

-- 
2.7.4

[Qemu-devel] [PATCH 1/4] migration: fix handling for --only-migratable

[Peter Xu]

MigrateState object is not ready at that time, so we'll get an
assertion. Use qemu_global_option() instead.

Reported-by: Eduardo Habkost <ehabkost@redhat.com>
Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Fixes: 3df663e ("migration: move only_migratable to MigrationState")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 include/migration/misc.h | 1 -
 migration/migration.c    | 5 -----
 vl.c                     | 2 +-
 3 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/include/migration/misc.h b/include/migration/misc.h
index 2255121..c079b77 100644
--- a/include/migration/misc.h
+++ b/include/migration/misc.h
@@ -53,7 +53,6 @@ bool migration_has_finished(MigrationState *);
 bool migration_has_failed(MigrationState *);
 /* ...and after the device transmission */
 bool migration_in_postcopy_after_devices(MigrationState *);
-void migration_only_migratable_set(void);
 void migration_global_dump(Monitor *mon);
 
 #endif
diff --git a/migration/migration.c b/migration/migration.c
index 51ccd1a..dbdc121 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -128,11 +128,6 @@ MigrationState *migrate_get_current(void)
     return current_migration;
 }
 
-void migration_only_migratable_set(void)
-{
-    migrate_get_current()->only_migratable = true;
-}
-
 MigrationIncomingState *migration_incoming_get_current(void)
 {
     static bool once;
diff --git a/vl.c b/vl.c
index 36ff3f4..0c497a3 100644
--- a/vl.c
+++ b/vl.c
@@ -3958,7 +3958,7 @@ int main(int argc, char **argv, char **envp)
                  *
                  * "-global migration.only-migratable=true"
                  */
-                migration_only_migratable_set();
+                qemu_global_option("migration.only-migratable=true");
                 break;
             case QEMU_OPTION_nodefaults:
                 has_defaults = 0;
-- 
2.7.4

[Qemu-devel] [PATCH 2/4] vl: move global property, migrate init earlier

[Peter Xu]

Currently drive_init_func() may call migrate_get_current() while the
migrate object is still not ready yet at that time. Move the migration
object init earlier, along with the global properties, right after
acceleration init.

This fixes a breakage for iotest 055, which caused an assertion failure.

Reported-by: Max Reitz <mreitz@redhat.com>
Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
Fixes: 3df663 ("migration: move only_migratable to MigrationState")
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 vl.c | 24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/vl.c b/vl.c
index 0c497a3..2ae4313 100644
--- a/vl.c
+++ b/vl.c
@@ -4414,6 +4414,18 @@ int main(int argc, char **argv, char **envp)
 
     configure_accelerator(current_machine);
 
+    /*
+     * Register all the global properties, including accel properties,
+     * machine properties, and user-specified ones.
+     */
+    register_global_properties(current_machine);
+
+    /*
+     * Migration object can only be created after global properties
+     * are applied correctly.
+     */
+    migration_object_init();
+
     if (qtest_chrdev) {
         qtest_init(qtest_chrdev, qtest_log, &error_fatal);
     }
@@ -4595,18 +4607,6 @@ int main(int argc, char **argv, char **envp)
             exit (i == 1 ? 1 : 0);
     }
 
-    /*
-     * Register all the global properties, including accel properties,
-     * machine properties, and user-specified ones.
-     */
-    register_global_properties(current_machine);
-
-    /*
-     * Migration object can only be created after global properties
-     * are applied correctly.
-     */
-    migration_object_init();
-
     /* This checkpoint is required by replay to separate prior clock
        reading from the other reads, because timer polling functions query
        clock values from the log. */
-- 
2.7.4

[Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Peter Xu]

It's never documented, and now we have one more parameter for it (which
means this one can be obsolete in the future). Document it properly.

Although now when enforce-config-section is set, it'll override the
other "-global" parameter, that is not necessarily a rule. Forbid that
usage in the document.

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 qemu-options.hx | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/qemu-options.hx b/qemu-options.hx
index 297bd8a..927c51f 100644
--- a/qemu-options.hx
+++ b/qemu-options.hx
@@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
 @item s390-squash-mcss=on|off
 Enables or disables squashing subchannels into the default css.
 The default is off.
+@item enforce-config-section=on|off
+Decides whether we will send the configuration section when doing
+migration. By default, it is turned on. We can set this to off to
+explicitly disable it. Note: this parameter will be obsolete soon,
+please use "-global migration.send-configuration=on|off" instead.
+"enforce-config-section" cannot be used together with "-global
+migration.send-configuration". If it happens, the behavior is
+undefined.
 @end table
 ETEXI
 
-- 
2.7.4

[Qemu-devel] [PATCH 4/4] doc: update TYPE_MIGRATION documents

[Peter Xu]

[Peter collected Eduardo's patch comment and formatted into patch]

Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 migration/migration.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/migration/migration.c b/migration/migration.c
index dbdc121..6c64b99 100644
--- a/migration/migration.c
+++ b/migration/migration.c
@@ -2052,12 +2052,15 @@ static void migration_instance_init(Object *obj)
 static const TypeInfo migration_type = {
     .name = TYPE_MIGRATION,
     /*
-     * NOTE: "migration" itself is not really a device. We used
-     * TYPE_DEVICE here only to leverage some existing QDev features
-     * like "-global" properties, and HW_COMPAT_* fields (which are
-     * finally applied as global properties as well). If one day the
-     * global property feature can be migrated from QDev to QObject in
-     * general, then we can switch to QObject as well.
+     * NOTE:
+     *
+     * TYPE_MIGRATION is not really a device, as the object is not
+     * created using qdev_create(), it is not attached to the qdev
+     * device tree, and it is never realized.
+     *
+     * If one day we allow non-device QOM objects to use the global
+     * property system, we can switch this from TYPE_DEVICE to
+     * TYPE_OBJECT.
      */
     .parent = TYPE_DEVICE,
     .class_init = migration_class_init,
-- 
2.7.4

Re: [Qemu-devel] [PATCH 0/4] migration: fix iotest 055, only-migratable break

[QingFeng Hao]

I tested the 2 patches and they can make iotest 055 passed on both x86 
and s390x.

thanks


在 2017/7/3 10:44, Peter Xu 写道:
> Two breakage introduced during the migration objectify series: one for
> --only-migratable, another one for iotest 055.
>
> First two patches fixes the breakages. Latter two are documentation
> updates suggested by Eduardo. Please review. Thanks.
>
> Peter Xu (4):
>    migration: fix handling for --only-migratable
>    vl: move global property, migrate init earlier
>    doc: add item for "-M enforce-config-section"
>    doc: update TYPE_MIGRATION documents
>
>   include/migration/misc.h |  1 -
>   migration/migration.c    | 20 +++++++++-----------
>   qemu-options.hx          |  8 ++++++++
>   vl.c                     | 26 +++++++++++++-------------
>   4 files changed, 30 insertions(+), 25 deletions(-)
>

-- 
Regards
QingFeng Hao

Re: [Qemu-devel] [PATCH 0/4] migration: fix iotest 055, only-migratable break

[Peter Xu]

On Mon, Jul 03, 2017 at 03:54:38PM +0800, QingFeng Hao wrote:
> I tested the 2 patches and they can make iotest 055 passed on both x86 and
> s390x.

Thanks for testing!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 1/4] migration: fix handling for --only-migratable

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> MigrateState object is not ready at that time, so we'll get an
> assertion. Use qemu_global_option() instead.
>
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Fixes: 3df663e ("migration: move only_migratable to MigrationState")
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Juan Quintela <quintela@redhat.com>

>  bool migration_in_postcopy_after_devices(MigrationState *);
> -void migration_only_migratable_set(void);
>  void migration_global_dump(Monitor *mon);

Nice, one less exported function.

> -                migration_only_migratable_set();
> +                qemu_global_option("migration.only-migratable=true");
>                  break;
>              case QEMU_OPTION_nodefaults:
>                  has_defaults = 0;

aha, that is a much, much nicer way to set functions.

Later, Juan.

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> It's never documented, and now we have one more parameter for it (which
> means this one can be obsolete in the future). Document it properly.
>
> Although now when enforce-config-section is set, it'll override the
> other "-global" parameter, that is not necessarily a rule. Forbid that
> usage in the document.
>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qemu-options.hx | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 297bd8a..927c51f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
>  @item s390-squash-mcss=on|off
>  Enables or disables squashing subchannels into the default css.
>  The default is off.
> +@item enforce-config-section=on|off
> +Decides whether we will send the configuration section when doing
> +migration. By default, it is turned on. We can set this to off to
> +explicitly disable it. Note: this parameter will be obsolete soon,
> +please use "-global migration.send-configuration=on|off" instead.
> +"enforce-config-section" cannot be used together with "-global
> +migration.send-configuration". If it happens, the behavior is
> +undefined.
>  @end table
>  ETEXI

Reviewed-by: Juan Quintela <quintela@redhat.com>

I will have been explicit that it is *already* obsolete, just that we
don't have a way to say that.

Later, Juan.

Re: [Qemu-devel] [PATCH 4/4] doc: update TYPE_MIGRATION documents

[Juan Quintela]

Peter Xu <peterx@redhat.com> wrote:
> [Peter collected Eduardo's patch comment and formatted into patch]
>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---

Reviewed-by: Juan Quintela <quintela@redhat.com>

Re: [Qemu-devel] [PATCH 2/4] vl: move global property, migrate init earlier

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:44:06AM +0800, Peter Xu wrote:
> Currently drive_init_func() may call migrate_get_current() while the
> migrate object is still not ready yet at that time. Move the migration
> object init earlier, along with the global properties, right after
> acceleration init.
> 
> This fixes a breakage for iotest 055, which caused an assertion failure.
> 
> Reported-by: Max Reitz <mreitz@redhat.com>
> Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Fixes: 3df663 ("migration: move only_migratable to MigrationState")
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  vl.c | 24 ++++++++++++------------
>  1 file changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/vl.c b/vl.c
> index 0c497a3..2ae4313 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4414,6 +4414,18 @@ int main(int argc, char **argv, char **envp)
>  
>      configure_accelerator(current_machine);
>  
> +    /*
> +     * Register all the global properties, including accel properties,
> +     * machine properties, and user-specified ones.
> +     */
> +    register_global_properties(current_machine);
> +
> +    /*
> +     * Migration object can only be created after global properties
> +     * are applied correctly.
> +     */
> +    migration_object_init();
> +

So, things that might introduce bugs here are:
1) Unexpected qdev_prop_register_global() calls between this place and
   the original register_global_properties() call (that would now happen
   in a different order).
2) register_global_properties() seeing a different global property list
   because it is being called earlier.
   2.1) AccelClass::global_props is statically defined and will be the
        same here. Not a problem.
   2.2) MachineClass::compat_props: same as above.
   2.3) user-provided global properties: we need to ensure all
        properties in the "global" QemuOpts section are already
        available at this point.


To ensure (1) is not a problem, we need to check all calls for
qdev_prop_register_global().  The callers are:

* configure_rtc()
  - Called very early, when parsing command-line options.  Not a problem.[1]
* global_init_func()
  * Called by user_register_global_props()
    * Called by register_global_properties().
      - That's the code we're moving.  Not a problem.
* QEMU_OPTION_rtc_td_hack case in main()
  - Called very early, when parsing command-line options.  Not a problem.[1]
* QEMU_OPTION_no_kvm_pit_reinjection case in main()
  - Called very early, when parsing command-line options.  Not a problem.[1]
* register_compat_prop()
  * Called by machine_register_compat_props()
    * Called by register_global_properties().
      - That's the code we're moving.  Not a problem.
  * Called by machine_register_compat_for_subclass()
    * Called by machine_register_compat_props() (see above)
  * Called by register_compat_props_array()
    * Called by accel_register_compat_props()
      * Called by register_global_properties().
        - That's the code we're moving.  Not a problem.
* qdev_prop_register_global_list()
  - Used only by unit test code.
* cpu_common_parse_features()
  - Used when initializing CPUs, which is done much later, when
    machine_run_board_init() is called (or when -device is handled by
    device_init_func()).  Not a problem.[2]
* x86_cpu_parse_featurestr()
  - Same as above.


To ensure (2.3) is not a problem, we need to look for references to
qemu_find_opts("global") or qemu_global_opts.  They are:

* user_register_global_props()
  - This is the code we're moving.  Not a problem.
* default_driver_check() call at main()
  - This happens earlier, before the code we're moving.  Not a problem.
* qemu_global_option()
  - Called very early, when parsing command-line options.  Not a problem.


So the code reordering looks OK.

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>


Notes about things we need to fix in the future:

[1] I think they should be replaced by qemu_global_option() calls, though.
[2] This is a fragile portion of the global property code and should be
    eventually moved to the command-line parsing section of main().


>      if (qtest_chrdev) {
>          qtest_init(qtest_chrdev, qtest_log, &error_fatal);
>      }
> @@ -4595,18 +4607,6 @@ int main(int argc, char **argv, char **envp)
>              exit (i == 1 ? 1 : 0);
>      }
>  
> -    /*
> -     * Register all the global properties, including accel properties,
> -     * machine properties, and user-specified ones.
> -     */
> -    register_global_properties(current_machine);
> -
> -    /*
> -     * Migration object can only be created after global properties
> -     * are applied correctly.
> -     */
> -    migration_object_init();
> -
>      /* This checkpoint is required by replay to separate prior clock
>         reading from the other reads, because timer polling functions query
>         clock values from the log. */
> -- 
> 2.7.4
> 

-- 
Eduardo

Re: [Qemu-devel] [PATCH 1/4] migration: fix handling for --only-migratable

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:44:05AM +0800, Peter Xu wrote:
> MigrateState object is not ready at that time, so we'll get an
> assertion. Use qemu_global_option() instead.
> 
> Reported-by: Eduardo Habkost <ehabkost@redhat.com>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Fixes: 3df663e ("migration: move only_migratable to MigrationState")
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 4/4] doc: update TYPE_MIGRATION documents

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:44:08AM +0800, Peter Xu wrote:
> [Peter collected Eduardo's patch comment and formatted into patch]
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>

Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Eduardo Habkost]

On Mon, Jul 03, 2017 at 10:44:07AM +0800, Peter Xu wrote:
> It's never documented, and now we have one more parameter for it (which
> means this one can be obsolete in the future). Document it properly.
> 
> Although now when enforce-config-section is set, it'll override the
> other "-global" parameter, that is not necessarily a rule. Forbid that
> usage in the document.
> 
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qemu-options.hx | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 297bd8a..927c51f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
>  @item s390-squash-mcss=on|off
>  Enables or disables squashing subchannels into the default css.
>  The default is off.
> +@item enforce-config-section=on|off
> +Decides whether we will send the configuration section when doing
> +migration. By default, it is turned on. We can set this to off to
> +explicitly disable it.
[...]

Wait, isn't it off by default?

This seems to imply that "-machine enforce-config-section=on" would have
no effect at all, as the option would be already on by default.  This is
not the case.

I suggest rewriting this as:

  If set to "on, force migration code to send configuration section even
  if the machine-type sets the "migration.send-configuration" property
  to "off".
  Note: this parameter is obsolete, please use "-global
  migration.send-configuration=on|off" instead.
  Behavior is undefined if "enforce-config-section" and "-global
  migration.send-configuration" are used together.

(Note: we probably should use proper markup (@option/@var/@samp?)
instead of quotes above, to format the option names properly in the
generated documentation.)

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] vl: move global property, migrate init earlier

[Peter Xu]

On Mon, Jul 03, 2017 at 11:59:03AM -0300, Eduardo Habkost wrote:
> On Mon, Jul 03, 2017 at 10:44:06AM +0800, Peter Xu wrote:
> > Currently drive_init_func() may call migrate_get_current() while the
> > migrate object is still not ready yet at that time. Move the migration
> > object init earlier, along with the global properties, right after
> > acceleration init.
> > 
> > This fixes a breakage for iotest 055, which caused an assertion failure.
> > 
> > Reported-by: Max Reitz <mreitz@redhat.com>
> > Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> > Fixes: 3df663 ("migration: move only_migratable to MigrationState")
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  vl.c | 24 ++++++++++++------------
> >  1 file changed, 12 insertions(+), 12 deletions(-)
> > 
> > diff --git a/vl.c b/vl.c
> > index 0c497a3..2ae4313 100644
> > --- a/vl.c
> > +++ b/vl.c
> > @@ -4414,6 +4414,18 @@ int main(int argc, char **argv, char **envp)
> >  
> >      configure_accelerator(current_machine);
> >  
> > +    /*
> > +     * Register all the global properties, including accel properties,
> > +     * machine properties, and user-specified ones.
> > +     */
> > +    register_global_properties(current_machine);
> > +
> > +    /*
> > +     * Migration object can only be created after global properties
> > +     * are applied correctly.
> > +     */
> > +    migration_object_init();
> > +
> 
> So, things that might introduce bugs here are:
> 1) Unexpected qdev_prop_register_global() calls between this place and
>    the original register_global_properties() call (that would now happen
>    in a different order).
> 2) register_global_properties() seeing a different global property list
>    because it is being called earlier.
>    2.1) AccelClass::global_props is statically defined and will be the
>         same here. Not a problem.
>    2.2) MachineClass::compat_props: same as above.
>    2.3) user-provided global properties: we need to ensure all
>         properties in the "global" QemuOpts section are already
>         available at this point.
> 
> 
> To ensure (1) is not a problem, we need to check all calls for
> qdev_prop_register_global().  The callers are:
> 
> * configure_rtc()
>   - Called very early, when parsing command-line options.  Not a problem.[1]
> * global_init_func()
>   * Called by user_register_global_props()
>     * Called by register_global_properties().
>       - That's the code we're moving.  Not a problem.
> * QEMU_OPTION_rtc_td_hack case in main()
>   - Called very early, when parsing command-line options.  Not a problem.[1]
> * QEMU_OPTION_no_kvm_pit_reinjection case in main()
>   - Called very early, when parsing command-line options.  Not a problem.[1]
> * register_compat_prop()
>   * Called by machine_register_compat_props()
>     * Called by register_global_properties().
>       - That's the code we're moving.  Not a problem.
>   * Called by machine_register_compat_for_subclass()
>     * Called by machine_register_compat_props() (see above)
>   * Called by register_compat_props_array()
>     * Called by accel_register_compat_props()
>       * Called by register_global_properties().
>         - That's the code we're moving.  Not a problem.
> * qdev_prop_register_global_list()
>   - Used only by unit test code.
> * cpu_common_parse_features()
>   - Used when initializing CPUs, which is done much later, when
>     machine_run_board_init() is called (or when -device is handled by
>     device_init_func()).  Not a problem.[2]
> * x86_cpu_parse_featurestr()
>   - Same as above.
> 
> 
> To ensure (2.3) is not a problem, we need to look for references to
> qemu_find_opts("global") or qemu_global_opts.  They are:
> 
> * user_register_global_props()
>   - This is the code we're moving.  Not a problem.
> * default_driver_check() call at main()
>   - This happens earlier, before the code we're moving.  Not a problem.
> * qemu_global_option()
>   - Called very early, when parsing command-line options.  Not a problem.
> 
> 
> So the code reordering looks OK.
> 
> Reviewed-by: Eduardo Habkost <ehabkost@redhat.com>

I really appreciate for such an in-depth review on this patch.

> 
> 
> Notes about things we need to fix in the future:
> 
> [1] I think they should be replaced by qemu_global_option() calls, though.
> [2] This is a fragile portion of the global property code and should be
>     eventually moved to the command-line parsing section of main().

Agree on both.

Thanks again!

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Peter Xu]

On Mon, Jul 03, 2017 at 02:07:12PM -0300, Eduardo Habkost wrote:
> On Mon, Jul 03, 2017 at 10:44:07AM +0800, Peter Xu wrote:
> > It's never documented, and now we have one more parameter for it (which
> > means this one can be obsolete in the future). Document it properly.
> > 
> > Although now when enforce-config-section is set, it'll override the
> > other "-global" parameter, that is not necessarily a rule. Forbid that
> > usage in the document.
> > 
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qemu-options.hx | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> > 
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 297bd8a..927c51f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
> >  @item s390-squash-mcss=on|off
> >  Enables or disables squashing subchannels into the default css.
> >  The default is off.
> > +@item enforce-config-section=on|off
> > +Decides whether we will send the configuration section when doing
> > +migration. By default, it is turned on. We can set this to off to
> > +explicitly disable it.
> [...]
> 
> Wait, isn't it off by default?
> 
> This seems to imply that "-machine enforce-config-section=on" would have
> no effect at all, as the option would be already on by default.  This is
> not the case.
> 
> I suggest rewriting this as:
> 
>   If set to "on, force migration code to send configuration section even
>   if the machine-type sets the "migration.send-configuration" property
>   to "off".
>   Note: this parameter is obsolete, please use "-global
>   migration.send-configuration=on|off" instead.
>   Behavior is undefined if "enforce-config-section" and "-global
>   migration.send-configuration" are used together.
> 
> (Note: we probably should use proper markup (@option/@var/@samp?)
> instead of quotes above, to format the option names properly in the
> generated documentation.)

Yes, you are right.  How's this one? (markup used this time)

If @option{enforce-config-section} is set to @var{on}, force migration
code to send configuration section even if the machine-type sets the
@option{migration.send-configuration} property to @var{off}.
@option{enforce-config-section} cannot be used together with
@option{-global} @option{migration.send-configuration}. Behavior is
undefined if @option{enforce-config-section} and @option{-global}
@option{migration.send-configuration} are used together.

NOTE: this parameter is obsolete. Please use @option{-global}
@option{migration.send-configuration}=@var{on|off} instead.

Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 4/4] doc: update TYPE_MIGRATION documents

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> [Peter collected Eduardo's patch comment and formatted into patch]
>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  migration/migration.c | 15 +++++++++------
>  1 file changed, 9 insertions(+), 6 deletions(-)
>
> diff --git a/migration/migration.c b/migration/migration.c
> index dbdc121..6c64b99 100644
> --- a/migration/migration.c
> +++ b/migration/migration.c
> @@ -2052,12 +2052,15 @@ static void migration_instance_init(Object *obj)
>  static const TypeInfo migration_type = {
>      .name = TYPE_MIGRATION,
>      /*
> -     * NOTE: "migration" itself is not really a device. We used
> -     * TYPE_DEVICE here only to leverage some existing QDev features
> -     * like "-global" properties, and HW_COMPAT_* fields (which are
> -     * finally applied as global properties as well). If one day the
> -     * global property feature can be migrated from QDev to QObject in
> -     * general, then we can switch to QObject as well.
> +     * NOTE:
> +     *
> +     * TYPE_MIGRATION is not really a device, as the object is not
> +     * created using qdev_create(), it is not attached to the qdev
> +     * device tree, and it is never realized.
> +     *
> +     * If one day we allow non-device QOM objects to use the global
> +     * property system, we can switch this from TYPE_DEVICE to
> +     * TYPE_OBJECT.

Suggest to turn this paragraph into a TODO:

        * TODO Make this TYPE_OBJECT once QOM provides something like
        * TYPE_DEVICE's "-global" properties.

>       */
>      .parent = TYPE_DEVICE,
>      .class_init = migration_class_init,

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Markus Armbruster]

Peter Xu <peterx@redhat.com> writes:

> It's never documented, and now we have one more parameter for it (which
> means this one can be obsolete in the future). Document it properly.
>
> Although now when enforce-config-section is set, it'll override the
> other "-global" parameter, that is not necessarily a rule. Forbid that
> usage in the document.
>
> Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>  qemu-options.hx | 8 ++++++++
>  1 file changed, 8 insertions(+)
>
> diff --git a/qemu-options.hx b/qemu-options.hx
> index 297bd8a..927c51f 100644
> --- a/qemu-options.hx
> +++ b/qemu-options.hx
> @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
>  @item s390-squash-mcss=on|off
>  Enables or disables squashing subchannels into the default css.
>  The default is off.
> +@item enforce-config-section=on|off
> +Decides whether we will send the configuration section when doing
> +migration. By default, it is turned on. We can set this to off to
> +explicitly disable it. Note: this parameter will be obsolete soon,
> +please use "-global migration.send-configuration=on|off" instead.

Please say "... is deprecated, please use ...", to make it visible in
"git-grep -i deprecat".

> +"enforce-config-section" cannot be used together with "-global
> +migration.send-configuration". If it happens, the behavior is
> +undefined.

Nasty.  Could we catch and reject such invalid usage?

>  @end table
>  ETEXI

Re: [Qemu-devel] [PATCH 2/4] vl: move global property, migrate init earlier

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Mon, Jul 03, 2017 at 10:44:06AM +0800, Peter Xu wrote:
>> Currently drive_init_func() may call migrate_get_current() while the
>> migrate object is still not ready yet at that time. Move the migration
>> object init earlier, along with the global properties, right after
>> acceleration init.
>> 
>> This fixes a breakage for iotest 055, which caused an assertion failure.
>> 
>> Reported-by: Max Reitz <mreitz@redhat.com>
>> Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> Fixes: 3df663 ("migration: move only_migratable to MigrationState")
>> Signed-off-by: Peter Xu <peterx@redhat.com>
>> ---
>>  vl.c | 24 ++++++++++++------------
>>  1 file changed, 12 insertions(+), 12 deletions(-)
>> 
>> diff --git a/vl.c b/vl.c
>> index 0c497a3..2ae4313 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -4414,6 +4414,18 @@ int main(int argc, char **argv, char **envp)
>>  
>>      configure_accelerator(current_machine);
>>  
>> +    /*
>> +     * Register all the global properties, including accel properties,
>> +     * machine properties, and user-specified ones.
>> +     */
>> +    register_global_properties(current_machine);
>> +
>> +    /*
>> +     * Migration object can only be created after global properties
>> +     * are applied correctly.
>> +     */
>> +    migration_object_init();
>> +
>
> So, things that might introduce bugs here are:
> 1) Unexpected qdev_prop_register_global() calls between this place and
>    the original register_global_properties() call (that would now happen
>    in a different order).
> 2) register_global_properties() seeing a different global property list
>    because it is being called earlier.
>    2.1) AccelClass::global_props is statically defined and will be the
>         same here. Not a problem.
>    2.2) MachineClass::compat_props: same as above.
>    2.3) user-provided global properties: we need to ensure all
>         properties in the "global" QemuOpts section are already
>         available at this point.

This stuff is fragile.  There are lots of dependencies, all implicit,
and once again we reorder things ever so slightly to keep them
satisfied.  We could certainly use a more robust solution.

[...]

Re: [Qemu-devel] [PATCH 4/4] doc: update TYPE_MIGRATION documents

[Peter Xu]

On Tue, Jul 04, 2017 at 10:04:01AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > [Peter collected Eduardo's patch comment and formatted into patch]
> >
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  migration/migration.c | 15 +++++++++------
> >  1 file changed, 9 insertions(+), 6 deletions(-)
> >
> > diff --git a/migration/migration.c b/migration/migration.c
> > index dbdc121..6c64b99 100644
> > --- a/migration/migration.c
> > +++ b/migration/migration.c
> > @@ -2052,12 +2052,15 @@ static void migration_instance_init(Object *obj)
> >  static const TypeInfo migration_type = {
> >      .name = TYPE_MIGRATION,
> >      /*
> > -     * NOTE: "migration" itself is not really a device. We used
> > -     * TYPE_DEVICE here only to leverage some existing QDev features
> > -     * like "-global" properties, and HW_COMPAT_* fields (which are
> > -     * finally applied as global properties as well). If one day the
> > -     * global property feature can be migrated from QDev to QObject in
> > -     * general, then we can switch to QObject as well.
> > +     * NOTE:
> > +     *
> > +     * TYPE_MIGRATION is not really a device, as the object is not
> > +     * created using qdev_create(), it is not attached to the qdev
> > +     * device tree, and it is never realized.
> > +     *
> > +     * If one day we allow non-device QOM objects to use the global
> > +     * property system, we can switch this from TYPE_DEVICE to
> > +     * TYPE_OBJECT.
> 
> Suggest to turn this paragraph into a TODO:
> 
>         * TODO Make this TYPE_OBJECT once QOM provides something like
>         * TYPE_DEVICE's "-global" properties.

Sure. Will modify in next version. Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Peter Xu]

On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > It's never documented, and now we have one more parameter for it (which
> > means this one can be obsolete in the future). Document it properly.
> >
> > Although now when enforce-config-section is set, it'll override the
> > other "-global" parameter, that is not necessarily a rule. Forbid that
> > usage in the document.
> >
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qemu-options.hx | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 297bd8a..927c51f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
> >  @item s390-squash-mcss=on|off
> >  Enables or disables squashing subchannels into the default css.
> >  The default is off.
> > +@item enforce-config-section=on|off
> > +Decides whether we will send the configuration section when doing
> > +migration. By default, it is turned on. We can set this to off to
> > +explicitly disable it. Note: this parameter will be obsolete soon,
> > +please use "-global migration.send-configuration=on|off" instead.
> 
> Please say "... is deprecated, please use ...", to make it visible in
> "git-grep -i deprecat".

Both "obsolete" and "deprecated" are used in QEMU for such an usage,
but I see indeed "deprecated" is used more often at least in
qemu-options.hx (used six times) than "obsolete" (one time only). Let
me switch then. :)

> 
> > +"enforce-config-section" cannot be used together with "-global
> > +migration.send-configuration". If it happens, the behavior is
> > +undefined.
> 
> Nasty.  Could we catch and reject such invalid usage?

Agree. However, looks like we cannot capture it easily on either of
the parameters (or, can we?). Since we defined it as "undefined", it
should be okay then, right?

After all, iiuc enforce-config-section is already very rarely used
(only on some special compat machines), and it'll be even more rare to
use the new one without removing the old.  Thanks,

-- 
Peter Xu

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Eduardo Habkost]

(CCing Greg, the original author of the code that added the
enforce-config-section option)

On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote:
> Peter Xu <peterx@redhat.com> writes:
> 
> > It's never documented, and now we have one more parameter for it (which
> > means this one can be obsolete in the future). Document it properly.
> >
> > Although now when enforce-config-section is set, it'll override the
> > other "-global" parameter, that is not necessarily a rule. Forbid that
> > usage in the document.
> >
> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >  qemu-options.hx | 8 ++++++++
> >  1 file changed, 8 insertions(+)
> >
> > diff --git a/qemu-options.hx b/qemu-options.hx
> > index 297bd8a..927c51f 100644
> > --- a/qemu-options.hx
> > +++ b/qemu-options.hx
> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
> >  @item s390-squash-mcss=on|off
> >  Enables or disables squashing subchannels into the default css.
> >  The default is off.
> > +@item enforce-config-section=on|off
> > +Decides whether we will send the configuration section when doing
> > +migration. By default, it is turned on. We can set this to off to
> > +explicitly disable it. Note: this parameter will be obsolete soon,
> > +please use "-global migration.send-configuration=on|off" instead.
> 
> Please say "... is deprecated, please use ...", to make it visible in
> "git-grep -i deprecat".
> 
> > +"enforce-config-section" cannot be used together with "-global
> > +migration.send-configuration". If it happens, the behavior is
> > +undefined.
> 
> Nasty.  Could we catch and reject such invalid usage?

Actually, the machine option will override
migration.send-configuration=off, and I don't believe we will break that
rule.  Documenting that behavior (but warning that it is deprecated)
sounds easier than adding extra code to detect the conflicting options.

We can simply replace "is undefined"  with "enforce-config-section will
override migration.send-configuration, but enforce-config-section is
deprecated".

I don't think anybody is relying on that option. If nobody is using it,
we can remove its code soon if we make it trigger a warning.
Machine-type compatibility code is already using
migration.send-configuration instead.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 2/4] vl: move global property, migrate init earlier

[Eduardo Habkost]

On Tue, Jul 04, 2017 at 10:12:45AM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Mon, Jul 03, 2017 at 10:44:06AM +0800, Peter Xu wrote:
> >> Currently drive_init_func() may call migrate_get_current() while the
> >> migrate object is still not ready yet at that time. Move the migration
> >> object init earlier, along with the global properties, right after
> >> acceleration init.
> >> 
> >> This fixes a breakage for iotest 055, which caused an assertion failure.
> >> 
> >> Reported-by: Max Reitz <mreitz@redhat.com>
> >> Reported-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> >> Fixes: 3df663 ("migration: move only_migratable to MigrationState")
> >> Signed-off-by: Peter Xu <peterx@redhat.com>
> >> ---
> >>  vl.c | 24 ++++++++++++------------
> >>  1 file changed, 12 insertions(+), 12 deletions(-)
> >> 
> >> diff --git a/vl.c b/vl.c
> >> index 0c497a3..2ae4313 100644
> >> --- a/vl.c
> >> +++ b/vl.c
> >> @@ -4414,6 +4414,18 @@ int main(int argc, char **argv, char **envp)
> >>  
> >>      configure_accelerator(current_machine);
> >>  
> >> +    /*
> >> +     * Register all the global properties, including accel properties,
> >> +     * machine properties, and user-specified ones.
> >> +     */
> >> +    register_global_properties(current_machine);
> >> +
> >> +    /*
> >> +     * Migration object can only be created after global properties
> >> +     * are applied correctly.
> >> +     */
> >> +    migration_object_init();
> >> +
> >
> > So, things that might introduce bugs here are:
> > 1) Unexpected qdev_prop_register_global() calls between this place and
> >    the original register_global_properties() call (that would now happen
> >    in a different order).
> > 2) register_global_properties() seeing a different global property list
> >    because it is being called earlier.
> >    2.1) AccelClass::global_props is statically defined and will be the
> >         same here. Not a problem.
> >    2.2) MachineClass::compat_props: same as above.
> >    2.3) user-provided global properties: we need to ensure all
> >         properties in the "global" QemuOpts section are already
> >         available at this point.
> 
> This stuff is fragile.  There are lots of dependencies, all implicit,
> and once again we reorder things ever so slightly to keep them
> satisfied.  We could certainly use a more robust solution.

Yes, and we are working on it.  Items 2.1-2.3 are already grouped in
register_global_properties() to make ordering rules explicit.  The rest
of the qdev_prop_register_global() calls need to be either replaced with
something else or moved to register_global_properties() too.

-- 
Eduardo

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Markus Armbruster]

Eduardo Habkost <ehabkost@redhat.com> writes:

> (CCing Greg, the original author of the code that added the
> enforce-config-section option)
>
> On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote:
>> Peter Xu <peterx@redhat.com> writes:
>> 
>> > It's never documented, and now we have one more parameter for it (which
>> > means this one can be obsolete in the future). Document it properly.
>> >
>> > Although now when enforce-config-section is set, it'll override the
>> > other "-global" parameter, that is not necessarily a rule. Forbid that
>> > usage in the document.
>> >
>> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
>> > Signed-off-by: Peter Xu <peterx@redhat.com>
>> > ---
>> >  qemu-options.hx | 8 ++++++++
>> >  1 file changed, 8 insertions(+)
>> >
>> > diff --git a/qemu-options.hx b/qemu-options.hx
>> > index 297bd8a..927c51f 100644
>> > --- a/qemu-options.hx
>> > +++ b/qemu-options.hx
>> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
>> >  @item s390-squash-mcss=on|off
>> >  Enables or disables squashing subchannels into the default css.
>> >  The default is off.
>> > +@item enforce-config-section=on|off
>> > +Decides whether we will send the configuration section when doing
>> > +migration. By default, it is turned on. We can set this to off to
>> > +explicitly disable it. Note: this parameter will be obsolete soon,
>> > +please use "-global migration.send-configuration=on|off" instead.
>> 
>> Please say "... is deprecated, please use ...", to make it visible in
>> "git-grep -i deprecat".
>> 
>> > +"enforce-config-section" cannot be used together with "-global
>> > +migration.send-configuration". If it happens, the behavior is
>> > +undefined.
>> 
>> Nasty.  Could we catch and reject such invalid usage?
>
> Actually, the machine option will override
> migration.send-configuration=off, and I don't believe we will break that
> rule.  Documenting that behavior (but warning that it is deprecated)
> sounds easier than adding extra code to detect the conflicting options.
>
> We can simply replace "is undefined"  with "enforce-config-section will
> override migration.send-configuration, but enforce-config-section is
> deprecated".

Better than the scary "behavior is undefined".  But do we have to say
anything at all?  If somebody gives both options with different values,
the conflicting settings fight it out.  In a sane command line, the last
one wins.  Ours isn't sane.  Do we really have to specify who wins?

> I don't think anybody is relying on that option. If nobody is using it,
> we can remove its code soon if we make it trigger a warning.
> Machine-type compatibility code is already using
> migration.send-configuration instead.

What about

      @item enforce-config-section=on|off
      Controls sending of the configuration section when doing migration
      (default on).  Note: this parameter is deprecated, please use
      "-global migration.send-configuration=on|off" instead.

Re: [Qemu-devel] [PATCH 3/4] doc: add item for "-M enforce-config-section"

[Peter Xu]

On Wed, Jul 05, 2017 at 05:31:22PM +0200, Markus Armbruster wrote:
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > (CCing Greg, the original author of the code that added the
> > enforce-config-section option)
> >
> > On Tue, Jul 04, 2017 at 10:06:54AM +0200, Markus Armbruster wrote:
> >> Peter Xu <peterx@redhat.com> writes:
> >> 
> >> > It's never documented, and now we have one more parameter for it (which
> >> > means this one can be obsolete in the future). Document it properly.
> >> >
> >> > Although now when enforce-config-section is set, it'll override the
> >> > other "-global" parameter, that is not necessarily a rule. Forbid that
> >> > usage in the document.
> >> >
> >> > Suggested-by: Eduardo Habkost <ehabkost@redhat.com>
> >> > Signed-off-by: Peter Xu <peterx@redhat.com>
> >> > ---
> >> >  qemu-options.hx | 8 ++++++++
> >> >  1 file changed, 8 insertions(+)
> >> >
> >> > diff --git a/qemu-options.hx b/qemu-options.hx
> >> > index 297bd8a..927c51f 100644
> >> > --- a/qemu-options.hx
> >> > +++ b/qemu-options.hx
> >> > @@ -85,6 +85,14 @@ Enables or disables NVDIMM support. The default is off.
> >> >  @item s390-squash-mcss=on|off
> >> >  Enables or disables squashing subchannels into the default css.
> >> >  The default is off.
> >> > +@item enforce-config-section=on|off
> >> > +Decides whether we will send the configuration section when doing
> >> > +migration. By default, it is turned on. We can set this to off to
> >> > +explicitly disable it. Note: this parameter will be obsolete soon,
> >> > +please use "-global migration.send-configuration=on|off" instead.
> >> 
> >> Please say "... is deprecated, please use ...", to make it visible in
> >> "git-grep -i deprecat".
> >> 
> >> > +"enforce-config-section" cannot be used together with "-global
> >> > +migration.send-configuration". If it happens, the behavior is
> >> > +undefined.
> >> 
> >> Nasty.  Could we catch and reject such invalid usage?
> >
> > Actually, the machine option will override
> > migration.send-configuration=off, and I don't believe we will break that
> > rule.  Documenting that behavior (but warning that it is deprecated)
> > sounds easier than adding extra code to detect the conflicting options.
> >
> > We can simply replace "is undefined"  with "enforce-config-section will
> > override migration.send-configuration, but enforce-config-section is
> > deprecated".
> 
> Better than the scary "behavior is undefined".  But do we have to say
> anything at all?  If somebody gives both options with different values,
> the conflicting settings fight it out.  In a sane command line, the last
> one wins.  Ours isn't sane.  Do we really have to specify who wins?
> 
> > I don't think anybody is relying on that option. If nobody is using it,
> > we can remove its code soon if we make it trigger a warning.
> > Machine-type compatibility code is already using
> > migration.send-configuration instead.
> 
> What about
> 
>       @item enforce-config-section=on|off
>       Controls sending of the configuration section when doing migration
>       (default on).

Sorry to be misleading in current patch. Its default should be "off"
but not "on" (as corrected by Eduardo, the point is we are talking
about "enforce-config-section", not "migration.send-configuration").
Actually it won't make much sense if someone specify "off" here.

> Note: this parameter is deprecated, please use
>       "-global migration.send-configuration=on|off" instead.

This is a suggestion I would like to take. I'll remove the whole
"undefined" sentence and update my post. Thanks,

-- 
Peter Xu