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

[Markus Armbruster]

Eric Blake <address@hidden> writes:

> 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 <address@hidden>
>> ---
>
>>      }
>> -    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.

Correct use of strol() & friends is surprisingly involved, and a bit
more than I bargained for when moving this code around :)

>> +            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.

I very much prefer all unsuccessful exits to look the same.  There are
eight instances of exit(1) in this file before my series, I'm removing
three, and adding six.

While I'm sympathetic to the general "symbols are nicer than magic
numbers" sentiment, exit(1) is as harmless as it gets.

> Neither issue is a show-stopper, so
>
> Reviewed-by: Eric Blake <address@hidden>

Thanks for the review!