[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH 1/2] hw: fw_cfg: refactor fw_cfg_bootsplash()
From: |
Markus Armbruster |
Subject: |
Re: [Qemu-devel] [PATCH 1/2] hw: fw_cfg: refactor fw_cfg_bootsplash() |
Date: |
Fri, 16 Nov 2018 17:33:34 +0100 |
User-agent: |
Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux) |
Li Qiang <address@hidden> 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 <address@hidden>
> ---
> 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 <address@hidden>