[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 2/2] hw: fw_cfg: refactor fw_cfg_reboot()
From: |
李强 |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] hw: fw_cfg: refactor fw_cfg_reboot() |
Date: |
Mon, 19 Nov 2018 09:24:06 +0800 (CST) |
At 2018-11-17 00:52:58, "Markus Armbruster" <address@hidden> wrote:
>Li Qiang <address@hidden> writes:
>
>> Currently the user can set a negative reboot_timeout.
>> Also it is wrong to parse 'reboot-timeout' with qemu_opt_get() and then
>> convert it to number.
>
>Again, it's not wrong per se, just needlessly complicated and
>error-prone. What makes it wrong is ...
>
>> convert it to number. This patch refactor this function by following:
>> 1. ensure reboot_timeout is in 0~0xffff
>> 2. use qemu_opt_get_number() to parse reboot_timeout
>> 3. simlify code
>>
>> Signed-off-by: Li Qiang <address@hidden>
>> ---
>> hw/nvram/fw_cfg.c | 23 +++++++++++------------
>> vl.c | 2 +-
>> 2 files changed, 12 insertions(+), 13 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 78f43dad93..6aca80846a 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -178,24 +178,23 @@ static void fw_cfg_bootsplash(FWCfgState *s)
>>
>> static void fw_cfg_reboot(FWCfgState *s)
>> {
>> - int reboot_timeout = -1;
>> - char *p;
>> - const char *temp;
>> + const char *reboot_timeout = NULL;
>>
>> /* 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, &p, 10);
>
>... the total lack of error checking here. Same in PATCH 1.
>
Got.
>Here's my attempt at a clearer commit message:
>
> fw_cfg: Fix -boot reboot-timeout error checking
>
> fw_cfg_reboot() gets option parameter "reboot-timeout" with
> qemu_opt_get(), then converts it to an integer by hand. It neglects
> to check that conversion for errors, and fails to reject negative
> values. Positive values above the limit get reported and replaced
> by the limit.
>
> Check for conversion errors properly, and reject all values outside
> 0..0xffff.
>
Thanks for your advice, I appreciate it and will change in the revision version.
>PATCH 1's commit message could be improved the same way.
>
>> - }
>> + reboot_timeout = qemu_opt_get(opts, "reboot-timeout");
>> +
>> + if (reboot_timeout == NULL) {
>> + return;
>> }
>> + int64_t rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
>> +
>> /* validate the input */
>> - if (reboot_timeout > 0xffff) {
>> - error_report("reboot timeout is larger than 65535, force it to
>> 65535.");
>> - reboot_timeout = 0xffff;
>> + if (rt_val < 0 || rt_val > 0xffff) {
>> + error_report("reboot timeout is invalid,"
>> + "it should be a value between 0 and 65535");
>> + exit(1);
>> }
>> fw_cfg_add_file(s, "etc/boot-fail-wait", g_memdup(&reboot_timeout, 4),
>> 4);
>> }
>
>Change in behavior when "reboot-timeout" isn't specified.
>
>Before your patch, we fw_cfg_add_file() with a value of -1.
>
>After your patch, we don't fw_cfg_add_file().
>
>Why is that okay?
>
Here I following Gerd's advice.
For values >0xffff or < 0, report and exit.
-->http://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00551.html
Thanks,
Li Qiang
>> diff --git a/vl.c b/vl.c
>> index be37da46f0..086127ff0b 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -339,7 +339,7 @@ static QemuOptsList qemu_boot_opts = {
>> .type = QEMU_OPT_NUMBER,
>> }, {
>> .name = "reboot-timeout",
>> - .type = QEMU_OPT_STRING,
>> + .type = QEMU_OPT_NUMBER,
>> }, {
>> .name = "strict",
>> .type = QEMU_OPT_BOOL,