Re: [Qemu-devel] [PATCH 2/2] hw: fw_cfg: refactor fw_cfg_reboot()

[Li Qiang]

Markus Armbruster <address@hidden> 于2018年11月19日周一 下午3:01写道：

> ÀîÇ¿ <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?
>


OK got. Once I think the 'reboot_timeout' can't be -1(as the user can't set
this),
but seems it's ok to be -1(the default value as no 'reboot-timeout'
specified).

I will prepare another patchset later.

Thanks,
Li Qiang



>
> [...]
>