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

Markus Armbruster posted 6 patches 6 years, 8 months ago
Maintainers: "Michael S. Tsirkin" <mst@redhat.com>, Marcel Apfelbaum <marcel.apfelbaum@gmail.com>, Paolo Bonzini <pbonzini@redhat.com>, Max Reitz <mreitz@redhat.com>, "Andreas Färber" <afaerber@suse.de>, Richard Henderson <rth@twiddle.net>, Kevin Wolf <kwolf@redhat.com>, Eduardo Habkost <ehabkost@redhat.com>
There is a newer version of this series
[Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties
Posted by Markus Armbruster 6 years, 8 months ago
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 <armbru@redhat.com>
---
 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);
     }
 
+    /* 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);
+
     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);
 
-- 
2.17.2


Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties
Posted by Philippe Mathieu-Daudé 6 years, 8 months ago
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 <armbru@redhat.com>
> ---
>  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 rejigger :)

> +    /* 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.
    */

>      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é <philmd@redhat.com>

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties
Posted by Markus Armbruster 6 years, 8 months ago
Philippe Mathieu-Daudé <philmd@redhat.com> 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 <armbru@redhat.com>
>> ---
>>  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 rejigger :)

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: <87y47hhw1g.fsf@dusky.pond.sub.org>
    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: <87poomg77m.fsf@dusky.pond.sub.org>
    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?

>>      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é <philmd@redhat.com>

Thanks!

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties
Posted by Philippe Mathieu-Daudé 6 years, 8 months ago
On 3/5/19 11:38 AM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <philmd@redhat.com> 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 <armbru@redhat.com>
>>> ---
>>>  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: <87y47hhw1g.fsf@dusky.pond.sub.org>
>     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: <87poomg77m.fsf@dusky.pond.sub.org>
>     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é <philmd@redhat.com>
> 
> Thanks!
> 

Re: [Qemu-devel] [RFC PATCH 5/6] vl: Create block backends before setting machine properties
Posted by Markus Armbruster 6 years, 8 months ago
Philippe Mathieu-Daudé <philmd@redhat.com> writes:

> On 3/5/19 11:38 AM, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@redhat.com> 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 <armbru@redhat.com>
>>>> ---
>>>>  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: <87y47hhw1g.fsf@dusky.pond.sub.org>
>>     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: <87poomg77m.fsf@dusky.pond.sub.org>
>>     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.

[...]