[Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Li Qiang]

And also do some code cleanup.
A lot of thanks to Markus's review and advice.

Li Qiang (3):
  fw_cfg: fix -boot bootsplash error checking
  fw_cfg: fix -boot reboot-timeout error checking
  fw_cfg: make qemu_extra_params_fw locally

 hw/nvram/fw_cfg.c       | 68 ++++++++++++++++++-----------------------
 include/sysemu/sysemu.h |  1 -
 vl.c                    |  4 +--
 3 files changed, 32 insertions(+), 41 deletions(-)

-- 
2.17.1

[Qemu-devel] [PATCH 1/3] fw_cfg: fix -boot bootsplash error checking

[Li Qiang]

fw_cfg_bootsplash() gets option parameter "splash-time"
with qemu_opt_get(), then converts it to an integer by hand.
It neglects to check that conversion for errors. This is
needlessly complicated and error-prone. But as "splash-time
not specified" is not the same as "splash-time=T" for any T,
we need use qemu_opt_get() to check if splash time exists.
This patch also make the qemu exit when finding or loading
splash file failed.

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 fa25d1ae2d..96ac0ddcf6 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

[Qemu-devel] [PATCH 2/3] fw_cfg: fix -boot reboot-timeout error checking

[Li Qiang]

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.
This patch checks for conversion errors properly, and reject all valus
outside 0...0xffff.

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

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 78f43dad93..0489d63a1b 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -178,26 +178,25 @@ 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;
+    int64_t rt_val = -1;
 
     /* 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) {
+        rt_val = qemu_opt_get_number(opts, "reboot-timeout", -1);
+        /* validate the input */
+        if (rt_val < 0 || rt_val > 0xffff) {
+            error_report("reboot timeout is invalid,"
+                         "it should be a value between 0 and 65535");
+            exit(1);
         }
     }
-    /* 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(&rt_val, 4), 4);
 }
 
 static void fw_cfg_write(FWCfgState *s, uint8_t value)
diff --git a/vl.c b/vl.c
index 96ac0ddcf6..38a1759461 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

[Qemu-devel] [PATCH 3/3] fw_cfg: make qemu_extra_params_fw locally

[Li Qiang]

qemu_extra_params_fw[] has external linkage, but is used
only in fw_cfg_bootsplash(), it makes sense to make it
locally.

Suggested-by: Markus Armbruster <armbru@redhat.com>
Signed-off-by: Li Qiang <liq3ea@163.com>
---
 hw/nvram/fw_cfg.c       | 1 +
 include/sysemu/sysemu.h | 1 -
 2 files changed, 1 insertion(+), 1 deletion(-)

diff --git a/hw/nvram/fw_cfg.c b/hw/nvram/fw_cfg.c
index 0489d63a1b..4eddd6a885 100644
--- a/hw/nvram/fw_cfg.c
+++ b/hw/nvram/fw_cfg.c
@@ -120,6 +120,7 @@ static void fw_cfg_bootsplash(FWCfgState *s)
 {
     const char *boot_splash_filename = NULL;
     const char *boot_splash_time = NULL;
+    uint8_t qemu_extra_params_fw[2];
     char *filename, *file_data;
     gsize file_size;
     int file_type;
diff --git a/include/sysemu/sysemu.h b/include/sysemu/sysemu.h
index 8d6095d98b..f8608d24d9 100644
--- a/include/sysemu/sysemu.h
+++ b/include/sysemu/sysemu.h
@@ -133,7 +133,6 @@ extern uint8_t *boot_splash_filedata;
 extern size_t boot_splash_filedata_size;
 extern bool enable_mlock;
 extern bool enable_cpu_pm;
-extern uint8_t qemu_extra_params_fw[2];
 extern QEMUClockType rtc_clock;
 extern const char *mem_path;
 extern int mem_prealloc;
-- 
2.17.1

Re: [Qemu-devel] [PATCH 1/3] fw_cfg: fix -boot bootsplash error checking

[Markus Armbruster]

Li Qiang <liq3ea@163.com> writes:

> fw_cfg_bootsplash() gets option parameter "splash-time"
> with qemu_opt_get(), then converts it to an integer by hand.
> It neglects to check that conversion for errors. This is
> needlessly complicated and error-prone. But as "splash-time
> not specified" is not the same as "splash-time=T" for any T,
> we need use qemu_opt_get() to check if splash time exists.
> This patch also make the qemu exit when finding or loading
> splash file failed.
>
> 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");

Let's match the parameter name exactly: "splash-time is invalid, ..."

> +            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);

Drop this error_report(), please.  read_splashfile() already reports the
failure more usefully.

> +            exit(1);
>          }
>          g_free(boot_splash_filedata);
>          boot_splash_filedata = (uint8_t *)file_data;
> diff --git a/vl.c b/vl.c
> index fa25d1ae2d..96ac0ddcf6 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,

With these minor error reporting improvements:
Reviewed-by: Markus Armbruster <armbru@redhat.com>

Re: [Qemu-devel] [PATCH 2/3] fw_cfg: fix -boot reboot-timeout error checking

[Markus Armbruster]

Li Qiang <liq3ea@163.com> writes:

> 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.
> This patch checks for conversion errors properly, and reject all valus

Spelling: all values.

> outside 0...0xffff.
>
> Signed-off-by: Li Qiang <liq3ea@163.com>

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

Re: [Qemu-devel] [PATCH 3/3] fw_cfg: make qemu_extra_params_fw locally

[Markus Armbruster]

Li Qiang <liq3ea@163.com> writes:

> qemu_extra_params_fw[] has external linkage, but is used
> only in fw_cfg_bootsplash(), it makes sense to make it
> locally.
>
> Suggested-by: Markus Armbruster <armbru@redhat.com>
> Signed-off-by: Li Qiang <liq3ea@163.com>

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

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Markus Armbruster]

Li Qiang <liq3ea@163.com> writes:

> And also do some code cleanup.
> A lot of thanks to Markus's review and advice.
>
> Li Qiang (3):
>   fw_cfg: fix -boot bootsplash error checking
>   fw_cfg: fix -boot reboot-timeout error checking
>   fw_cfg: make qemu_extra_params_fw locally
>
>  hw/nvram/fw_cfg.c       | 68 ++++++++++++++++++-----------------------
>  include/sysemu/sysemu.h |  1 -
>  vl.c                    |  4 +--
>  3 files changed, 32 insertions(+), 41 deletions(-)

fw_cfg.c has no maintainer.  Who would be willing to merge this?

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Philippe Mathieu-Daudé]

Hi Markus, Li.

On 20/11/18 20:34, Markus Armbruster wrote:
> Li Qiang <liq3ea@163.com> writes:
> 
>> And also do some code cleanup.
>> A lot of thanks to Markus's review and advice.
>>
>> Li Qiang (3):
>>    fw_cfg: fix -boot bootsplash error checking
>>    fw_cfg: fix -boot reboot-timeout error checking
>>    fw_cfg: make qemu_extra_params_fw locally
>>
>>   hw/nvram/fw_cfg.c       | 68 ++++++++++++++++++-----------------------
>>   include/sysemu/sysemu.h |  1 -
>>   vl.c                    |  4 +--
>>   3 files changed, 32 insertions(+), 41 deletions(-)
> 
> fw_cfg.c has no maintainer.  Who would be willing to merge this?

I have various patches for fw_cfg waiting 4.0 to start, one add a 
MAINTAINERS entry covered by Laszlo, Gerd and myself.

This series was published way before soft freeze, but I am not sure it 
applies for hard freeze. If so, it might goes thru Paolo's Misc tree, 
else we'll take it for 4.0 (via fw_cfg-next).

Regards,

Phil.

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[李强]

At 2018-11-21 03:59:28, "Philippe Mathieu-Daudé" <philmd@redhat.com> wrote:
>Hi Markus, Li.
>
>On 20/11/18 20:34, Markus Armbruster wrote:
>> Li Qiang <liq3ea@163.com> writes:
>> 
>>> And also do some code cleanup.
>>> A lot of thanks to Markus's review and advice.
>>>
>>> Li Qiang (3):
>>>    fw_cfg: fix -boot bootsplash error checking
>>>    fw_cfg: fix -boot reboot-timeout error checking
>>>    fw_cfg: make qemu_extra_params_fw locally
>>>
>>>   hw/nvram/fw_cfg.c       | 68 ++++++++++++++++++-----------------------
>>>   include/sysemu/sysemu.h |  1 -
>>>   vl.c                    |  4 +--
>>>   3 files changed, 32 insertions(+), 41 deletions(-)
>> 
>> fw_cfg.c has no maintainer.  Who would be willing to merge this?
>
>I have various patches for fw_cfg waiting 4.0 to start, one add a 
>MAINTAINERS entry covered by Laszlo, Gerd and myself.

>


Nice, when 4.0 window open? I don't find the release planning.
Maybe my another fw_cfg patches can be merged:
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00004.html
-->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00005.html


Also I've prepared some qtest patch for fw_cfg. 


PS: I'm quite surprise the qemu's version up to 4 quickly(anyway, the 3.1 is just begin)...


Thanks,
Li Qiang


>This series was published way before soft freeze, but I am not sure it 
>applies for hard freeze. If so, it might goes thru Paolo's Misc tree, 

>else we'll take it for 4.0 (via fw_cfg-next).


>
>Regards,
>
>Phil.

Re: [Qemu-devel] [PATCH 0/3] fw_cfg: fix boot bootsplash and reboot-timeout error checking

[Eric Blake]

On 11/20/18 9:31 PM, 李强 wrote:

> 
> Nice, when 4.0 window open? I don't find the release planning.

https://wiki.qemu.org/Planning gives the overview, and 
https://wiki.qemu.org/Planning/3.1 gives the dates we are trying to hit 
for 3.1.

> Maybe my another fw_cfg patches can be merged:
> -->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00004.html
> -->https://lists.gnu.org/archive/html/qemu-devel/2018-11/msg00005.html

Any new features have missed 3.1, especially since -rc2 has already been 
cut.  There is still room for bug fixes in -rc3, but the bar gets 
progressively higher (we are reluctant to take something that might 
destabilize the binaries without adequate time for testing), and while 
we want to avoid an -rc4, past history says we'll probably have one, 
although it will be as limited as possible.

> PS: I'm quite surprise the qemu's version up to 4 quickly(anyway, the 3.1 is just begin)...

Version numbers are somewhat arbitrary.  We recently switched to bumping 
the major version every year (it's arbitrary, after all), so 2019 will 
have 4.0, 4.1, and probably 4.2 (if we stick to a release every 4 
months) before 2020 starting with 5.0.  The change from 3.0 to 3.1 will, 
in reality, be about the same as the change from 3.1 to 4.0.  A better 
way to think of it is that both the old and new naming schemes are both 
arbitrary, but the new naming scheme has the benefit of not resulting in 
large minor numbers.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org