Re: [Qemu-block] [PATCH v3 1/6] qemu-option: Allow integer defaults

[Eric Blake]

On 1/11/19 8:14 AM, Leonid Bloch wrote:
> On 1/10/19 9:18 PM, Eric Blake wrote:
>> Set the framework up for declaring integer options with an integer
>> default, instead of our current insane approach of requiring the
>> default value to be given as a string (which then has to be reparsed
>> at every use that wants a number).  git grep '[^.]def_value_str' says
>> that we have done a good job of NOT abusing the internal field
>> outside of the implementation in qemu-option.c; therefore, it is not
>> too hard to audit that all code can either handle the new integer
>> defaults correctly or abort because a caller violated constraints.
>> Sadly, we DO have a constraint that qemu_opt_get() should not be
>> called on an option that has an integer type, because we have no
>> where to stash a cached const char * result; but callers that want
>> an integer should be using qemu_opt_get_number() and friends
>> anyways.
>>
>> Signed-off-by: Eric Blake <address@hidden>
>> ---
>>   include/qemu/option.h | 12 ++++++++
>>   util/qemu-option.c    | 69 +++++++++++++++++++++++++++++++++++++------
>>   2 files changed, 72 insertions(+), 9 deletions(-)
>>
>> diff --git a/include/qemu/option.h b/include/qemu/option.h
>> index 844587cab39..46b80d5a6e1 100644
>> --- a/include/qemu/option.h
>> +++ b/include/qemu/option.h
>> @@ -46,6 +46,18 @@ typedef struct QemuOptDesc {
>>       const char *name;
>>       enum QemuOptType type;
>>       const char *help;
>> +    /*
>> +     * For QEMU_OPT_STRING: Leave def_value_int 0, and set def_value_str
>> +     * to a default value or leave NULL for no default.
>> +     *
>> +     * For other types: Initialize at most non-zero def_value_int or a
>> +     * parseable def_value_str for a default (must use a string for an
>> +     * explicit default of 0, although an implicit default generally
>> +     * works).  If setting def_value_int, calling qemu_opt_get() on
>> +     * that option will abort(); instead, call qemu_opt_get_del() or a
>> +     * typed getter.
>> +     */
>> +    uint64_t def_value_int;
>>       const char *def_value_str;
>>   } QemuOptDesc;
>>

> 
> If I understand correctly, the number will still be compiled into the 
> binary as an expression string, but will be printed correctly during 
> runtime?

Anyone that uses .def_value_int will compile into the binary as an
8-byte integer (regardless of how that number was spelled in C, either
as a single constant, or as a constant expression), and NOT as a decimal
string.  String conversions for code that asks for a string will happen
by a runtime printf() (ideally, such code is limited to the case of
printing out information during help output).  Code that is smart enough
to request a number now gets the default value directly, rather than the
old way of having to strtoll() decode a decimal string back into a
number.  No one should ever be using .def_value_str = stringify(macro),
when they can instead just use .def_value_int = macro (which is what the
next patch fixes).

I split the addition of def_value_int from the use in order to ease review.

> If so, it still doesn't feel right to put expressions in binaries, but I 
> agree that it's more elegant than my solution with the lookup table. I'd 
> say the table would be more elegant if there would be more cases in 
> which it's needed, but that's not the case.
> 
> The solution which Markus described as "attractively stupid" also could 
> work, as there are exactly 2 places (which I know of) where it's really 
> needed. Also, absolutely no changes is an option, as was mentioned before.


Markus' suggestion boils down to forgoing patch 1 and 2, rewriting patch
3 and 4 to use hard-coded strings in the two spots that need them (more
of a maintenance burden - if we ever change the macro's value, we have
to also remember to change the string), but keeping patch 5 and 6 as-is.
 It's a smaller diffstat, but less type-safe, and thus a slightly larger
maintenance burden (more technical debt).  My approach is to get rid of
the technical debt (use more type safety, so that we can catch bugs
sooner, and avoid the risk of duplicated values getting out of sync ifa
future patch changes a macro but not the corresponding string). I also
replied to my thread about how we can make further enhancements by even
more patches to FORCE that all integer-typed defaults use only
def_value_int, all string-typed defaults use only def_value_str, at
which point we could switch to using a union for def_value_str/int once
we are sure our code base properly follows the safe typing rules.  But
of course, doing it right costs more time up front (we're avoiding
technical debt, but the patch is bigger and has to be reviewed).  So now
it boils down to Markus' opinion on whether adding type-safe defaults to
QemuOpts is worth it for less technical debt, or if it is adding too
much burden to the fact that we already know that QemuOpts is a hack and
we want to get rid of it in favor of QAPI.

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3226
Virtualization:  qemu.org | libvirt.org

signature.asc
Description: OpenPGP digital signature