Re: [Qemu-devel] [PATCH 4/7] smbios: Make multiple -smbios type= accumulate sanely

[Eric Blake <eblake@redhat.com>]

[-- Attachment #1: Type: text/plain, Size: 2652 bytes --]

On 07/17/2013 11:16 AM, Markus Armbruster wrote:
> Currently, -smbios type=T,NAME=VAL,... adds one field (T,NAME) with
> value VAL to fw_cfg for each unique NAME.  If NAME occurs multiple
> times, the last one's VAL is used (before the QemuOpts conversion, the
> first one was used).
> 
> Multiple -smbios can add multiple fields with the same (T, NAME).
> SeaBIOS reads all of them from fw_cfg, but uses only the first field
> (T, NAME).  The others are ignored.
> 
> "First one wins, subsequent ones get ignored silently" isn't nice.  We
> commonly let the last option win.  Useful, because it lets you
> -readconfig first, then selectively override with command line
> options.
> 
> Clean up -smbios to work the common way.  Accumulate the settings,
> with later ones overwriting earlier ones.  Put the result into fw_cfg
> (no more useless duplicates).
> 
> Bonus cleanup: qemu_uuid_parse() no longer sets SMBIOS system uuid by
> side effect.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---

>      }
> -    val = qemu_opt_get(opts, "release");
> -    if (val) {
> -        if (sscanf(val, "%hhu.%hhu", &major, &minor) != 2) {
> -            error_report("Invalid release");
> -            exit(1);

> @@ -348,7 +353,18 @@ void smbios_entry_add(QemuOpts *opts)
>                  error_report("%s", error_get_pretty(local_err));
>                  exit(1);
>              }
> -            smbios_build_type_0_fields(opts);
> +            save_opt(&type0.vendor, opts, "vendor");
> +            save_opt(&type0.version, opts, "version");
> +            save_opt(&type0.date, opts, "date");
> +
> +            val = qemu_opt_get(opts, "release");
> +            if (val) {
> +                if (sscanf(val, "%hhu.%hhu", &type0.major, &type0.minor) != 2) {

sscanf() is lousy at detecting overflow.  Since this is just code
motion, you can get away without changing it now, but it would be nice
if you at least considered switching this to use [a sane wrapper around]
strtol() for proper overflow detection of user input at some point in
your refactoring.


> +            val = qemu_opt_get(opts, "uuid");
> +            if (val) {
> +                if (qemu_uuid_parse(val, qemu_uuid) != 0) {
> +                    error_report("Invalid UUID");
> +                    exit(1);

My comments from earlier in the series about preferring
exit(EXIT_FAILURE) apply here as well.

Neither issue is a show-stopper, so

Reviewed-by: Eric Blake <eblake@redhat.com>

-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 621 bytes --]