| Message ID | 20181110034115.103335-2-liq3ea@163.com |
|---|---|
| State | New |
| Headers | show |
| Series | refactor fw_cfg_bootsplash() and fw_cfg_reboot() | expand |
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>
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>
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,
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(-)