diff mbox

[v1,0/3] Introduce qemu_get_boot_opts()

Message ID CAEgOgz7qGYUnJxCkmX5XeNq+Gd0k5=+-pRJ82PTvZ+Tu_L1YWQ@mail.gmail.com
State New
Headers show

Commit Message

Peter Crosthwaite April 17, 2014, 2:50 a.m. UTC 
On Wed, Apr 16, 2014 at 6:05 PM, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>
>> Hi Markus,
>>
>> This series introduces qemu_get_boot_opts(), in much the same way as
>> was done for qemu_get_machine_opts().
>>
>> As usual, I have out-of-scope and out-of-tree usages :) But P3 does
>> clean up the one existing instance of the long-and-awkward form of
>> this query and makes it consistent with an immediately surrounding
>> qemu_get_machine_opts().
>
> I doubt this is worthwhile on its own as it stands.
>
> However, you missed the two uses of "boot-opts" in hw/nvram/fw_cfg.c.
> Since these uses are currently wrong the same way as the the uses of
> "machine" fixed in commit 36ad0e9 were, covering them could strengthen
> your case quite a bit,
>

Yeh I noticed it, andI thought that "get the first list element" code
was weird. And I decided to let sleeing dogs lie. But are you saying
its wrong and we can just simplify to:

Comments

Markus Armbruster April 17, 2014, 6:45 a.m. UTC | #1 
Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:

> On Wed, Apr 16, 2014 at 6:05 PM, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Crosthwaite <peter.crosthwaite@xilinx.com> writes:
>>
>>> Hi Markus,
>>>
>>> This series introduces qemu_get_boot_opts(), in much the same way as
>>> was done for qemu_get_machine_opts().
>>>
>>> As usual, I have out-of-scope and out-of-tree usages :) But P3 does
>>> clean up the one existing instance of the long-and-awkward form of
>>> this query and makes it consistent with an immediately surrounding
>>> qemu_get_machine_opts().
>>
>> I doubt this is worthwhile on its own as it stands.
>>
>> However, you missed the two uses of "boot-opts" in hw/nvram/fw_cfg.c.
>> Since these uses are currently wrong the same way as the the uses of
>> "machine" fixed in commit 36ad0e9 were, covering them could strengthen
>> your case quite a bit,
>>
>
> Yeh I noticed it, andI thought that "get the first list element" code
> was weird. And I decided to let sleeing dogs lie. But are you saying
> its wrong and we can just simplify to:
>
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -125,18 +125,16 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>      const char *temp;
>
>      /* get user configuration */
> -    QemuOptsList *plist = qemu_find_opts("boot-opts");
> -    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> +    QemuOpts *opts = qemu_get_boot_opts();
>
> ? Happy to make this change.

Also drop the if (opts != NULL).  Suggest to pattern the commit message
after commit 36ad0e9, or at least reference it.
diff mbox

Patch

--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -125,18 +125,16 @@  static void fw_cfg_bootsplash(FWCfgState *s)
     const char *temp;

     /* get user configuration */
-    QemuOptsList *plist = qemu_find_opts("boot-opts");
-    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
+    QemuOpts *opts = qemu_get_boot_opts();

? Happy to make this change.

Regards,
Peter