From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:53530) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQ1O6-0008I3-Cr for qemu-devel@nongnu.org; Sat, 28 Sep 2013 16:45:20 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1VQ1O0-0007mT-AM for qemu-devel@nongnu.org; Sat, 28 Sep 2013 16:45:14 -0400 Received: from mx1.redhat.com ([209.132.183.28]:10450) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1VQ1O0-0007lX-0p for qemu-devel@nongnu.org; Sat, 28 Sep 2013 16:45:08 -0400 Date: Sat, 28 Sep 2013 23:47:24 +0300 From: "Michael S. Tsirkin" Message-ID: <20130928204724.GA10454@redhat.com> References: <1376659114-6630-1-git-send-email-armbru@redhat.com> <1376659114-6630-3-git-send-email-armbru@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: <1376659114-6630-3-git-send-email-armbru@redhat.com> Content-Transfer-Encoding: quoted-printable Subject: Re: [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: armbru@redhat.com Cc: lersek@redhat.com, qemu-devel@nongnu.org, Anthony Liguori , ehabkost@redhat.com On Fri, Aug 16, 2013 at 03:18:29PM +0200, armbru@redhat.com wrote: > From: Markus Armbruster >=20 > So that it can be set in config file for -readconfig. >=20 > This tightens parsing of -smbios, and makes it more consistent with > other options: unknown parameters are rejected, numbers with trailing > junk are rejected, when a parameter is given multiple times, last > rather than first wins, ... >=20 > Signed-off-by: Markus Armbruster > Reviewed-by: Eric Blake > --- > arch_init.c | 4 +- > hw/i386/smbios.c | 211 ++++++++++++++++++++++++++++++++++++-= -------- > include/hw/i386/smbios.h | 4 +- > include/sysemu/arch_init.h | 2 +- > vl.c | 3 +- > 5 files changed, 180 insertions(+), 44 deletions(-) >=20 > diff --git a/arch_init.c b/arch_init.c > index 1ddead0..96477fd 100644 > --- a/arch_init.c > +++ b/arch_init.c > @@ -1133,10 +1133,10 @@ void do_acpitable_option(const QemuOpts *opts) > #endif > } > =20 > -void do_smbios_option(const char *optarg) > +void do_smbios_option(QemuOpts *opts) > { > #ifdef TARGET_I386 > - smbios_entry_add(optarg); > + smbios_entry_add(opts); > #endif > } > =20 > diff --git a/hw/i386/smbios.c b/hw/i386/smbios.c > index 0608aee..a113f8b 100644 > --- a/hw/i386/smbios.c > +++ b/hw/i386/smbios.c > @@ -2,9 +2,11 @@ > * SMBIOS Support > * > * Copyright (C) 2009 Hewlett-Packard Development Company, L.P. > + * Copyright (C) 2013 Red Hat, Inc. > * > * Authors: > * Alex Williamson > + * Markus Armbruster > * > * This work is licensed under the terms of the GNU GPL, version 2. S= ee > * the COPYING file in the top-level directory. > @@ -13,6 +15,7 @@ > * GNU GPL, version 2 or (at your option) any later version. > */ > =20 > +#include "qemu/config-file.h" > #include "qemu/error-report.h" > #include "sysemu/sysemu.h" > #include "hw/i386/smbios.h" > @@ -41,11 +44,100 @@ struct smbios_table { > #define SMBIOS_FIELD_ENTRY 0 > #define SMBIOS_TABLE_ENTRY 1 > =20 > - > static uint8_t *smbios_entries; > static size_t smbios_entries_len; > static int smbios_type4_count =3D 0; > =20 > +static QemuOptsList qemu_smbios_opts =3D { > + .name =3D "smbios", > + .head =3D QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head), > + .desc =3D { > + /* > + * no elements =3D> accept any params > + * validation will happen later > + */ > + { /* end of list */ } > + } > +}; > + > +static const QemuOptDesc qemu_smbios_file_opts[] =3D { > + { > + .name =3D "file", > + .type =3D QEMU_OPT_STRING, > + .help =3D "binary file containing an SMBIOS element", > + }, > + { /* end of list */ } > +}; > + > +static const QemuOptDesc qemu_smbios_type0_opts[] =3D { > + { > + .name =3D "type", > + .type =3D QEMU_OPT_NUMBER, > + .help =3D "SMBIOS element type", > + },{ > + .name =3D "vendor", > + .type =3D QEMU_OPT_STRING, > + .help =3D "vendor name", > + },{ > + .name =3D "version", > + .type =3D QEMU_OPT_STRING, > + .help =3D "version number", > + },{ > + .name =3D "date", > + .type =3D QEMU_OPT_STRING, > + .help =3D "release date", > + },{ > + .name =3D "release", > + .type =3D QEMU_OPT_STRING, > + .help =3D "revision number", > + }, > + { /* end of list */ } > +}; > + > +static const QemuOptDesc qemu_smbios_type1_opts[] =3D { > + { > + .name =3D "type", > + .type =3D QEMU_OPT_NUMBER, > + .help =3D "SMBIOS element type", > + },{ > + .name =3D "manufacturer", > + .type =3D QEMU_OPT_STRING, > + .help =3D "manufacturer name", > + },{ > + .name =3D "product", > + .type =3D QEMU_OPT_STRING, > + .help =3D "product name", > + },{ > + .name =3D "version", > + .type =3D QEMU_OPT_STRING, > + .help =3D "version number", > + },{ > + .name =3D "serial", > + .type =3D QEMU_OPT_STRING, > + .help =3D "serial number", > + },{ > + .name =3D "uuid", > + .type =3D QEMU_OPT_STRING, > + .help =3D "UUID", > + },{ > + .name =3D "sku", > + .type =3D QEMU_OPT_STRING, > + .help =3D "SKU number", > + },{ > + .name =3D "family", > + .type =3D QEMU_OPT_STRING, > + .help =3D "family name", > + }, > + { /* end of list */ } > +}; > + > +static void smbios_register_config(void) > +{ > + qemu_add_opts(&qemu_smbios_opts); > +} > + > +machine_init(smbios_register_config); > + > static void smbios_validate_table(void) > { > if (smbios_type4_count && smbios_type4_count !=3D smp_cpus) { > @@ -124,23 +216,30 @@ void smbios_add_field(int type, int offset, const= void *data, size_t len) > cpu_to_le16(le16_to_cpu(*(uint16_t *)smbios_entries) + 1); > } > =20 > -static void smbios_build_type_0_fields(const char *t) > +static void smbios_build_type_0_fields(QemuOpts *opts) > { > - char buf[1024]; > + const char *val; > unsigned char major, minor; > =20 > - if (get_param_value(buf, sizeof(buf), "vendor", t)) > + val =3D qemu_opt_get(opts, "vendor"); > + if (val) { > smbios_add_field(0, offsetof(struct smbios_type_0, vendor_str)= , > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "version", t)) > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "version"); > + if (val) { > smbios_add_field(0, offsetof(struct smbios_type_0, bios_versio= n_str), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "date", t)) > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "date"); > + if (val) { > smbios_add_field(0, offsetof(struct smbios_type_0, > bios_release_date_str), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "release", t)) { > - if (sscanf(buf, "%hhu.%hhu", &major, &minor) !=3D 2) { > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "release"); > + if (val) { > + if (sscanf(val, "%hhu.%hhu", &major, &minor) !=3D 2) { > error_report("Invalid release"); > exit(1); > } > @@ -153,47 +252,69 @@ static void smbios_build_type_0_fields(const char= *t) > } > } > =20 > -static void smbios_build_type_1_fields(const char *t) > +static void smbios_build_type_1_fields(QemuOpts *opts) > { > - char buf[1024]; > + const char *val; > =20 > - if (get_param_value(buf, sizeof(buf), "manufacturer", t)) > + val =3D qemu_opt_get(opts, "manufacturer"); > + if (val) { > smbios_add_field(1, offsetof(struct smbios_type_1, manufacture= r_str), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "product", t)) > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "product"); > + if (val) { > smbios_add_field(1, offsetof(struct smbios_type_1, product_nam= e_str), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "version", t)) > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "version"); > + if (val) { > smbios_add_field(1, offsetof(struct smbios_type_1, version_str= ), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "serial", t)) > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "serial"); > + if (val) { > smbios_add_field(1, offsetof(struct smbios_type_1, serial_numb= er_str), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "uuid", t)) { > - if (qemu_uuid_parse(buf, qemu_uuid) !=3D 0) { > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "uuid"); > + if (val) { > + if (qemu_uuid_parse(val, qemu_uuid) !=3D 0) { > error_report("Invalid UUID"); > exit(1); > } > } > - if (get_param_value(buf, sizeof(buf), "sku", t)) > + val =3D qemu_opt_get(opts, "sku"); > + if (val) { > smbios_add_field(1, offsetof(struct smbios_type_1, sku_number_= str), > - buf, strlen(buf) + 1); > - if (get_param_value(buf, sizeof(buf), "family", t)) > + val, strlen(val) + 1); > + } > + val =3D qemu_opt_get(opts, "family"); > + if (val) { > smbios_add_field(1, offsetof(struct smbios_type_1, family_str)= , > - buf, strlen(buf) + 1); > + val, strlen(val) + 1); > + } > } > =20 > -void smbios_entry_add(const char *t) > +void smbios_entry_add(QemuOpts *opts) > { > - char buf[1024]; > + Error *local_err =3D NULL; > + const char *val; > =20 > - if (get_param_value(buf, sizeof(buf), "file", t)) { > + val =3D qemu_opt_get(opts, "file"); > + if (val) { > struct smbios_structure_header *header; > struct smbios_table *table; > - int size =3D get_image_size(buf); > + int size; > + > + qemu_opts_validate(opts, qemu_smbios_file_opts, &local_err); > + if (local_err) { > + error_report("%s", error_get_pretty(local_err)); > + exit(1); > + } > =20 > + size =3D get_image_size(val); > if (size =3D=3D -1 || size < sizeof(struct smbios_structure_he= ader)) { > - error_report("Cannot read SMBIOS file %s", buf); > + error_report("Cannot read SMBIOS file %s", val); > exit(1); > } > =20 > @@ -208,8 +329,8 @@ void smbios_entry_add(const char *t) > table->header.type =3D SMBIOS_TABLE_ENTRY; > table->header.length =3D cpu_to_le16(sizeof(*table) + size); > =20 > - if (load_image(buf, table->data) !=3D size) { > - error_report("Failed to load SMBIOS file %s", buf); > + if (load_image(val, table->data) !=3D size) { > + error_report("Failed to load SMBIOS file %s", val); > exit(1); > } > =20 > @@ -225,17 +346,29 @@ void smbios_entry_add(const char *t) > return; > } > =20 > - if (get_param_value(buf, sizeof(buf), "type", t)) { > - unsigned long type =3D strtoul(buf, NULL, 0); > + val =3D qemu_opt_get(opts, "type"); > + if (val) { > + unsigned long type =3D strtoul(val, NULL, 0); > + > switch (type) { > case 0: > - smbios_build_type_0_fields(t); > + qemu_opts_validate(opts, qemu_smbios_type0_opts, &local_er= r); > + if (local_err) { > + error_report("%s", error_get_pretty(local_err)); > + exit(1); > + } > + smbios_build_type_0_fields(opts); > return; > case 1: > - smbios_build_type_1_fields(t); > + qemu_opts_validate(opts, qemu_smbios_type1_opts, &local_er= r); > + if (local_err) { > + error_report("%s", error_get_pretty(local_err)); > + exit(1); > + } > + smbios_build_type_1_fields(opts); > return; > default: > - error_report("Don't know how to build fields for SMBIOS ty= pe %ld", > + error_report("Don't know how to build fields for SMBIOS ty= pe %" PRIu64, > type); > exit(1); > } This triggers a build failure: /scm/qemu/hw/i386/smbios.c: In function =E2=80=98smbios_entry_add=E2=80=99= : /scm/qemu/hw/i386/smbios.c:382:26: error: format =E2=80=98%llu=E2=80=99 e= xpects argument of type =E2=80=98long long unsigned int=E2=80=99, but argument 2 has type= =E2=80=98long unsigned int=E2=80=99 [-Werror=3Dformat=3D] type); ^ It's a long value, why are you printing it with PRIu64? %ld seems right. I reverted just this chunk. > diff --git a/include/hw/i386/smbios.h b/include/hw/i386/smbios.h > index 56c6108..d9f43b7 100644 > --- a/include/hw/i386/smbios.h > +++ b/include/hw/i386/smbios.h > @@ -13,7 +13,9 @@ > * > */ > =20 > -void smbios_entry_add(const char *t); > +#include "qemu/option.h" > + > +void smbios_entry_add(QemuOpts *opts); > void smbios_add_field(int type, int offset, const void *data, size_t l= en); > uint8_t *smbios_get_table(size_t *length); > =20 > diff --git a/include/sysemu/arch_init.h b/include/sysemu/arch_init.h > index dece913..be71bca 100644 > --- a/include/sysemu/arch_init.h > +++ b/include/sysemu/arch_init.h > @@ -28,7 +28,7 @@ extern const uint32_t arch_type; > =20 > void select_soundhw(const char *optarg); > void do_acpitable_option(const QemuOpts *opts); > -void do_smbios_option(const char *optarg); > +void do_smbios_option(QemuOpts *opts); > void cpudef_init(void); > void audio_init(void); > int tcg_available(void); > diff --git a/vl.c b/vl.c > index f422a1c..f63ffd2 100644 > --- a/vl.c > +++ b/vl.c > @@ -3565,7 +3565,8 @@ int main(int argc, char **argv, char **envp) > do_acpitable_option(opts); > break; > case QEMU_OPTION_smbios: > - do_smbios_option(optarg); > + opts =3D qemu_opts_parse(qemu_find_opts("smbios"), opt= arg, 0); > + do_smbios_option(opts); > break; > case QEMU_OPTION_enable_kvm: > olist =3D qemu_find_opts("machine"); > --=20 > 1.8.1.4 >=20