Re: [Qemu-devel] [PATCH v3 3/3] nvram: fw_cfg: Fix -boot options in nvram/fw_cfg

[Markus Armbruster]

Peter Crosthwaite <address@hidden> writes:

> Multiple -boot options with the same ID are merged.  All but the
> one without an ID are to be silently ignored.
>
> In other places, we query boot options with qemu_get_boot_opts().
> This is correct.
>
> In this instance, we instead query whatever options come first in the
> list.  This is wrong. When the -boot processed first happens to
> have an ID, options are taken from that ID, and the ones specified
> without ID are silently ignored.
>
> 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>
> ---
> changed from v2:
> Taken more commit message from 36ad0e9 (Markus Review)
>
>  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 */
> @@ -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) {

I'd appreciate a follow-up patch making "splash-time" and
"reboot-timeout" QEMU_OPT_NUMBER, gotten with qemu_opt_number().  But
it's not required to get this one in.