Re: [Qemu-devel] [PATCH] fw_cfg_reboot: ensure reboot_time is nonegative

["Philippe Mathieu-Daudé" <philmd@redhat.com>]

On 25/10/18 3:45, Li Qiang wrote:
> Hello Laszlo and Philippe ,
> 
> Thanks for your review,
> 
> 
> 
> Philippe Mathieu-Daudé <philmd@redhat.com <mailto:philmd@redhat.com>> 于 
> 2018年10月25日周四 上午6:56写道：
> 
>     Hi,
> 
>     On 24/10/18 13:35, Laszlo Ersek wrote:
>      > On 10/24/18 09:11, Li Qiang wrote:
>      >> This can avoid setting a negative value to
>      >> etc/boot-fail-wait.
> 
>     Li Qiang, can you add a qtest for this?
> 
> 
> I will try to write one.
> Is it ok to write it without this patch, as the test will be not passed?

It is OK to add the test after the fix, else the test would fail.
But now your test protect from future regressions.

> 
> Thanks,
> Li Qiang
> 
> 
>      >>
>      >> Signed-off-by: Li Qiang <liq3ea@gmail.com <mailto:liq3ea@gmail.com>>
>      >> ---
>      >>   hw/nvram/fw_cfg.c | 15 ++++++++++-----
>      >>   1 file changed, 10 insertions(+), 5 deletions(-)
>      >>
>      >> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>      >> index f4a52d8..276dcb1 100644
>      >> --- a/hw/nvram/fw_cfg.c
>      >> +++ b/hw/nvram/fw_cfg.c
>      >> @@ -199,12 +199,17 @@ static void fw_cfg_reboot(FWCfgState *s)
>      >>               reboot_timeout = strtol(p, &p, 10);
>      >>           }
>      >>       }
>      >> -    /* validate the input */
>      >> -    if (reboot_timeout > 0xffff) {
>      >> -        error_report("reboot timeout is larger than 65535,
>     force it to 65535.");
>      >> -        reboot_timeout = 0xffff;
>      >> +
>      >> +    if (reboot_timeout >= 0) {
>      >> +        /* validate the input */
>      >> +        if (reboot_timeout > 0xffff) {
>      >> +            error_report("reboot timeout is larger than 65535,"
>      >> +                         "force it to 65535.");
>      >> +            reboot_timeout = 0xffff;
>      >> +        }
>      >> +        fw_cfg_add_file(s, "etc/boot-fail-wait",
>      >> +                        g_memdup(&reboot_timeout, 4), 4);
>      >>       }
>      >> -    fw_cfg_add_file(s, "etc/boot-fail-wait",
>     g_memdup(&reboot_timeout, 4), 4);
>      >>   }
>      >>
>      >>   static void fw_cfg_write(FWCfgState *s, uint8_t value)
>      >>
>      >
>      > I don't feel strongly about fixing this issue.
>      >
>      > However, if we decide to fix it, we should start with the bare-bones
>      > strtol() call, visible at the top of the context. I'm not
>     up-to-date on
>      > what's the best QEMU helper function for this, but I seem to
>     remember it
>      > checks for trailing garbage, and perhaps even for range. Maybe we
>     should
> 
>     Are you suggesting qemu_strtoul()? I agree this would be cleaner.
> 
> 
> 
> I reference the 'boot_splash_time' in fw_cfg_bootsplash.
> We can also change there.
> 
>      > even use a different (better) option parsing facility thatn
>      > qemu_opt_get(). Adding Eric and Markus.
>      >
>      > Also, I would suggest forcing negative values (that were explicitly
>      > specified) to some sensible positive default, such as 5 seconds
>     or so.
>      >
>      > Thanks
>      > Laszlo
>      >
>