Calling error_report() in a function that takes an Error ** argument
is suspicious. smbios_entry_add() does that, and then exit()s. It
also passes &error_fatal to qemu_opts_validate(). Both wrong, but
currently harmless, as its only caller passes &error_fatal. Messed up
in commit 1007a37e208. Clean it up.
Cc: Paolo Bonzini <pbonzini@redhat.com>
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
hw/smbios/smbios.c | 90 +++++++++++++++++++++++++++++++---------------
1 file changed, 62 insertions(+), 28 deletions(-)
diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
index a27e54b2fa..920939454e 100644
--- a/hw/smbios/smbios.c
+++ b/hw/smbios/smbios.c
@@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest,
void smbios_entry_add(QemuOpts *opts, Error **errp)
{
+ Error *err = NULL;
const char *val;
assert(!smbios_immutable);
@@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
int size;
struct smbios_table *table; /* legacy mode only */
- qemu_opts_validate(opts, qemu_smbios_file_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_file_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
size = get_image_size(val);
if (size == -1 || size < sizeof(struct smbios_structure_header)) {
- error_report("Cannot read SMBIOS file %s", val);
- exit(1);
+ error_setg(errp, "Cannot read SMBIOS file %s", val);
+ return;
}
/*
@@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
smbios_tables_len);
if (load_image(val, (uint8_t *)header) != size) {
- error_report("Failed to load SMBIOS file %s", val);
- exit(1);
+ error_setg(errp, "Failed to load SMBIOS file %s", val);
+ return;
}
if (test_bit(header->type, have_fields_bitmap)) {
- error_report("can't load type %d struct, fields already specified!",
- header->type);
- exit(1);
+ error_setg(errp,
+ "can't load type %d struct, fields already specified!",
+ header->type);
+ return;
}
set_bit(header->type, have_binfile_bitmap);
@@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
unsigned long type = strtoul(val, NULL, 0);
if (type > SMBIOS_MAX_TYPE) {
- error_report("out of range!");
- exit(1);
+ error_setg(errp, "out of range!");
+ return;
}
if (test_bit(type, have_binfile_bitmap)) {
- error_report("can't add fields, binary file already loaded!");
- exit(1);
+ error_setg(errp, "can't add fields, binary file already loaded!");
+ return;
}
set_bit(type, have_fields_bitmap);
switch (type) {
case 0:
- qemu_opts_validate(opts, qemu_smbios_type0_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type0_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type0.vendor, opts, "vendor");
save_opt(&type0.version, opts, "version");
save_opt(&type0.date, opts, "date");
@@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
val = qemu_opt_get(opts, "release");
if (val) {
if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
- error_report("Invalid release");
- exit(1);
+ error_setg(errp, "Invalid release");
+ return;
}
type0.have_major_minor = true;
}
return;
case 1:
- qemu_opts_validate(opts, qemu_smbios_type1_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type1_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type1.manufacturer, opts, "manufacturer");
save_opt(&type1.product, opts, "product");
save_opt(&type1.version, opts, "version");
@@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
val = qemu_opt_get(opts, "uuid");
if (val) {
if (qemu_uuid_parse(val, &qemu_uuid) != 0) {
- error_report("Invalid UUID");
- exit(1);
+ error_setg(errp, "Invalid UUID");
+ return;
}
qemu_uuid_set = true;
}
return;
case 2:
- qemu_opts_validate(opts, qemu_smbios_type2_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type2_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type2.manufacturer, opts, "manufacturer");
save_opt(&type2.product, opts, "product");
save_opt(&type2.version, opts, "version");
@@ -1085,7 +1103,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(&type2.location, opts, "location");
return;
case 3:
- qemu_opts_validate(opts, qemu_smbios_type3_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type3_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type3.manufacturer, opts, "manufacturer");
save_opt(&type3.version, opts, "version");
save_opt(&type3.serial, opts, "serial");
@@ -1093,7 +1115,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(&type3.sku, opts, "sku");
return;
case 4:
- qemu_opts_validate(opts, qemu_smbios_type4_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type4_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type4.sock_pfx, opts, "sock_pfx");
save_opt(&type4.manufacturer, opts, "manufacturer");
save_opt(&type4.version, opts, "version");
@@ -1102,11 +1128,19 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
save_opt(&type4.part, opts, "part");
return;
case 11:
- qemu_opts_validate(opts, qemu_smbios_type11_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt_list(&type11.nvalues, &type11.values, opts, "value");
return;
case 17:
- qemu_opts_validate(opts, qemu_smbios_type17_opts, &error_fatal);
+ qemu_opts_validate(opts, qemu_smbios_type17_opts, &err);
+ if (err) {
+ error_propagate(errp, err);
+ return;
+ }
save_opt(&type17.loc_pfx, opts, "loc_pfx");
save_opt(&type17.bank, opts, "bank");
save_opt(&type17.manufacturer, opts, "manufacturer");
@@ -1116,12 +1150,12 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
type17.speed = qemu_opt_get_number(opts, "speed", 0);
return;
default:
- error_report("Don't know how to build fields for SMBIOS type %ld",
- type);
- exit(1);
+ error_setg(errp,
+ "Don't know how to build fields for SMBIOS type %ld",
+ type);
+ return;
}
}
- error_report("Must specify type= or file=");
- exit(1);
+ error_setg(errp, "Must specify type= or file=");
}
--
2.17.1
Hi
On Mon, Oct 8, 2018 at 9:47 PM Markus Armbruster <armbru@redhat.com> wrote:
>
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. smbios_entry_add() does that, and then exit()s. It
> also passes &error_fatal to qemu_opts_validate(). Both wrong, but
> currently harmless, as its only caller passes &error_fatal. Messed up
> in commit 1007a37e208. Clean it up.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>
> ---
> hw/smbios/smbios.c | 90 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 62 insertions(+), 28 deletions(-)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index a27e54b2fa..920939454e 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest,
>
> void smbios_entry_add(QemuOpts *opts, Error **errp)
> {
> + Error *err = NULL;
> const char *val;
>
> assert(!smbios_immutable);
> @@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> int size;
> struct smbios_table *table; /* legacy mode only */
>
> - qemu_opts_validate(opts, qemu_smbios_file_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_file_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
>
> size = get_image_size(val);
> if (size == -1 || size < sizeof(struct smbios_structure_header)) {
> - error_report("Cannot read SMBIOS file %s", val);
> - exit(1);
> + error_setg(errp, "Cannot read SMBIOS file %s", val);
> + return;
> }
>
> /*
> @@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> smbios_tables_len);
>
> if (load_image(val, (uint8_t *)header) != size) {
> - error_report("Failed to load SMBIOS file %s", val);
> - exit(1);
> + error_setg(errp, "Failed to load SMBIOS file %s", val);
> + return;
> }
>
> if (test_bit(header->type, have_fields_bitmap)) {
> - error_report("can't load type %d struct, fields already specified!",
> - header->type);
> - exit(1);
> + error_setg(errp,
> + "can't load type %d struct, fields already specified!",
> + header->type);
> + return;
> }
> set_bit(header->type, have_binfile_bitmap);
>
> @@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> unsigned long type = strtoul(val, NULL, 0);
>
> if (type > SMBIOS_MAX_TYPE) {
> - error_report("out of range!");
> - exit(1);
> + error_setg(errp, "out of range!");
> + return;
> }
>
> if (test_bit(type, have_binfile_bitmap)) {
> - error_report("can't add fields, binary file already loaded!");
> - exit(1);
> + error_setg(errp, "can't add fields, binary file already loaded!");
> + return;
> }
> set_bit(type, have_fields_bitmap);
>
> switch (type) {
> case 0:
> - qemu_opts_validate(opts, qemu_smbios_type0_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type0_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type0.vendor, opts, "vendor");
> save_opt(&type0.version, opts, "version");
> save_opt(&type0.date, opts, "date");
> @@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> val = qemu_opt_get(opts, "release");
> if (val) {
> if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
> - error_report("Invalid release");
> - exit(1);
> + error_setg(errp, "Invalid release");
> + return;
> }
> type0.have_major_minor = true;
> }
> return;
> case 1:
> - qemu_opts_validate(opts, qemu_smbios_type1_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type1_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type1.manufacturer, opts, "manufacturer");
> save_opt(&type1.product, opts, "product");
> save_opt(&type1.version, opts, "version");
> @@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> val = qemu_opt_get(opts, "uuid");
> if (val) {
> if (qemu_uuid_parse(val, &qemu_uuid) != 0) {
> - error_report("Invalid UUID");
> - exit(1);
> + error_setg(errp, "Invalid UUID");
> + return;
> }
> qemu_uuid_set = true;
> }
> return;
> case 2:
> - qemu_opts_validate(opts, qemu_smbios_type2_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type2_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type2.manufacturer, opts, "manufacturer");
> save_opt(&type2.product, opts, "product");
> save_opt(&type2.version, opts, "version");
> @@ -1085,7 +1103,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> save_opt(&type2.location, opts, "location");
> return;
> case 3:
> - qemu_opts_validate(opts, qemu_smbios_type3_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type3_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type3.manufacturer, opts, "manufacturer");
> save_opt(&type3.version, opts, "version");
> save_opt(&type3.serial, opts, "serial");
> @@ -1093,7 +1115,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> save_opt(&type3.sku, opts, "sku");
> return;
> case 4:
> - qemu_opts_validate(opts, qemu_smbios_type4_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type4_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type4.sock_pfx, opts, "sock_pfx");
> save_opt(&type4.manufacturer, opts, "manufacturer");
> save_opt(&type4.version, opts, "version");
> @@ -1102,11 +1128,19 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> save_opt(&type4.part, opts, "part");
> return;
> case 11:
> - qemu_opts_validate(opts, qemu_smbios_type11_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt_list(&type11.nvalues, &type11.values, opts, "value");
> return;
> case 17:
> - qemu_opts_validate(opts, qemu_smbios_type17_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type17_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type17.loc_pfx, opts, "loc_pfx");
> save_opt(&type17.bank, opts, "bank");
> save_opt(&type17.manufacturer, opts, "manufacturer");
> @@ -1116,12 +1150,12 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> type17.speed = qemu_opt_get_number(opts, "speed", 0);
> return;
> default:
> - error_report("Don't know how to build fields for SMBIOS type %ld",
> - type);
> - exit(1);
> + error_setg(errp,
> + "Don't know how to build fields for SMBIOS type %ld",
> + type);
> + return;
> }
> }
>
> - error_report("Must specify type= or file=");
> - exit(1);
> + error_setg(errp, "Must specify type= or file=");
> }
> --
> 2.17.1
>
>
--
Marc-André Lureau
On 08/10/2018 19:31, Markus Armbruster wrote:
> Calling error_report() in a function that takes an Error ** argument
> is suspicious. smbios_entry_add() does that, and then exit()s. It
> also passes &error_fatal to qemu_opts_validate(). Both wrong, but
> currently harmless, as its only caller passes &error_fatal. Messed up
> in commit 1007a37e208. Clean it up.
>
> Cc: Paolo Bonzini <pbonzini@redhat.com>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> hw/smbios/smbios.c | 90 +++++++++++++++++++++++++++++++---------------
> 1 file changed, 62 insertions(+), 28 deletions(-)
>
> diff --git a/hw/smbios/smbios.c b/hw/smbios/smbios.c
> index a27e54b2fa..920939454e 100644
> --- a/hw/smbios/smbios.c
> +++ b/hw/smbios/smbios.c
> @@ -950,6 +950,7 @@ static void save_opt_list(size_t *ndest, const char ***dest,
>
> void smbios_entry_add(QemuOpts *opts, Error **errp)
> {
> + Error *err = NULL;
> const char *val;
>
> assert(!smbios_immutable);
> @@ -960,12 +961,16 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> int size;
> struct smbios_table *table; /* legacy mode only */
>
> - qemu_opts_validate(opts, qemu_smbios_file_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_file_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
>
> size = get_image_size(val);
> if (size == -1 || size < sizeof(struct smbios_structure_header)) {
> - error_report("Cannot read SMBIOS file %s", val);
> - exit(1);
> + error_setg(errp, "Cannot read SMBIOS file %s", val);
> + return;
> }
>
> /*
> @@ -978,14 +983,15 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> smbios_tables_len);
>
> if (load_image(val, (uint8_t *)header) != size) {
> - error_report("Failed to load SMBIOS file %s", val);
> - exit(1);
> + error_setg(errp, "Failed to load SMBIOS file %s", val);
> + return;
> }
>
> if (test_bit(header->type, have_fields_bitmap)) {
> - error_report("can't load type %d struct, fields already specified!",
> - header->type);
> - exit(1);
> + error_setg(errp,
> + "can't load type %d struct, fields already specified!",
> + header->type);
> + return;
> }
> set_bit(header->type, have_binfile_bitmap);
>
> @@ -1030,19 +1036,23 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> unsigned long type = strtoul(val, NULL, 0);
>
> if (type > SMBIOS_MAX_TYPE) {
> - error_report("out of range!");
> - exit(1);
> + error_setg(errp, "out of range!");
> + return;
> }
>
> if (test_bit(type, have_binfile_bitmap)) {
> - error_report("can't add fields, binary file already loaded!");
> - exit(1);
> + error_setg(errp, "can't add fields, binary file already loaded!");
> + return;
> }
> set_bit(type, have_fields_bitmap);
>
> switch (type) {
> case 0:
> - qemu_opts_validate(opts, qemu_smbios_type0_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type0_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type0.vendor, opts, "vendor");
> save_opt(&type0.version, opts, "version");
> save_opt(&type0.date, opts, "date");
> @@ -1051,14 +1061,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> val = qemu_opt_get(opts, "release");
> if (val) {
> if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {
> - error_report("Invalid release");
> - exit(1);
> + error_setg(errp, "Invalid release");
> + return;
> }
> type0.have_major_minor = true;
> }
> return;
> case 1:
> - qemu_opts_validate(opts, qemu_smbios_type1_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type1_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type1.manufacturer, opts, "manufacturer");
> save_opt(&type1.product, opts, "product");
> save_opt(&type1.version, opts, "version");
> @@ -1069,14 +1083,18 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> val = qemu_opt_get(opts, "uuid");
> if (val) {
> if (qemu_uuid_parse(val, &qemu_uuid) != 0) {
> - error_report("Invalid UUID");
> - exit(1);
> + error_setg(errp, "Invalid UUID");
> + return;
> }
> qemu_uuid_set = true;
> }
> return;
> case 2:
> - qemu_opts_validate(opts, qemu_smbios_type2_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type2_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type2.manufacturer, opts, "manufacturer");
> save_opt(&type2.product, opts, "product");
> save_opt(&type2.version, opts, "version");
> @@ -1085,7 +1103,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> save_opt(&type2.location, opts, "location");
> return;
> case 3:
> - qemu_opts_validate(opts, qemu_smbios_type3_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type3_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type3.manufacturer, opts, "manufacturer");
> save_opt(&type3.version, opts, "version");
> save_opt(&type3.serial, opts, "serial");
> @@ -1093,7 +1115,11 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> save_opt(&type3.sku, opts, "sku");
> return;
> case 4:
> - qemu_opts_validate(opts, qemu_smbios_type4_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type4_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type4.sock_pfx, opts, "sock_pfx");
> save_opt(&type4.manufacturer, opts, "manufacturer");
> save_opt(&type4.version, opts, "version");
> @@ -1102,11 +1128,19 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> save_opt(&type4.part, opts, "part");
> return;
> case 11:
> - qemu_opts_validate(opts, qemu_smbios_type11_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type11_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt_list(&type11.nvalues, &type11.values, opts, "value");
> return;
> case 17:
> - qemu_opts_validate(opts, qemu_smbios_type17_opts, &error_fatal);
> + qemu_opts_validate(opts, qemu_smbios_type17_opts, &err);
> + if (err) {
> + error_propagate(errp, err);
> + return;
> + }
> save_opt(&type17.loc_pfx, opts, "loc_pfx");
> save_opt(&type17.bank, opts, "bank");
> save_opt(&type17.manufacturer, opts, "manufacturer");
> @@ -1116,12 +1150,12 @@ void smbios_entry_add(QemuOpts *opts, Error **errp)
> type17.speed = qemu_opt_get_number(opts, "speed", 0);
> return;
> default:
> - error_report("Don't know how to build fields for SMBIOS type %ld",
> - type);
> - exit(1);
> + error_setg(errp,
> + "Don't know how to build fields for SMBIOS type %ld",
> + type);
> + return;
> }
> }
>
> - error_report("Must specify type= or file=");
> - exit(1);
> + error_setg(errp, "Must specify type= or file=");
> }
>
Acked-by: Paolo Bonzini <pbonzini@redhat.com>
© 2016 - 2025 Red Hat, Inc.