diff mbox series

[10/19] smbios: handle errors consistently

Message ID 20240227154749.1818189-11-imammedo@redhat.com
State New
Headers show
Series Workaround Windows failing to find 64bit SMBIOS entry point with SeaBIOS | expand

Commit Message

Igor Mammedov Feb. 27, 2024, 3:47 p.m. UTC 
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(-)

Comments

Ani Sinha March 4, 2024, 11:14 a.m. UTC | #1 
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
>
>
Igor Mammedov March 4, 2024, 1:39 p.m. UTC | #2 
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
> >
> >  
>
Ani Sinha March 4, 2024, 1:53 p.m. UTC | #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 mbox series

Patch

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;
 }