| Message ID | 20240227154749.1818189-11-imammedo@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Workaround Windows failing to find 64bit SMBIOS entry point with SeaBIOS | expand |
On Tue, 27 Feb 2024, Igor Mammedov wrote: > Current code uses mix of error_report()+exit(1) > and error_setg() to handle errors. > Use newer error_setg() everywhere, beside consistency > it will allow to detect error condition without killing > QEMU and attempt switch-over to SMBIOS3.x tables/entrypoint > in follow up patch. > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> Other than the nit below, Reviewed-by: Ani Sinha <anisinha@redhat.com> > --- > include/hw/firmware/smbios.h | 4 ++-- > hw/i386/fw_cfg.c | 3 ++- > hw/smbios/smbios.c | 32 ++++++++++++++++++++------------ > hw/smbios/smbios_legacy.c | 22 ++++++++++++++-------- > 4 files changed, 38 insertions(+), 23 deletions(-) > > diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h > index a6d8fd6591..d1194c9cc2 100644 > --- a/include/hw/firmware/smbios.h > +++ b/include/hw/firmware/smbios.h > @@ -310,7 +310,7 @@ struct smbios_type_127 { > struct smbios_structure_header header; > } QEMU_PACKED; > > -void smbios_validate_table(void); > +bool smbios_validate_table(Error **errp); > void smbios_add_usr_blob_size(size_t size); > void smbios_entry_add(QemuOpts *opts, Error **errp); > void smbios_set_cpuid(uint32_t version, uint32_t features); > @@ -318,7 +318,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product, > const char *version, > bool uuid_encoded, SmbiosEntryPointType ep_type); > void smbios_set_default_processor_family(uint16_t processor_family); > -uint8_t *smbios_get_table_legacy(size_t *length); > +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp); > void smbios_get_tables(MachineState *ms, > const struct smbios_phys_mem_area *mem_array, > const unsigned int mem_array_size, > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c > index d1281066f4..e387bf50d0 100644 > --- a/hw/i386/fw_cfg.c > +++ b/hw/i386/fw_cfg.c > @@ -71,7 +71,8 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg) > smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]); > > if (pcmc->smbios_legacy_mode) { > - smbios_tables = smbios_get_table_legacy(&smbios_tables_len); > + smbios_tables = smbios_get_table_legacy(&smbios_tables_len, > + &error_fatal); > fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, > smbios_tables, smbios_tables_len); > return; > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > index fb1f05fcde..7c28b5f748 100644 > --- a/hw/smbios/smbios.c > +++ b/hw/smbios/smbios.c > @@ -19,7 +19,6 @@ > #include "qemu/units.h" > #include "qapi/error.h" > #include "qemu/config-file.h" > -#include "qemu/error-report.h" > #include "qemu/module.h" > #include "qemu/option.h" > #include "sysemu/sysemu.h" > @@ -448,23 +447,25 @@ opts_init(smbios_register_config); > */ > #define SMBIOS_21_MAX_TABLES_LEN 0xffff > > -static void smbios_check_type4_count(uint32_t expected_t4_count) > +static bool smbios_check_type4_count(uint32_t expected_t4_count, Error **errp) > { > if (smbios_type4_count && smbios_type4_count != expected_t4_count) { > - error_report("Expected %d SMBIOS Type 4 tables, got %d instead", > - expected_t4_count, smbios_type4_count); > - exit(1); > + error_setg(errp, "Expected %d SMBIOS Type 4 tables, got %d instead", > + expected_t4_count, smbios_type4_count); > + return false; > } > + return true; > } > > -void smbios_validate_table(void) > +bool smbios_validate_table(Error **errp) > { > if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 && > smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) { > - error_report("SMBIOS 2.1 table length %zu exceeds %d", > - smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); > - exit(1); > + error_setg(errp, "SMBIOS 2.1 table length %zu exceeds %d", > + smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); > + return false; > } > + return true; > } > > bool smbios_skip_table(uint8_t type, bool required_table) > @@ -1027,15 +1028,18 @@ void smbios_get_tables(MachineState *ms, > smbios_build_type_41_table(errp); > smbios_build_type_127_table(); > > - smbios_check_type4_count(ms->smp.sockets); > - smbios_validate_table(); > + if (!smbios_check_type4_count(ms->smp.sockets, errp)) { nit: I would do a gfree(smbios_tables) here .. > + goto err_exit; > + } > + if (!smbios_validate_table(errp)) { > + goto err_exit; and here. Then in err_exit ... > + } > smbios_entry_point_setup(); > > /* return tables blob and entry point (anchor), and their sizes */ > *tables = smbios_tables; > *tables_len = smbios_tables_len; > *anchor = (uint8_t *)&ep; > - > /* calculate length based on anchor string */ > if (!strncmp((char *)&ep, "_SM_", 4)) { > *anchor_len = sizeof(struct smbios_21_entry_point); > @@ -1044,6 +1048,10 @@ void smbios_get_tables(MachineState *ms, > } else { > abort(); > } > + > + return; > +err_exit: I would add a return here. That way all paths explicitly return void. > + g_free(smbios_tables); No return here? > } > > static void save_opt(const char **dest, QemuOpts *opts, const char *name) > diff --git a/hw/smbios/smbios_legacy.c b/hw/smbios/smbios_legacy.c > index 21f143e738..a6544bf55a 100644 > --- a/hw/smbios/smbios_legacy.c > +++ b/hw/smbios/smbios_legacy.c > @@ -19,7 +19,7 @@ > #include "qemu/bswap.h" > #include "hw/firmware/smbios.h" > #include "sysemu/sysemu.h" > -#include "qemu/error-report.h" > +#include "qapi/error.h" > > struct smbios_header { > uint16_t length; > @@ -128,7 +128,7 @@ static void smbios_build_type_1_fields(void) > } > } > > -uint8_t *smbios_get_table_legacy(size_t *length) > +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp) > { > int i; > size_t usr_offset; > @@ -136,15 +136,15 @@ uint8_t *smbios_get_table_legacy(size_t *length) > /* complain if fields were given for types > 1 */ > if (find_next_bit(smbios_have_fields_bitmap, > SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) { > - error_report("can't process fields for smbios " > + error_setg(errp, "can't process fields for smbios " > "types > 1 on machine versions < 2.1!"); > - exit(1); > + goto err_exit; > } > > if (test_bit(4, smbios_have_binfile_bitmap)) { > - error_report("can't process table for smbios " > - "type 4 on machine versions < 2.1!"); > - exit(1); > + error_setg(errp, "can't process table for smbios " > + "type 4 on machine versions < 2.1!"); > + goto err_exit; > } > > g_free(smbios_entries); > @@ -173,7 +173,13 @@ uint8_t *smbios_get_table_legacy(size_t *length) > > smbios_build_type_0_fields(); > smbios_build_type_1_fields(); > - smbios_validate_table(); > + if (!smbios_validate_table(errp)) { > + goto err_exit; > + } > + > *length = smbios_entries_len; > return smbios_entries; > +err_exit: > + g_free(smbios_entries); > + return NULL; > } > -- > 2.39.3 > >
On Mon, 4 Mar 2024 16:44:34 +0530 (IST) Ani Sinha <anisinha@redhat.com> wrote: > On Tue, 27 Feb 2024, Igor Mammedov wrote: > > > Current code uses mix of error_report()+exit(1) > > and error_setg() to handle errors. > > Use newer error_setg() everywhere, beside consistency > > it will allow to detect error condition without killing > > QEMU and attempt switch-over to SMBIOS3.x tables/entrypoint > > in follow up patch. > > > > Signed-off-by: Igor Mammedov <imammedo@redhat.com> > > Other than the nit below, > > Reviewed-by: Ani Sinha <anisinha@redhat.com> > > > --- > > include/hw/firmware/smbios.h | 4 ++-- > > hw/i386/fw_cfg.c | 3 ++- > > hw/smbios/smbios.c | 32 ++++++++++++++++++++------------ > > hw/smbios/smbios_legacy.c | 22 ++++++++++++++-------- > > 4 files changed, 38 insertions(+), 23 deletions(-) > > > > diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h > > index a6d8fd6591..d1194c9cc2 100644 > > --- a/include/hw/firmware/smbios.h > > +++ b/include/hw/firmware/smbios.h > > @@ -310,7 +310,7 @@ struct smbios_type_127 { > > struct smbios_structure_header header; > > } QEMU_PACKED; > > > > -void smbios_validate_table(void); > > +bool smbios_validate_table(Error **errp); > > void smbios_add_usr_blob_size(size_t size); > > void smbios_entry_add(QemuOpts *opts, Error **errp); > > void smbios_set_cpuid(uint32_t version, uint32_t features); > > @@ -318,7 +318,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product, > > const char *version, > > bool uuid_encoded, SmbiosEntryPointType ep_type); > > void smbios_set_default_processor_family(uint16_t processor_family); > > -uint8_t *smbios_get_table_legacy(size_t *length); > > +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp); > > void smbios_get_tables(MachineState *ms, > > const struct smbios_phys_mem_area *mem_array, > > const unsigned int mem_array_size, > > diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c > > index d1281066f4..e387bf50d0 100644 > > --- a/hw/i386/fw_cfg.c > > +++ b/hw/i386/fw_cfg.c > > @@ -71,7 +71,8 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg) > > smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]); > > > > if (pcmc->smbios_legacy_mode) { > > - smbios_tables = smbios_get_table_legacy(&smbios_tables_len); > > + smbios_tables = smbios_get_table_legacy(&smbios_tables_len, > > + &error_fatal); > > fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, > > smbios_tables, smbios_tables_len); > > return; > > diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c > > index fb1f05fcde..7c28b5f748 100644 > > --- a/hw/smbios/smbios.c > > +++ b/hw/smbios/smbios.c > > @@ -19,7 +19,6 @@ > > #include "qemu/units.h" > > #include "qapi/error.h" > > #include "qemu/config-file.h" > > -#include "qemu/error-report.h" > > #include "qemu/module.h" > > #include "qemu/option.h" > > #include "sysemu/sysemu.h" > > @@ -448,23 +447,25 @@ opts_init(smbios_register_config); > > */ > > #define SMBIOS_21_MAX_TABLES_LEN 0xffff > > > > -static void smbios_check_type4_count(uint32_t expected_t4_count) > > +static bool smbios_check_type4_count(uint32_t expected_t4_count, Error **errp) > > { > > if (smbios_type4_count && smbios_type4_count != expected_t4_count) { > > - error_report("Expected %d SMBIOS Type 4 tables, got %d instead", > > - expected_t4_count, smbios_type4_count); > > - exit(1); > > + error_setg(errp, "Expected %d SMBIOS Type 4 tables, got %d instead", > > + expected_t4_count, smbios_type4_count); > > + return false; > > } > > + return true; > > } > > > > -void smbios_validate_table(void) > > +bool smbios_validate_table(Error **errp) > > { > > if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 && > > smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) { > > - error_report("SMBIOS 2.1 table length %zu exceeds %d", > > - smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); > > - exit(1); > > + error_setg(errp, "SMBIOS 2.1 table length %zu exceeds %d", > > + smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); > > + return false; > > } > > + return true; > > } > > > > bool smbios_skip_table(uint8_t type, bool required_table) > > @@ -1027,15 +1028,18 @@ void smbios_get_tables(MachineState *ms, > > smbios_build_type_41_table(errp); > > smbios_build_type_127_table(); > > > > - smbios_check_type4_count(ms->smp.sockets); > > - smbios_validate_table(); > > + if (!smbios_check_type4_count(ms->smp.sockets, errp)) { > > nit: I would do a gfree(smbios_tables) here .. > > > + goto err_exit; > > + } > > + if (!smbios_validate_table(errp)) { > > + goto err_exit; > > and here. Then in err_exit ... > the whole point of err_exit is to do error patch clean up in one place to avoid duplicate the work. > > + } > > smbios_entry_point_setup(); > > > > /* return tables blob and entry point (anchor), and their sizes */ > > *tables = smbios_tables; > > *tables_len = smbios_tables_len; > > *anchor = (uint8_t *)&ep; > > - > > /* calculate length based on anchor string */ > > if (!strncmp((char *)&ep, "_SM_", 4)) { > > *anchor_len = sizeof(struct smbios_21_entry_point); > > @@ -1044,6 +1048,10 @@ void smbios_get_tables(MachineState *ms, > > } else { > > abort(); > > } > > + > > + return; > > +err_exit: > > I would add a return here. > > That way all paths explicitly return void. > > > + g_free(smbios_tables); > > No return here? technically there is no need for at the end, but I can fix it up for clarity. > > > > } > > > > static void save_opt(const char **dest, QemuOpts *opts, const char *name) > > diff --git a/hw/smbios/smbios_legacy.c b/hw/smbios/smbios_legacy.c > > index 21f143e738..a6544bf55a 100644 > > --- a/hw/smbios/smbios_legacy.c > > +++ b/hw/smbios/smbios_legacy.c > > @@ -19,7 +19,7 @@ > > #include "qemu/bswap.h" > > #include "hw/firmware/smbios.h" > > #include "sysemu/sysemu.h" > > -#include "qemu/error-report.h" > > +#include "qapi/error.h" > > > > struct smbios_header { > > uint16_t length; > > @@ -128,7 +128,7 @@ static void smbios_build_type_1_fields(void) > > } > > } > > > > -uint8_t *smbios_get_table_legacy(size_t *length) > > +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp) > > { > > int i; > > size_t usr_offset; > > @@ -136,15 +136,15 @@ uint8_t *smbios_get_table_legacy(size_t *length) > > /* complain if fields were given for types > 1 */ > > if (find_next_bit(smbios_have_fields_bitmap, > > SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) { > > - error_report("can't process fields for smbios " > > + error_setg(errp, "can't process fields for smbios " > > "types > 1 on machine versions < 2.1!"); > > - exit(1); > > + goto err_exit; > > } > > > > if (test_bit(4, smbios_have_binfile_bitmap)) { > > - error_report("can't process table for smbios " > > - "type 4 on machine versions < 2.1!"); > > - exit(1); > > + error_setg(errp, "can't process table for smbios " > > + "type 4 on machine versions < 2.1!"); > > + goto err_exit; > > } > > > > g_free(smbios_entries); > > @@ -173,7 +173,13 @@ uint8_t *smbios_get_table_legacy(size_t *length) > > > > smbios_build_type_0_fields(); > > smbios_build_type_1_fields(); > > - smbios_validate_table(); > > + if (!smbios_validate_table(errp)) { > > + goto err_exit; > > + } > > + > > *length = smbios_entries_len; > > return smbios_entries; > > +err_exit: > > + g_free(smbios_entries); > > + return NULL; > > } > > -- > > 2.39.3 > > > > >
> On 04-Mar-2024, at 19:09, Igor Mammedov <imammedo@redhat.com> wrote: > > On Mon, 4 Mar 2024 16:44:34 +0530 (IST) > Ani Sinha <anisinha@redhat.com> wrote: > >> On Tue, 27 Feb 2024, Igor Mammedov wrote: >> >>> Current code uses mix of error_report()+exit(1) >>> and error_setg() to handle errors. >>> Use newer error_setg() everywhere, beside consistency >>> it will allow to detect error condition without killing >>> QEMU and attempt switch-over to SMBIOS3.x tables/entrypoint >>> in follow up patch. >>> >>> Signed-off-by: Igor Mammedov <imammedo@redhat.com> >> >> Other than the nit below, >> >> Reviewed-by: Ani Sinha <anisinha@redhat.com> >> >>> --- >>> include/hw/firmware/smbios.h | 4 ++-- >>> hw/i386/fw_cfg.c | 3 ++- >>> hw/smbios/smbios.c | 32 ++++++++++++++++++++------------ >>> hw/smbios/smbios_legacy.c | 22 ++++++++++++++-------- >>> 4 files changed, 38 insertions(+), 23 deletions(-) >>> >>> diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h >>> index a6d8fd6591..d1194c9cc2 100644 >>> --- a/include/hw/firmware/smbios.h >>> +++ b/include/hw/firmware/smbios.h >>> @@ -310,7 +310,7 @@ struct smbios_type_127 { >>> struct smbios_structure_header header; >>> } QEMU_PACKED; >>> >>> -void smbios_validate_table(void); >>> +bool smbios_validate_table(Error **errp); >>> void smbios_add_usr_blob_size(size_t size); >>> void smbios_entry_add(QemuOpts *opts, Error **errp); >>> void smbios_set_cpuid(uint32_t version, uint32_t features); >>> @@ -318,7 +318,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product, >>> const char *version, >>> bool uuid_encoded, SmbiosEntryPointType ep_type); >>> void smbios_set_default_processor_family(uint16_t processor_family); >>> -uint8_t *smbios_get_table_legacy(size_t *length); >>> +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp); >>> void smbios_get_tables(MachineState *ms, >>> const struct smbios_phys_mem_area *mem_array, >>> const unsigned int mem_array_size, >>> diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c >>> index d1281066f4..e387bf50d0 100644 >>> --- a/hw/i386/fw_cfg.c >>> +++ b/hw/i386/fw_cfg.c >>> @@ -71,7 +71,8 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg) >>> smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]); >>> >>> if (pcmc->smbios_legacy_mode) { >>> - smbios_tables = smbios_get_table_legacy(&smbios_tables_len); >>> + smbios_tables = smbios_get_table_legacy(&smbios_tables_len, >>> + &error_fatal); >>> fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, >>> smbios_tables, smbios_tables_len); >>> return; >>> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c >>> index fb1f05fcde..7c28b5f748 100644 >>> --- a/hw/smbios/smbios.c >>> +++ b/hw/smbios/smbios.c >>> @@ -19,7 +19,6 @@ >>> #include "qemu/units.h" >>> #include "qapi/error.h" >>> #include "qemu/config-file.h" >>> -#include "qemu/error-report.h" >>> #include "qemu/module.h" >>> #include "qemu/option.h" >>> #include "sysemu/sysemu.h" >>> @@ -448,23 +447,25 @@ opts_init(smbios_register_config); >>> */ >>> #define SMBIOS_21_MAX_TABLES_LEN 0xffff >>> >>> -static void smbios_check_type4_count(uint32_t expected_t4_count) >>> +static bool smbios_check_type4_count(uint32_t expected_t4_count, Error **errp) >>> { >>> if (smbios_type4_count && smbios_type4_count != expected_t4_count) { >>> - error_report("Expected %d SMBIOS Type 4 tables, got %d instead", >>> - expected_t4_count, smbios_type4_count); >>> - exit(1); >>> + error_setg(errp, "Expected %d SMBIOS Type 4 tables, got %d instead", >>> + expected_t4_count, smbios_type4_count); >>> + return false; >>> } >>> + return true; >>> } >>> >>> -void smbios_validate_table(void) >>> +bool smbios_validate_table(Error **errp) >>> { >>> if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 && >>> smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) { >>> - error_report("SMBIOS 2.1 table length %zu exceeds %d", >>> - smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); >>> - exit(1); >>> + error_setg(errp, "SMBIOS 2.1 table length %zu exceeds %d", >>> + smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); >>> + return false; >>> } >>> + return true; >>> } >>> >>> bool smbios_skip_table(uint8_t type, bool required_table) >>> @@ -1027,15 +1028,18 @@ void smbios_get_tables(MachineState *ms, >>> smbios_build_type_41_table(errp); >>> smbios_build_type_127_table(); >>> >>> - smbios_check_type4_count(ms->smp.sockets); >>> - smbios_validate_table(); >>> + if (!smbios_check_type4_count(ms->smp.sockets, errp)) { >> >> nit: I would do a gfree(smbios_tables) here .. >> >>> + goto err_exit; >>> + } >>> + if (!smbios_validate_table(errp)) { >>> + goto err_exit; >> >> and here. Then in err_exit ... >> > > the whole point of err_exit is to do error patch clean up > in one place to avoid duplicate the work. > >>> + } >>> smbios_entry_point_setup(); >>> >>> /* return tables blob and entry point (anchor), and their sizes */ >>> *tables = smbios_tables; >>> *tables_len = smbios_tables_len; >>> *anchor = (uint8_t *)&ep; >>> - >>> /* calculate length based on anchor string */ >>> if (!strncmp((char *)&ep, "_SM_", 4)) { >>> *anchor_len = sizeof(struct smbios_21_entry_point); >>> @@ -1044,6 +1048,10 @@ void smbios_get_tables(MachineState *ms, >>> } else { >>> abort(); >>> } >>> + >>> + return; >>> +err_exit: >> >> I would add a return here. >> >> That way all paths explicitly return void. >> >>> + g_free(smbios_tables); >> >> No return here? > > technically there is no need for at the end, > but I can fix it up for clarity. I think you do that in patch 11. I suggest you can squash this one with patch 11 and I am happy then. > >> >> >>> } >>> >>> static void save_opt(const char **dest, QemuOpts *opts, const char *name) >>> diff --git a/hw/smbios/smbios_legacy.c b/hw/smbios/smbios_legacy.c >>> index 21f143e738..a6544bf55a 100644 >>> --- a/hw/smbios/smbios_legacy.c >>> +++ b/hw/smbios/smbios_legacy.c >>> @@ -19,7 +19,7 @@ >>> #include "qemu/bswap.h" >>> #include "hw/firmware/smbios.h" >>> #include "sysemu/sysemu.h" >>> -#include "qemu/error-report.h" >>> +#include "qapi/error.h" >>> >>> struct smbios_header { >>> uint16_t length; >>> @@ -128,7 +128,7 @@ static void smbios_build_type_1_fields(void) >>> } >>> } >>> >>> -uint8_t *smbios_get_table_legacy(size_t *length) >>> +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp) >>> { >>> int i; >>> size_t usr_offset; >>> @@ -136,15 +136,15 @@ uint8_t *smbios_get_table_legacy(size_t *length) >>> /* complain if fields were given for types > 1 */ >>> if (find_next_bit(smbios_have_fields_bitmap, >>> SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) { >>> - error_report("can't process fields for smbios " >>> + error_setg(errp, "can't process fields for smbios " >>> "types > 1 on machine versions < 2.1!"); >>> - exit(1); >>> + goto err_exit; >>> } >>> >>> if (test_bit(4, smbios_have_binfile_bitmap)) { >>> - error_report("can't process table for smbios " >>> - "type 4 on machine versions < 2.1!"); >>> - exit(1); >>> + error_setg(errp, "can't process table for smbios " >>> + "type 4 on machine versions < 2.1!"); >>> + goto err_exit; >>> } >>> >>> g_free(smbios_entries); >>> @@ -173,7 +173,13 @@ uint8_t *smbios_get_table_legacy(size_t *length) >>> >>> smbios_build_type_0_fields(); >>> smbios_build_type_1_fields(); >>> - smbios_validate_table(); >>> + if (!smbios_validate_table(errp)) { >>> + goto err_exit; >>> + } >>> + >>> *length = smbios_entries_len; >>> return smbios_entries; >>> +err_exit: >>> + g_free(smbios_entries); >>> + return NULL; >>> } >>> -- >>> 2.39.3
diff --git a/include/hw/firmware/smbios.h b/include/hw/firmware/smbios.h index a6d8fd6591..d1194c9cc2 100644 --- a/include/hw/firmware/smbios.h +++ b/include/hw/firmware/smbios.h @@ -310,7 +310,7 @@ struct smbios_type_127 { struct smbios_structure_header header; } QEMU_PACKED; -void smbios_validate_table(void); +bool smbios_validate_table(Error **errp); void smbios_add_usr_blob_size(size_t size); void smbios_entry_add(QemuOpts *opts, Error **errp); void smbios_set_cpuid(uint32_t version, uint32_t features); @@ -318,7 +318,7 @@ void smbios_set_defaults(const char *manufacturer, const char *product, const char *version, bool uuid_encoded, SmbiosEntryPointType ep_type); void smbios_set_default_processor_family(uint16_t processor_family); -uint8_t *smbios_get_table_legacy(size_t *length); +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp); void smbios_get_tables(MachineState *ms, const struct smbios_phys_mem_area *mem_array, const unsigned int mem_array_size, diff --git a/hw/i386/fw_cfg.c b/hw/i386/fw_cfg.c index d1281066f4..e387bf50d0 100644 --- a/hw/i386/fw_cfg.c +++ b/hw/i386/fw_cfg.c @@ -71,7 +71,8 @@ void fw_cfg_build_smbios(PCMachineState *pcms, FWCfgState *fw_cfg) smbios_set_cpuid(cpu->env.cpuid_version, cpu->env.features[FEAT_1_EDX]); if (pcmc->smbios_legacy_mode) { - smbios_tables = smbios_get_table_legacy(&smbios_tables_len); + smbios_tables = smbios_get_table_legacy(&smbios_tables_len, + &error_fatal); fw_cfg_add_bytes(fw_cfg, FW_CFG_SMBIOS_ENTRIES, smbios_tables, smbios_tables_len); return; diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c index fb1f05fcde..7c28b5f748 100644 --- a/hw/smbios/smbios.c +++ b/hw/smbios/smbios.c @@ -19,7 +19,6 @@ #include "qemu/units.h" #include "qapi/error.h" #include "qemu/config-file.h" -#include "qemu/error-report.h" #include "qemu/module.h" #include "qemu/option.h" #include "sysemu/sysemu.h" @@ -448,23 +447,25 @@ opts_init(smbios_register_config); */ #define SMBIOS_21_MAX_TABLES_LEN 0xffff -static void smbios_check_type4_count(uint32_t expected_t4_count) +static bool smbios_check_type4_count(uint32_t expected_t4_count, Error **errp) { if (smbios_type4_count && smbios_type4_count != expected_t4_count) { - error_report("Expected %d SMBIOS Type 4 tables, got %d instead", - expected_t4_count, smbios_type4_count); - exit(1); + error_setg(errp, "Expected %d SMBIOS Type 4 tables, got %d instead", + expected_t4_count, smbios_type4_count); + return false; } + return true; } -void smbios_validate_table(void) +bool smbios_validate_table(Error **errp) { if (smbios_ep_type == SMBIOS_ENTRY_POINT_TYPE_32 && smbios_tables_len > SMBIOS_21_MAX_TABLES_LEN) { - error_report("SMBIOS 2.1 table length %zu exceeds %d", - smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); - exit(1); + error_setg(errp, "SMBIOS 2.1 table length %zu exceeds %d", + smbios_tables_len, SMBIOS_21_MAX_TABLES_LEN); + return false; } + return true; } bool smbios_skip_table(uint8_t type, bool required_table) @@ -1027,15 +1028,18 @@ void smbios_get_tables(MachineState *ms, smbios_build_type_41_table(errp); smbios_build_type_127_table(); - smbios_check_type4_count(ms->smp.sockets); - smbios_validate_table(); + if (!smbios_check_type4_count(ms->smp.sockets, errp)) { + goto err_exit; + } + if (!smbios_validate_table(errp)) { + goto err_exit; + } smbios_entry_point_setup(); /* return tables blob and entry point (anchor), and their sizes */ *tables = smbios_tables; *tables_len = smbios_tables_len; *anchor = (uint8_t *)&ep; - /* calculate length based on anchor string */ if (!strncmp((char *)&ep, "_SM_", 4)) { *anchor_len = sizeof(struct smbios_21_entry_point); @@ -1044,6 +1048,10 @@ void smbios_get_tables(MachineState *ms, } else { abort(); } + + return; +err_exit: + g_free(smbios_tables); } static void save_opt(const char **dest, QemuOpts *opts, const char *name) diff --git a/hw/smbios/smbios_legacy.c b/hw/smbios/smbios_legacy.c index 21f143e738..a6544bf55a 100644 --- a/hw/smbios/smbios_legacy.c +++ b/hw/smbios/smbios_legacy.c @@ -19,7 +19,7 @@ #include "qemu/bswap.h" #include "hw/firmware/smbios.h" #include "sysemu/sysemu.h" -#include "qemu/error-report.h" +#include "qapi/error.h" struct smbios_header { uint16_t length; @@ -128,7 +128,7 @@ static void smbios_build_type_1_fields(void) } } -uint8_t *smbios_get_table_legacy(size_t *length) +uint8_t *smbios_get_table_legacy(size_t *length, Error **errp) { int i; size_t usr_offset; @@ -136,15 +136,15 @@ uint8_t *smbios_get_table_legacy(size_t *length) /* complain if fields were given for types > 1 */ if (find_next_bit(smbios_have_fields_bitmap, SMBIOS_MAX_TYPE + 1, 2) < SMBIOS_MAX_TYPE + 1) { - error_report("can't process fields for smbios " + error_setg(errp, "can't process fields for smbios " "types > 1 on machine versions < 2.1!"); - exit(1); + goto err_exit; } if (test_bit(4, smbios_have_binfile_bitmap)) { - error_report("can't process table for smbios " - "type 4 on machine versions < 2.1!"); - exit(1); + error_setg(errp, "can't process table for smbios " + "type 4 on machine versions < 2.1!"); + goto err_exit; } g_free(smbios_entries); @@ -173,7 +173,13 @@ uint8_t *smbios_get_table_legacy(size_t *length) smbios_build_type_0_fields(); smbios_build_type_1_fields(); - smbios_validate_table(); + if (!smbios_validate_table(errp)) { + goto err_exit; + } + *length = smbios_entries_len; return smbios_entries; +err_exit: + g_free(smbios_entries); + return NULL; }
Current code uses mix of error_report()+exit(1) and error_setg() to handle errors. Use newer error_setg() everywhere, beside consistency it will allow to detect error condition without killing QEMU and attempt switch-over to SMBIOS3.x tables/entrypoint in follow up patch. Signed-off-by: Igor Mammedov <imammedo@redhat.com> --- include/hw/firmware/smbios.h | 4 ++-- hw/i386/fw_cfg.c | 3 ++- hw/smbios/smbios.c | 32 ++++++++++++++++++++------------ hw/smbios/smbios_legacy.c | 22 ++++++++++++++-------- 4 files changed, 38 insertions(+), 23 deletions(-)