Recent commit cda4aa9a5a0 moved block backend creation before machine
property evaluation.  This broke qemu-iotests 055.  Turns out we need
to create the migration object before block backends, so block
backends can add migration blockers.  Fix by calling
migration_object_init() earlier, right before configure_blockdev().

Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
Reported-by: Kevin Wolf <kwolf@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
 vl.c | 15 ++++++++-------
 1 file changed, 8 insertions(+), 7 deletions(-)

diff --git a/vl.c b/vl.c
index 027b853d92..b3e48eff27 100644
--- a/vl.c
+++ b/vl.c
@@ -4276,10 +4276,17 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    /*
+     * Migration object can only be created after global properties
+     * are applied correctly.
+     */
+    migration_object_init();
+
     /*
      * Note: we need to create block backends before
      * machine_set_property(), so machine properties can refer to
-     * them.
+     * them, and after migration_object_init(), so we can create
+     * migration blockers.
      */
     configure_blockdev(&bdo_queue, machine_class, snapshot);
 
@@ -4297,12 +4304,6 @@ int main(int argc, char **argv, char **envp)
                      machine_class->name, machine_class->deprecation_reason);
     }
 
-    /*
-     * 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);
     }
-- 
2.17.2

Am 13.03.2019 um 09:43 hat Markus Armbruster geschrieben:
> Recent commit cda4aa9a5a0 moved block backend creation before machine
> property evaluation.  This broke qemu-iotests 055.  Turns out we need
> to create the migration object before block backends, so block
> backends can add migration blockers.  Fix by calling
> migration_object_init() earlier, right before configure_blockdev().
> 
> Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Thanks, applied to the block branch.

Kevin

On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote:
> Recent commit cda4aa9a5a0 moved block backend creation before machine
> property evaluation.  This broke qemu-iotests 055.  Turns out we need
> to create the migration object before block backends, so block
> backends can add migration blockers.  Fix by calling
> migration_object_init() earlier, right before configure_blockdev().
> 
> Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
> Reported-by: Kevin Wolf <kwolf@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>

Hi,

After this patch is applied, migration with Xen doesn't work anymore.
When QEMU on the receiving end is started, it prints:
    qemu-system-i386: load of migration failed: Invalid argument

> +    /*
> +     * Migration object can only be created after global properties
> +     * are applied correctly.
> +     */
> +    migration_object_init();

I think it's because migration_object_init() is now called before
configure_accelerator(). xen_accel_class_init() have some compat_props
for migration.

Any idea how to fix this?

Thanks,

-- 
Anthony PERARD

On Tue, Mar 26, 2019 at 03:58:52PM +0000, Anthony PERARD wrote:
> On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote:
> > Recent commit cda4aa9a5a0 moved block backend creation before machine
> > property evaluation.  This broke qemu-iotests 055.  Turns out we need
> > to create the migration object before block backends, so block
> > backends can add migration blockers.  Fix by calling
> > migration_object_init() earlier, right before configure_blockdev().
> > 
> > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
> > Reported-by: Kevin Wolf <kwolf@redhat.com>
> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
> 
> Hi,
> 
> After this patch is applied, migration with Xen doesn't work anymore.
> When QEMU on the receiving end is started, it prints:
>     qemu-system-i386: load of migration failed: Invalid argument

I should have quote this from QEMU stderr instead of the single line
abrove:
    qemu-system-i386: Configuration section missing
    qemu-system-i386: load of migration failed: Invalid argument

> > +    /*
> > +     * Migration object can only be created after global properties
> > +     * are applied correctly.
> > +     */
> > +    migration_object_init();
> 
> I think it's because migration_object_init() is now called before
> configure_accelerator(). xen_accel_class_init() have some compat_props
> for migration.
> 
> Any idea how to fix this?

-- 
Anthony PERARD

Anthony PERARD <anthony.perard@citrix.com> writes:

> On Tue, Mar 26, 2019 at 03:58:52PM +0000, Anthony PERARD wrote:
>> On Wed, Mar 13, 2019 at 09:43:30AM +0100, Markus Armbruster wrote:
>> > Recent commit cda4aa9a5a0 moved block backend creation before machine
>> > property evaluation.  This broke qemu-iotests 055.  Turns out we need
>> > to create the migration object before block backends, so block
>> > backends can add migration blockers.  Fix by calling
>> > migration_object_init() earlier, right before configure_blockdev().
>> > 
>> > Fixes: cda4aa9a5a08777cf13e164c0543bd4888b8adce
>> > Reported-by: Kevin Wolf <kwolf@redhat.com>
>> > Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> 
>> Hi,
>> 
>> After this patch is applied, migration with Xen doesn't work anymore.
>> When QEMU on the receiving end is started, it prints:
>>     qemu-system-i386: load of migration failed: Invalid argument
>
> I should have quote this from QEMU stderr instead of the single line
> abrove:
>     qemu-system-i386: Configuration section missing
>     qemu-system-i386: load of migration failed: Invalid argument
>
>> > +    /*
>> > +     * Migration object can only be created after global properties
>> > +     * are applied correctly.
>> > +     */
>> > +    migration_object_init();
>> 
>> I think it's because migration_object_init() is now called before
>> configure_accelerator(). xen_accel_class_init() have some compat_props
>> for migration.

I see.

Anything that gets created before a certain compat property becomes
known silently ignores the compat property[*].

Accelerator compat properties become known when we select the
accelerator.  This is awfully late, because we want to pick the first
accelerator that initializes, which requires a machine.

>> Any idea how to fix this?

Known dependencies:

* migration object_init() must run before configure_blockdev(), so block
  backends can register migration blockers

* configure_blockdev() must run before machine_set_property(), so
  machine properties can refer to block backends.

* configure_accelerator() must run after machine creation, because it
  needs a machine

* configure_accelerator() must run before migration_object_init(), so
  migration_object_init() can see accelerator compat properties.

We're currently violating the last one, because weren't aware of it.  To
fix that, rejigger some more: move configure_accelerator() before
migration_object_init() (with a suitable comment), then see what else
breaks.  Could you try the appended patch and report whether it fixes
your problem?


[*] Silent ignore is nasty.  I got experimental code to turn it into a
hard failure.


diff --git a/vl.c b/vl.c
index d61d5604e5..28b9e9a170 100644
--- a/vl.c
+++ b/vl.c
@@ -4276,6 +4276,12 @@ int main(int argc, char **argv, char **envp)
         exit(0);
     }
 
+    /*
+     * Must run before migration_object_init() to make Xen's
+     * accelerator compat properties stick
+     */
+    configure_accelerator(current_machine, argv[0]);
+
     /*
      * Migration object can only be created after global properties
      * are applied correctly.
@@ -4297,8 +4303,6 @@ int main(int argc, char **argv, char **envp)
     current_machine->maxram_size = maxram_size;
     current_machine->ram_slots = ram_slots;
 
-    configure_accelerator(current_machine, argv[0]);
-
     if (!qtest_enabled() && machine_class->deprecation_reason) {
         error_report("Machine type '%s' is deprecated: %s",
                      machine_class->name, machine_class->deprecation_reason);

On Tue, Mar 26, 2019 at 06:48:54PM +0100, Markus Armbruster wrote:
> Known dependencies:
> 
> * migration object_init() must run before configure_blockdev(), so block
>   backends can register migration blockers
> 
> * configure_blockdev() must run before machine_set_property(), so
>   machine properties can refer to block backends.
> 
> * configure_accelerator() must run after machine creation, because it
>   needs a machine
> 
> * configure_accelerator() must run before migration_object_init(), so
>   migration_object_init() can see accelerator compat properties.
> 
> We're currently violating the last one, because weren't aware of it.  To
> fix that, rejigger some more: move configure_accelerator() before
> migration_object_init() (with a suitable comment), then see what else
> breaks.  Could you try the appended patch and report whether it fixes
> your problem?

Yes, that patch works and doesn't break anything else with Xen.


> diff --git a/vl.c b/vl.c
> index d61d5604e5..28b9e9a170 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -4276,6 +4276,12 @@ int main(int argc, char **argv, char **envp)
>          exit(0);
>      }
>  
> +    /*
> +     * Must run before migration_object_init() to make Xen's
> +     * accelerator compat properties stick
> +     */
> +    configure_accelerator(current_machine, argv[0]);
> +
>      /*
>       * Migration object can only be created after global properties
>       * are applied correctly.
> @@ -4297,8 +4303,6 @@ int main(int argc, char **argv, char **envp)
>      current_machine->maxram_size = maxram_size;
>      current_machine->ram_slots = ram_slots;
>  
> -    configure_accelerator(current_machine, argv[0]);
> -
>      if (!qtest_enabled() && machine_class->deprecation_reason) {
>          error_report("Machine type '%s' is deprecated: %s",
>                       machine_class->name, machine_class->deprecation_reason);

-- 
Anthony PERARD

Anthony PERARD <anthony.perard@citrix.com> writes:

> On Tue, Mar 26, 2019 at 06:48:54PM +0100, Markus Armbruster wrote:
>> Known dependencies:
>> 
>> * migration object_init() must run before configure_blockdev(), so block
>>   backends can register migration blockers
>> 
>> * configure_blockdev() must run before machine_set_property(), so
>>   machine properties can refer to block backends.
>> 
>> * configure_accelerator() must run after machine creation, because it
>>   needs a machine
>> 
>> * configure_accelerator() must run before migration_object_init(), so
>>   migration_object_init() can see accelerator compat properties.
>> 
>> We're currently violating the last one, because weren't aware of it.  To
>> fix that, rejigger some more: move configure_accelerator() before
>> migration_object_init() (with a suitable comment), then see what else
>> breaks.  Could you try the appended patch and report whether it fixes
>> your problem?
>
> Yes, that patch works and doesn't break anything else with Xen.

Thanks!  I posted this fix together with another one as "[PATCH 0/2]
Compat props bug fixes".  I fogot to add your Tested-by, sorry.