[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] fw_cfg_reboot: ensure reboot_time is nonegative
From: |
Li Qiang |
Subject: |
Re: [Qemu-devel] [PATCH] fw_cfg_reboot: ensure reboot_time is nonegative |
Date: |
Tue, 30 Oct 2018 09:29:40 +0800 |
Philippe Mathieu-Daudé <address@hidden> 于2018年10月30日周二 上午7:50写道:
> On 25/10/18 3:45, Li Qiang wrote:
> > Hello Laszlo and Philippe ,
> >
> > Thanks for your review,
> >
> >
> >
> > Philippe Mathieu-Daudé <address@hidden <mailto:address@hidden>> 于
> > 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.
>
>
Ok, I think this fix is not that bad, this is the same as
'boot_splash_time'.
As for Laszlo's consideration, we think we can send another patch to
drop strtol in both 'boot_splash_time' and this 'reboot-timeout'.
Thanks,
Li Qiang
>
> > Thanks,
> > Li Qiang
> >
> >
> > >>
> > >> Signed-off-by: Li Qiang <address@hidden <mailto:
> address@hidden>>
> > >> ---
> > >> 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
> > >
> >
>