[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: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 2/2] hw: fw_cfg: refactor fw_cfg_reboot() |
Date: |
Mon, 19 Nov 2018 08:01:04 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
ÀîÇ¿ <address@hidden> writes:
> 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
Cases:
0. "reboot-timeout" not specified (e.g. no -boot option given)
1. "reboot-timeout" specified, value out of bounds
1.a. < 0
1.b. > 0xffff
2. "reboot-timeout" specified, value okay
Gerd's advice is about case 1. Your patch implements it.
My question is about case 0.
Do you understand my question now?
[...]