| Message ID | 1540365080-6844-1-git-send-email-liq3ea@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | fw_cfg_reboot: ensure reboot_time is nonegative | expand |
On 10/24/18 09:11, Li Qiang wrote: > This can avoid setting a negative value to > etc/boot-fail-wait. > > Signed-off-by: Li Qiang <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 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
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? >> >> Signed-off-by: Li Qiang <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. > 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 >
Hello Laszlo and Philippe , Thanks for your review, Philippe Mathieu-Daudé <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? Thanks, Li Qiang > >> > >> Signed-off-by: Li Qiang <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 > > >
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 > > >
Philippe Mathieu-Daudé <philmd@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é <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. > > 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 <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 > > > > > >
Laszlo Ersek <lersek@redhat.com> writes: > On 10/24/18 09:11, Li Qiang wrote: >> This can avoid setting a negative value to >> etc/boot-fail-wait. >> >> Signed-off-by: Li Qiang <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 > even use a different (better) option parsing facility thatn > qemu_opt_get(). Adding Eric and Markus. Whenever we qemu_opt_get() a string, then convert it to a number ourselves, we're doing it wrong. Can you use qemu_opt_get_number()? When you have to convert string to integer, use the qemu_strtoX() that gives you the width and signedness you need. Or maybe one of the parse_uint*(). > Also, I would suggest forcing negative values (that were explicitly > specified) to some sensible positive default, such as 5 seconds or so. > > Thanks > Laszlo
On 10/24/18 6:35 AM, Laszlo Ersek wrote: > On 10/24/18 09:11, Li Qiang wrote: >> This can avoid setting a negative value to >> etc/boot-fail-wait. >> >> Signed-off-by: Li Qiang <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); Looks like Markus handled the question about replacing strtol(), so I'll just point out one additional nit: >> } >> } >> - /* validate the input */ >> - if (reboot_timeout > 0xffff) { >> - error_report("reboot timeout is larger than 65535, force it to 65535."); Pre-existing, but now's as good a time as any to improve it: >> - 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."); error_report() callers generally do not end with trailing '.'
Thanks Eric and Markus's review, I have sent out another patch. I'm not sure what's the effect when the etc/reboot-timeout and etc/splash-time is 0 in seabios, so CC'd Gerd. Maybe it can be more simplicity. Thanks, Li Qiang Eric Blake <eblake@redhat.com> 于2018年10月31日周三 下午11:55写道： > On 10/24/18 6:35 AM, Laszlo Ersek wrote: > > On 10/24/18 09:11, Li Qiang wrote: > >> This can avoid setting a negative value to > >> etc/boot-fail-wait. > >> > >> Signed-off-by: Li Qiang <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); > > Looks like Markus handled the question about replacing strtol(), so I'll > just point out one additional nit: > > >> } > >> } > >> - /* validate the input */ > >> - if (reboot_timeout > 0xffff) { > >> - error_report("reboot timeout is larger than 65535, force it to > 65535."); > > Pre-existing, but now's as good a time as any to improve it: > > >> - 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."); > > error_report() callers generally do not end with trailing '.' > > -- > Eric Blake, Principal Software Engineer > Red Hat, Inc. +1-919-301-3266 > Virtualization: qemu.org | libvirt.org >
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)
This can avoid setting a negative value to etc/boot-fail-wait. Signed-off-by: Li Qiang <liq3ea@gmail.com> --- hw/nvram/fw_cfg.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-)