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

[Markus Armbruster]

Philippe Mathieu-Daudé <address@hidden> writes:

> 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 :)

Makes sense, thanks.

[...]