Philippe Mathieu-Daudé <phi...@redhat.com> 于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é <phi...@redhat.com <mailto:phi...@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. > > 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 <liq...@gmail.com <mailto: > liq...@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 > > > > > >
