[Qemu-devel] [PATCH 0/2] refactor fw_cfg_bootsplash() and fw_cfg_reboot()

[Qemu-devel] [PATCH 0/2] refactor fw_cfg_bootsplash() and fw_cfg_reboot()

[Li Qiang]

This patchset comes out as the result of the following review as per 
Markus's and Gerd's advice:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-10/msg06975.html
-->http://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00701.html

The second patch also fix that the user can set a negative
reboot_timeout.

Li Qiang (2):
  hw: fw_cfg: refactor fw_cfg_bootsplash()
  hw: fw_cfg: refactor fw_cfg_reboot()

 hw/nvram/fw_cfg.c | 63 ++++++++++++++++++++---------------------------
 vl.c              |  4 +--
 2 files changed, 29 insertions(+), 38 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/2] hw: fw_cfg: refactor fw_cfg_bootsplash()

[Li Qiang]

Currently when the splash-time value is bigger than 0xffff
we report and correct it, when it is less than 0 we just ingore it.
Also we use qemu_opt_get() to get 'splash-time', then convert it to a number
ourselves. This is wrong. This patch does following:
1. use qemu_opt_get_number() to parse 'splash-time'
2. exit when the splash-time is invalid or loading the splash file failed
3. simplify code

Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/nvram/fw_cfg.c | 40 ++++++++++++++++------------------------
 vl.c              |  2 +-
 2 files changed, 17 insertions(+), 25 deletions(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 946f765f7f..78f43dad93 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -118,55 +118,47 @@ error:
 
 static void fw_cfg_bootsplash(FWCfgState *s)
 {
-    int boot_splash_time = -1;
     const char *boot_splash_filename = NULL;
-    char *p;
+    const char *boot_splash_time = NULL;
     char *filename, *file_data;
     gsize file_size;
     int file_type;
-    const char *temp;
 
     /* get user configuration */
     QemuOptsList *plist = qemu_find_opts("boot-opts");
     QemuOpts *opts = QTAILQ_FIRST(&plist->head);
-    if (opts != NULL) {
-        temp = qemu_opt_get(opts, "splash");
-        if (temp != NULL) {
-            boot_splash_filename = temp;
-        }
-        temp = qemu_opt_get(opts, "splash-time");
-        if (temp != NULL) {
-            p = (char *)temp;
-            boot_splash_time = strtol(p, &p, 10);
-        }
-    }
+    boot_splash_filename = qemu_opt_get(opts, "splash");
+    boot_splash_time = qemu_opt_get(opts, "splash-time");
 
     /* insert splash time if user configurated */
-    if (boot_splash_time >= 0) {
+    if (boot_splash_time) {
+        int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);
         /* validate the input */
-        if (boot_splash_time > 0xffff) {
-            error_report("splash time is big than 65535, force it to 65535.");
-            boot_splash_time = 0xffff;
+        if (bst_val < 0 || bst_val > 0xffff) {
+            error_report("splash time is invalid,"
+                         "it should be a value between 0 and 65535");
+            exit(1);
         }
         /* use little endian format */
-        qemu_extra_params_fw[0] = (uint8_t)(boot_splash_time & 0xff);
-        qemu_extra_params_fw[1] = (uint8_t)((boot_splash_time >> 8) & 0xff);
+        qemu_extra_params_fw[0] = (uint8_t)(bst_val & 0xff);
+        qemu_extra_params_fw[1] = (uint8_t)((bst_val >> 8) & 0xff);
         fw_cfg_add_file(s, "etc/boot-menu-wait", qemu_extra_params_fw, 2);
     }
 
     /* insert splash file if user configurated */
-    if (boot_splash_filename != NULL) {
+    if (boot_splash_filename) {
         filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, boot_splash_filename);
         if (filename == NULL) {
-            error_report("failed to find file '%s'.", boot_splash_filename);
-            return;
+            error_report("failed to find file '%s'", boot_splash_filename);
+            exit(1);
         }
 
         /* loading file data */
         file_data = read_splashfile(filename, &file_size, &file_type);
         if (file_data == NULL) {
             g_free(filename);
-            return;
+            error_report("failed to read file '%s'", boot_splash_filename);
+            exit(1);
         }
         g_free(boot_splash_filedata);
         boot_splash_filedata = (uint8_t *)file_data;
diff --git a/vl.c b/vl.c
index 55bab005b6..be37da46f0 100644
--- a/vl.c
+++ b/vl.c
@@ -336,7 +336,7 @@ static QemuOptsList qemu_boot_opts = {
             .type = QEMU_OPT_STRING,
         }, {
             .name = "splash-time",
-            .type = QEMU_OPT_STRING,
+            .type = QEMU_OPT_NUMBER,
         }, {
             .name = "reboot-timeout",
             .type = QEMU_OPT_STRING,
-- 
2.17.1

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

[Markus Armbruster]

Li Qiang <liq3ea@163.com> writes:

> Currently when the splash-time value is bigger than 0xffff
> we report and correct it, when it is less than 0 we just ingore it.

s/ingore/ignore/

> Also we use qemu_opt_get() to get 'splash-time', then convert it to a number
> ourselves. This is wrong.

Well, doing it that way isn't wrong, it's just needlessly complicated
and error-prone.

Suggest starting a new paragraph right here.

>                           This patch does following:
> 1. use qemu_opt_get_number() to parse 'splash-time'
> 2. exit when the splash-time is invalid or loading the splash file failed
> 3. simplify code
>
> Signed-off-by: Li Qiang <liq3ea@163.com>
> ---
>  hw/nvram/fw_cfg.c | 40 ++++++++++++++++------------------------
>  vl.c              |  2 +-
>  2 files changed, 17 insertions(+), 25 deletions(-)
>
> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
> index 946f765f7f..78f43dad93 100644
> --- a/hw/nvram/fw_cfg.c
> +++ b/hw/nvram/fw_cfg.c
> @@ -118,55 +118,47 @@ error:
>  
>  static void fw_cfg_bootsplash(FWCfgState *s)
>  {
> -    int boot_splash_time = -1;
>      const char *boot_splash_filename = NULL;
> -    char *p;
> +    const char *boot_splash_time = NULL;
>      char *filename, *file_data;
>      gsize file_size;
>      int file_type;
> -    const char *temp;
>  
>      /* get user configuration */
>      QemuOptsList *plist = qemu_find_opts("boot-opts");
>      QemuOpts *opts = QTAILQ_FIRST(&plist->head);
> -    if (opts != NULL) {
> -        temp = qemu_opt_get(opts, "splash");
> -        if (temp != NULL) {
> -            boot_splash_filename = temp;
> -        }
> -        temp = qemu_opt_get(opts, "splash-time");
> -        if (temp != NULL) {
> -            p = (char *)temp;
> -            boot_splash_time = strtol(p, &p, 10);
> -        }
> -    }
> +    boot_splash_filename = qemu_opt_get(opts, "splash");
> +    boot_splash_time = qemu_opt_get(opts, "splash-time");

You first get "splash-time" as a string, and then ...
>  
>      /* insert splash time if user configurated */
> -    if (boot_splash_time >= 0) {
> +    if (boot_splash_time) {
> +        int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);

... you get it again as a number.  I figure you do that because
"splash-time not specified" is not the same as "splash-time=T" for any
T.  I don't like such interfaces.  Not this patch's fault.

Just noticed: qemu_extra_params_fw[] has external linkage, but is used
only in this function.  Care to make it static in this function in a
separate patch?

>          /* validate the input */
> -        if (boot_splash_time > 0xffff) {
> -            error_report("splash time is big than 65535, force it to 65535.");
> -            boot_splash_time = 0xffff;
> +        if (bst_val < 0 || bst_val > 0xffff) {
> +            error_report("splash time is invalid,"
> +                         "it should be a value between 0 and 65535");
> +            exit(1);
>          }
>          /* use little endian format */
> -        qemu_extra_params_fw[0] = (uint8_t)(boot_splash_time & 0xff);
> -        qemu_extra_params_fw[1] = (uint8_t)((boot_splash_time >> 8) & 0xff);
> +        qemu_extra_params_fw[0] = (uint8_t)(bst_val & 0xff);
> +        qemu_extra_params_fw[1] = (uint8_t)((bst_val >> 8) & 0xff);
>          fw_cfg_add_file(s, "etc/boot-menu-wait", qemu_extra_params_fw, 2);
>      }
>  
>      /* insert splash file if user configurated */
> -    if (boot_splash_filename != NULL) {
> +    if (boot_splash_filename) {
>          filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, boot_splash_filename);
>          if (filename == NULL) {
> -            error_report("failed to find file '%s'.", boot_splash_filename);
> -            return;
> +            error_report("failed to find file '%s'", boot_splash_filename);
> +            exit(1);
>          }
>  
>          /* loading file data */
>          file_data = read_splashfile(filename, &file_size, &file_type);
>          if (file_data == NULL) {
>              g_free(filename);
> -            return;
> +            error_report("failed to read file '%s'", boot_splash_filename);
> +            exit(1);
>          }
>          g_free(boot_splash_filedata);
>          boot_splash_filedata = (uint8_t *)file_data;
> diff --git a/vl.c b/vl.c
> index 55bab005b6..be37da46f0 100644
> --- a/vl.c
> +++ b/vl.c
> @@ -336,7 +336,7 @@ static QemuOptsList qemu_boot_opts = {
>              .type = QEMU_OPT_STRING,
>          }, {
>              .name = "splash-time",
> -            .type = QEMU_OPT_STRING,
> +            .type = QEMU_OPT_NUMBER,
>          }, {
>              .name = "reboot-timeout",
>              .type = QEMU_OPT_STRING,

Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[李强]

At 2018-11-17 00:33:34, "Markus Armbruster" <armbru@redhat.com> wrote:
>Li Qiang <liq3ea@163.com> writes:
>
>> Currently when the splash-time value is bigger than 0xffff
>> we report and correct it, when it is less than 0 we just ingore it.
>
>s/ingore/ignore/
>
>> Also we use qemu_opt_get() to get 'splash-time', then convert it to a number
>> ourselves. This is wrong.
>
>Well, doing it that way isn't wrong, it's just needlessly complicated
>and error-prone.
>
>Suggest starting a new paragraph right here.
>
>>                           This patch does following:
>> 1. use qemu_opt_get_number() to parse 'splash-time'
>> 2. exit when the splash-time is invalid or loading the splash file failed
>> 3. simplify code
>>
>> Signed-off-by: Li Qiang <liq3ea@163.com>
>> ---
>>  hw/nvram/fw_cfg.c | 40 ++++++++++++++++------------------------
>>  vl.c              |  2 +-
>>  2 files changed, 17 insertions(+), 25 deletions(-)
>>
>> diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
>> index 946f765f7f..78f43dad93 100644
>> --- a/hw/nvram/fw_cfg.c
>> +++ b/hw/nvram/fw_cfg.c
>> @@ -118,55 +118,47 @@ error:
>>  
>>  static void fw_cfg_bootsplash(FWCfgState *s)
>>  {
>> -    int boot_splash_time = -1;
>>      const char *boot_splash_filename = NULL;
>> -    char *p;
>> +    const char *boot_splash_time = NULL;
>>      char *filename, *file_data;
>>      gsize file_size;
>>      int file_type;
>> -    const char *temp;
>>  
>>      /* get user configuration */
>>      QemuOptsList *plist = qemu_find_opts("boot-opts");
>>      QemuOpts *opts = QTAILQ_FIRST(&plist->head);
>> -    if (opts != NULL) {
>> -        temp = qemu_opt_get(opts, "splash");
>> -        if (temp != NULL) {
>> -            boot_splash_filename = temp;
>> -        }
>> -        temp = qemu_opt_get(opts, "splash-time");
>> -        if (temp != NULL) {
>> -            p = (char *)temp;
>> -            boot_splash_time = strtol(p, &p, 10);
>> -        }
>> -    }
>> +    boot_splash_filename = qemu_opt_get(opts, "splash");
>> +    boot_splash_time = qemu_opt_get(opts, "splash-time");
>
>You first get "splash-time" as a string, and then ...
>>  
>>      /* insert splash time if user configurated */
>> -    if (boot_splash_time >= 0) {
>> +    if (boot_splash_time) {
>> +        int64_t bst_val = qemu_opt_get_number(opts, "splash-time", -1);
>
>... you get it again as a number.  I figure you do that because
>"splash-time not specified" is not the same as "splash-time=T" for any
>T.  I don't like such interfaces.  Not this patch's fault.
>
>Just noticed: qemu_extra_params_fw[] has external linkage, but is used
>only in this function.  Care to make it static in this function in a

>separate patch?


Will do in the next revision.


Thanks,
Li Qiang


>
>>          /* validate the input */
>> -        if (boot_splash_time > 0xffff) {
>> -            error_report("splash time is big than 65535, force it to 65535.");
>> -            boot_splash_time = 0xffff;
>> +        if (bst_val < 0 || bst_val > 0xffff) {
>> +            error_report("splash time is invalid,"
>> +                         "it should be a value between 0 and 65535");
>> +            exit(1);
>>          }
>>          /* use little endian format */
>> -        qemu_extra_params_fw[0] = (uint8_t)(boot_splash_time & 0xff);
>> -        qemu_extra_params_fw[1] = (uint8_t)((boot_splash_time >> 8) & 0xff);
>> +        qemu_extra_params_fw[0] = (uint8_t)(bst_val & 0xff);
>> +        qemu_extra_params_fw[1] = (uint8_t)((bst_val >> 8) & 0xff);
>>          fw_cfg_add_file(s, "etc/boot-menu-wait", qemu_extra_params_fw, 2);
>>      }
>>  
>>      /* insert splash file if user configurated */
>> -    if (boot_splash_filename != NULL) {
>> +    if (boot_splash_filename) {
>>          filename = qemu_find_file(QEMU_FILE_TYPE_BIOS, boot_splash_filename);
>>          if (filename == NULL) {
>> -            error_report("failed to find file '%s'.", boot_splash_filename);
>> -            return;
>> +            error_report("failed to find file '%s'", boot_splash_filename);
>> +            exit(1);
>>          }
>>  
>>          /* loading file data */
>>          file_data = read_splashfile(filename, &file_size, &file_type);
>>          if (file_data == NULL) {
>>              g_free(filename);
>> -            return;
>> +            error_report("failed to read file '%s'", boot_splash_filename);
>> +            exit(1);
>>          }
>>          g_free(boot_splash_filedata);
>>          boot_splash_filedata = (uint8_t *)file_data;
>> diff --git a/vl.c b/vl.c
>> index 55bab005b6..be37da46f0 100644
>> --- a/vl.c
>> +++ b/vl.c
>> @@ -336,7 +336,7 @@ static QemuOptsList qemu_boot_opts = {
>>              .type = QEMU_OPT_STRING,
>>          }, {
>>              .name = "splash-time",
>> -            .type = QEMU_OPT_STRING,
>> +            .type = QEMU_OPT_NUMBER,
>>          }, {
>>              .name = "reboot-timeout",
>>              .type = QEMU_OPT_STRING,
>
>Reviewed-by: Markus Armbruster <armbru@redhat.com>

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

[Li Qiang]

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. 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 <liq3ea@163.com>
---
 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);
-        }
+    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);
 }
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,
-- 
2.17.1

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

[Markus Armbruster]

Li Qiang <liq3ea@163.com> 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 <liq3ea@163.com>
> ---
>  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.

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.

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?

> 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,

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

[李强]

At 2018-11-17 00:52:58, "Markus Armbruster" <armbru@redhat.com> wrote:
>Li Qiang <liq3ea@163.com> 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 <liq3ea@163.com>
>> ---
>>  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,

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

[Markus Armbruster]

ÀîÇ¿ <liq3ea@163.com> writes:

> At 2018-11-17 00:52:58, "Markus Armbruster" <armbru@redhat.com> wrote:
>>Li Qiang <liq3ea@163.com> 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 <liq3ea@163.com>
>>> ---
>>>  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?

[...]

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

[Li Qiang]

Markus Armbruster <armbru@redhat.com> 于2018年11月19日周一 下午3:01写道：

> ÀîÇ¿ <liq3ea@163.com> writes:
>
> > At 2018-11-17 00:52:58, "Markus Armbruster" <armbru@redhat.com> wrote:
> >>Li Qiang <liq3ea@163.com> 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 <liq3ea@163.com>
> >>> ---
> >>>  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



>
> [...]
>