Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties

[Philippe Mathieu-Daudé]

On 3/5/19 11:38 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <address@hidden> writes:
>> On 2/25/19 7:37 PM, Markus Armbruster wrote:
>>> qemu-system-FOO's main() acts on command line arguments in its own
>>> idiosyncratic order.  There's not much method to its madness.
>>> Whenever we find a case where one kind of command line argument needs
>>> to refer to something created for another kind later, we rejigger the
>>> order.
>>>
>>> Block devices get created long after machine properties get processed.
>>> Therefore, block device machine properties can be created, but not
>>> set.  No such properties exist.  But the next commit will create some.
>>> Time to rejigger again: create block devices earlier.
>>>
>>> Signed-off-by: Markus Armbruster <address@hidden>
>>> ---
>>>  vl.c | 66 ++++++++++++++++++++++++++++++------------------------------
>>>  1 file changed, 33 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/vl.c b/vl.c
>>> index 6ce3d2d448..5cb0810ffa 100644
>>> --- a/vl.c
>>> +++ b/vl.c
>>> @@ -4232,6 +4232,39 @@ int main(int argc, char **argv, char **envp)
>>>          exit(0);
>>>      }
>>>  
>>
>> Can we extract this from the main, maybe as "parse_blockdev()"? This
>> will ease further     :)
> 
> I can try and see whether I like it.
> 
>>> +    /* If the currently selected machine wishes to override the 
>>> units-per-bus
>>> +     * property of its default HBA interface type, do so now. */
>>> +    if (machine_class->units_per_default_bus) {
>>> +        override_max_devs(machine_class->block_default_type,
>>> +                          machine_class->units_per_default_bus);
>>> +    }
>>> +
>>> +    /* open the virtual block devices */
>>> +    while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
>>> +        BlockdevOptions_queue *bdo = QSIMPLEQ_FIRST(&bdo_queue);
>>> +
>>> +        QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
>>> +        loc_push_restore(&bdo->loc);
>>> +        qmp_blockdev_add(bdo->bdo, &error_fatal);
>>> +        loc_pop(&bdo->loc);
>>> +        qapi_free_BlockdevOptions(bdo->bdo);
>>> +        g_free(bdo);
>>> +    }
>>> +    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
>>> +        qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
>>> +                          NULL, NULL);
>>> +    }
>>> +    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>>> +                          &machine_class->block_default_type, 
>>> &error_fatal)) {
>>> +        /* We printed help */
>>> +        exit(0);
>>> +    }
>>> +
>>> +    default_drive(default_cdrom, snapshot, 
>>> machine_class->block_default_type, 2,
>>> +                  CDROM_OPTS);
>>> +    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>> +    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>> +
>>
>> Can you add a comment here such:
>>
>>    /*
>>     * Arguments setting properties on devices has to be processed before
>>     * the following qemu_opt_foreach(...machine_set_property...) call.
>>     */
> 
> Hmm, is this accurate?  The issue this patch addresses is
> 
>     "arguments creating block backends have to be processed before
>     machine_set_property()",
> 
> which is an instance of
> 
>     "arguments creating backends ...",
> 
> which is an instance of
> 
>     "arguments creating stuff machine property values can reference ..."
> 
> The patch only addresses the instance "block backends".  I have no
> appetite for attacking more general problems right now.
> 
> See also
> 
>     Subject: Re: [Qemu-devel] [PATCH RFC 0/3] qdev: order devices by priority 
> before creating them
>     Date: Wed, 11 May 2016 09:51:39 +0200
>     Message-ID: <address@hidden>
>     https://lists.nongnu.org/archive/html/qemu-devel/2016-05/msg01589.html
> 
>     Subject: Re: [Qemu-devel] [PATCH] vl: Delay initialization of memory 
> backends
>     Date: Fri, 02 Sep 2016 08:13:17 +0200
>     Message-ID: <address@hidden>
>     https://lists.nongnu.org/archive/html/qemu-devel/2016-09/msg00163.html
> 
> That said, I'm fine with adding a comment explaining the more general
> mess.  Can you give me some hints on what future readers will find
> useful?

This can be as simple as:

     /*
      * Arguments creating block backends have to be processed before
      * machine_set_property().
      */
     parse_blockdev(...);

So if I have to hack around in the future I'd look at the commit
introducing this comment:

  Block devices get created long after machine properties get processed.
  Therefore, block device machine properties can be created, but not
  set.  No such properties exist.  But the next commit will create some.

So I won't rejig this at his previous place :)

> 
>>>      machine_opts = qemu_get_machine_opts();
>>>      qemu_opt_foreach(machine_opts, machine_set_property, current_machine,
>>>                       &error_fatal);
>>> @@ -4355,39 +4388,6 @@ int main(int argc, char **argv, char **envp)
>>>      ram_mig_init();
>>>      dirty_bitmap_mig_init();
>>>  
>>> -    /* If the currently selected machine wishes to override the 
>>> units-per-bus
>>> -     * property of its default HBA interface type, do so now. */
>>> -    if (machine_class->units_per_default_bus) {
>>> -        override_max_devs(machine_class->block_default_type,
>>> -                          machine_class->units_per_default_bus);
>>> -    }
>>> -
>>> -    /* open the virtual block devices */
>>> -    while (!QSIMPLEQ_EMPTY(&bdo_queue)) {
>>> -        BlockdevOptions_queue *bdo = QSIMPLEQ_FIRST(&bdo_queue);
>>> -
>>> -        QSIMPLEQ_REMOVE_HEAD(&bdo_queue, entry);
>>> -        loc_push_restore(&bdo->loc);
>>> -        qmp_blockdev_add(bdo->bdo, &error_fatal);
>>> -        loc_pop(&bdo->loc);
>>> -        qapi_free_BlockdevOptions(bdo->bdo);
>>> -        g_free(bdo);
>>> -    }
>>> -    if (snapshot || replay_mode != REPLAY_MODE_NONE) {
>>> -        qemu_opts_foreach(qemu_find_opts("drive"), drive_enable_snapshot,
>>> -                          NULL, NULL);
>>> -    }
>>> -    if (qemu_opts_foreach(qemu_find_opts("drive"), drive_init_func,
>>> -                          &machine_class->block_default_type, 
>>> &error_fatal)) {
>>> -        /* We printed help */
>>> -        exit(0);
>>> -    }
>>> -
>>> -    default_drive(default_cdrom, snapshot, 
>>> machine_class->block_default_type, 2,
>>> -                  CDROM_OPTS);
>>> -    default_drive(default_floppy, snapshot, IF_FLOPPY, 0, FD_OPTS);
>>> -    default_drive(default_sdcard, snapshot, IF_SD, 0, SD_OPTS);
>>> -
>>>      qemu_opts_foreach(qemu_find_opts("mon"),
>>>                        mon_init_func, NULL, &error_fatal);
>>>  
>>
>> Reviewed-by: Philippe Mathieu-Daudé <address@hidden>
> 
> Thanks!
>