Re: [Qemu-devel] [PATCH v2 4/4] nvram: fw_cfg: Fix -boot options in nvram/fw_cfg

[Markus Armbruster]

Peter Crosthwaite <address@hidden> writes:

> When accessing boot options, we query whatever options come first in
> the boot opts list.  This is wrong.

You could copy a bit more text from commit 36ad0e9 to explain how
exactly it is wrong.  First three paragraphs, perhaps.

> Use qemu_get_boot_opts() to fix these bugs.
>
> This change is similar to and based on 36ad0e9.
>
> We also take to opportunity to remove the now unneeded null boot-opts
> conditional, removing a level of indentation on usage code.
>
> Signed-off-by: Peter Crosthwaite <address@hidden>
> ---
>
>  hw/nvram/fw_cfg.c | 36 ++++++++++++++++--------------------
>  1 file changed, 16 insertions(+), 20 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 282341a..8537669 100644
> --- 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);
> -    if (opts != NULL) {
> -        temp = qemu_opt_get(opts, "splash");
> -        if (temp != NULL) {
> -            boot_splash_filename = temp;
> -        }
> -        temp = qemu_opt_get(opts, "splash-time");
> -        if (temp != NULL) {
> -            p = (char *)temp;
> -            boot_splash_time = strtol(p, (char **)&p, 10);
> -        }
> +    QemuOpts *opts = qemu_get_boot_opts();
> +
> +    temp = qemu_opt_get(opts, "splash");
> +    if (temp != NULL) {
> +        boot_splash_filename = temp;
> +    }
> +    temp = qemu_opt_get(opts, "splash-time");
> +    if (temp != NULL) {
> +        p = (char *)temp;
> +        boot_splash_time = strtol(p, (char **)&p, 10);
>      }
>  
>      /* insert splash time if user configurated */

Separate issue, and I'm not demanding you fix it: strtol() can fail.
Parameter "splash-time" should be QEMU_OPT_NUMBER, and it should be
gotten with qemu_opt_get_number().

> @@ -191,14 +189,12 @@ static void fw_cfg_reboot(FWCfgState *s)
>      const char *temp;
>  
>      /* get user configuration */
> -    QemuOptsList *plist = qemu_find_opts("boot-opts");
> -    QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> -    if (opts != NULL) {
> -        temp = qemu_opt_get(opts, "reboot-timeout");
> -        if (temp != NULL) {
> -            p = (char *)temp;
> -            reboot_timeout = strtol(p, (char **)&p, 10);
> -        }
> +    QemuOpts *opts = qemu_get_boot_opts();
> +
> +    temp = qemu_opt_get(opts, "reboot-timeout");
> +    if (temp != NULL) {
> +        p = (char *)temp;
> +        reboot_timeout = strtol(p, (char **)&p, 10);
>      }
>      /* validate the input */
>      if (reboot_timeout > 0xffff) {

Likewise.

Patch looks good.  Let's wait a few days for qemu_find_opts_singleton(),
okay?