From: "Michael S. Tsirkin" <mst@redhat.com>
To: armbru@redhat.com
Cc: lersek@redhat.com, qemu-devel@nongnu.org,
Anthony Liguori <anthony@codemonkey.ws>,
ehabkost@redhat.com
Subject: Re: [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts
Date: Sat, 28 Sep 2013 23:47:24 +0300 [thread overview]
Message-ID: <20130928204724.GA10454@redhat.com> (raw)
In-Reply-To: <1376659114-6630-3-git-send-email-armbru@redhat.com>
On Fri, Aug 16, 2013 at 03:18:29PM +0200, armbru@redhat.com wrote:
> From: Markus Armbruster <armbru@redhat.com>
>
> So that it can be set in config file for -readconfig.
>
> 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, ...
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> Reviewed-by: Eric Blake <eblake@redhat.com>
> ---
> 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(-)
>
> 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
> }
>
> -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
> }
>
> 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 <alex.williamson@hp.com>
> + * Markus Armbruster <armbru@redhat.com>
> *
> * This work is licensed under the terms of the GNU GPL, version 2. See
> * the COPYING file in the top-level directory.
> @@ -13,6 +15,7 @@
> * GNU GPL, version 2 or (at your option) any later version.
> */
>
> +#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
>
> -
> static uint8_t *smbios_entries;
> static size_t smbios_entries_len;
> static int smbios_type4_count = 0;
>
> +static QemuOptsList qemu_smbios_opts = {
> + .name = "smbios",
> + .head = QTAILQ_HEAD_INITIALIZER(qemu_smbios_opts.head),
> + .desc = {
> + /*
> + * no elements => accept any params
> + * validation will happen later
> + */
> + { /* end of list */ }
> + }
> +};
> +
> +static const QemuOptDesc qemu_smbios_file_opts[] = {
> + {
> + .name = "file",
> + .type = QEMU_OPT_STRING,
> + .help = "binary file containing an SMBIOS element",
> + },
> + { /* end of list */ }
> +};
> +
> +static const QemuOptDesc qemu_smbios_type0_opts[] = {
> + {
> + .name = "type",
> + .type = QEMU_OPT_NUMBER,
> + .help = "SMBIOS element type",
> + },{
> + .name = "vendor",
> + .type = QEMU_OPT_STRING,
> + .help = "vendor name",
> + },{
> + .name = "version",
> + .type = QEMU_OPT_STRING,
> + .help = "version number",
> + },{
> + .name = "date",
> + .type = QEMU_OPT_STRING,
> + .help = "release date",
> + },{
> + .name = "release",
> + .type = QEMU_OPT_STRING,
> + .help = "revision number",
> + },
> + { /* end of list */ }
> +};
> +
> +static const QemuOptDesc qemu_smbios_type1_opts[] = {
> + {
> + .name = "type",
> + .type = QEMU_OPT_NUMBER,
> + .help = "SMBIOS element type",
> + },{
> + .name = "manufacturer",
> + .type = QEMU_OPT_STRING,
> + .help = "manufacturer name",
> + },{
> + .name = "product",
> + .type = QEMU_OPT_STRING,
> + .help = "product name",
> + },{
> + .name = "version",
> + .type = QEMU_OPT_STRING,
> + .help = "version number",
> + },{
> + .name = "serial",
> + .type = QEMU_OPT_STRING,
> + .help = "serial number",
> + },{
> + .name = "uuid",
> + .type = QEMU_OPT_STRING,
> + .help = "UUID",
> + },{
> + .name = "sku",
> + .type = QEMU_OPT_STRING,
> + .help = "SKU number",
> + },{
> + .name = "family",
> + .type = QEMU_OPT_STRING,
> + .help = "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 != 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);
> }
>
> -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;
>
> - if (get_param_value(buf, sizeof(buf), "vendor", t))
> + val = 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 = qemu_opt_get(opts, "version");
> + if (val) {
> smbios_add_field(0, offsetof(struct smbios_type_0, bios_version_str),
> - buf, strlen(buf) + 1);
> - if (get_param_value(buf, sizeof(buf), "date", t))
> + val, strlen(val) + 1);
> + }
> + val = 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) != 2) {
> + val, strlen(val) + 1);
> + }
> + val = qemu_opt_get(opts, "release");
> + if (val) {
> + if (sscanf(val, "%hhu.%hhu", &major, &minor) != 2) {
> error_report("Invalid release");
> exit(1);
> }
> @@ -153,47 +252,69 @@ static void smbios_build_type_0_fields(const char *t)
> }
> }
>
> -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;
>
> - if (get_param_value(buf, sizeof(buf), "manufacturer", t))
> + val = qemu_opt_get(opts, "manufacturer");
> + if (val) {
> smbios_add_field(1, offsetof(struct smbios_type_1, manufacturer_str),
> - buf, strlen(buf) + 1);
> - if (get_param_value(buf, sizeof(buf), "product", t))
> + val, strlen(val) + 1);
> + }
> + val = qemu_opt_get(opts, "product");
> + if (val) {
> smbios_add_field(1, offsetof(struct smbios_type_1, product_name_str),
> - buf, strlen(buf) + 1);
> - if (get_param_value(buf, sizeof(buf), "version", t))
> + val, strlen(val) + 1);
> + }
> + val = 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 = qemu_opt_get(opts, "serial");
> + if (val) {
> smbios_add_field(1, offsetof(struct smbios_type_1, serial_number_str),
> - buf, strlen(buf) + 1);
> - if (get_param_value(buf, sizeof(buf), "uuid", t)) {
> - if (qemu_uuid_parse(buf, qemu_uuid) != 0) {
> + val, strlen(val) + 1);
> + }
> + val = qemu_opt_get(opts, "uuid");
> + if (val) {
> + if (qemu_uuid_parse(val, qemu_uuid) != 0) {
> error_report("Invalid UUID");
> exit(1);
> }
> }
> - if (get_param_value(buf, sizeof(buf), "sku", t))
> + val = 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 = 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);
> + }
> }
>
> -void smbios_entry_add(const char *t)
> +void smbios_entry_add(QemuOpts *opts)
> {
> - char buf[1024];
> + Error *local_err = NULL;
> + const char *val;
>
> - if (get_param_value(buf, sizeof(buf), "file", t)) {
> + val = qemu_opt_get(opts, "file");
> + if (val) {
> struct smbios_structure_header *header;
> struct smbios_table *table;
> - int size = 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);
> + }
>
> + size = get_image_size(val);
> if (size == -1 || size < sizeof(struct smbios_structure_header)) {
> - error_report("Cannot read SMBIOS file %s", buf);
> + error_report("Cannot read SMBIOS file %s", val);
> exit(1);
> }
>
> @@ -208,8 +329,8 @@ void smbios_entry_add(const char *t)
> table->header.type = SMBIOS_TABLE_ENTRY;
> table->header.length = cpu_to_le16(sizeof(*table) + size);
>
> - if (load_image(buf, table->data) != size) {
> - error_report("Failed to load SMBIOS file %s", buf);
> + if (load_image(val, table->data) != size) {
> + error_report("Failed to load SMBIOS file %s", val);
> exit(1);
> }
>
> @@ -225,17 +346,29 @@ void smbios_entry_add(const char *t)
> return;
> }
>
> - if (get_param_value(buf, sizeof(buf), "type", t)) {
> - unsigned long type = strtoul(buf, NULL, 0);
> + val = qemu_opt_get(opts, "type");
> + if (val) {
> + unsigned long type = strtoul(val, NULL, 0);
> +
> switch (type) {
> case 0:
> - smbios_build_type_0_fields(t);
> + qemu_opts_validate(opts, qemu_smbios_type0_opts, &local_err);
> + 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_err);
> + 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 type %ld",
> + error_report("Don't know how to build fields for SMBIOS type %" PRIu64,
> type);
> exit(1);
> }
This triggers a build failure:
/scm/qemu/hw/i386/smbios.c: In function ‘smbios_entry_add’:
/scm/qemu/hw/i386/smbios.c:382:26: error: format ‘%llu’ expects argument
of type ‘long long unsigned int’, but argument 2 has type ‘long unsigned
int’ [-Werror=format=]
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 @@
> *
> */
>
> -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 len);
> uint8_t *smbios_get_table(size_t *length);
>
> 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;
>
> 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 = qemu_opts_parse(qemu_find_opts("smbios"), optarg, 0);
> + do_smbios_option(opts);
> break;
> case QEMU_OPTION_enable_kvm:
> olist = qemu_find_opts("machine");
> --
> 1.8.1.4
>
next prev parent reply other threads:[~2013-09-28 20:45 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-08-16 13:18 [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 1/7] smbios: Normalize smbios_entry_add()'s error handling to exit(1) armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 2/7] smbios: Convert to QemuOpts armbru
2013-09-28 20:47 ` Michael S. Tsirkin [this message]
2013-09-30 8:48 ` Markus Armbruster
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 3/7] smbios: Improve diagnostics for conflicting entries armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 4/7] smbios: Make multiple -smbios type= accumulate sanely armbru
2013-08-17 12:48 ` Eric Blake
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 5/7] smbios: Factor out smbios_maybe_add_str() armbru
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 6/7] vl: Set current_machine early armbru
2013-08-17 13:07 ` Andreas Färber
2013-08-19 9:35 ` Markus Armbruster
2013-08-19 16:37 ` Andreas Färber
2013-08-20 9:09 ` Markus Armbruster
2013-08-16 13:18 ` [Qemu-devel] [PATCH v2 7/7] smbios: Set system manufacturer, product & version by default armbru
2013-08-27 17:04 ` Michael S. Tsirkin
2013-08-27 23:22 ` Eric Blake
2013-08-28 6:05 ` Michael S. Tsirkin
2013-08-17 12:08 ` [Qemu-devel] [PATCH v2 0/7] smbios cleanup & nicer defaults for type 1 Laszlo Ersek
2013-08-17 12:50 ` Eric Blake
2013-09-17 15:27 ` Michael S. Tsirkin
2013-09-18 11:42 ` Markus Armbruster
2013-09-24 6:07 ` Michael S. Tsirkin
2013-09-28 19:41 ` Michael S. Tsirkin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20130928204724.GA10454@redhat.com \
--to=mst@redhat.com \
--cc=anthony@codemonkey.ws \
--cc=armbru@redhat.com \
--cc=ehabkost@redhat.com \
--cc=lersek@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.