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(-)
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;
}
--
2.39.3
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
© 2016 - 2024 Red Hat, Inc.