Re: [PATCH v2 5/7] target/i386/cpu: Improve errors for out of bounds property values

[Markus Armbruster <armbru@redhat.com>]

Philippe Mathieu-Daudé <philmd@linaro.org> writes:

> On 10/10/24 16:25, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé <philmd@linaro.org> writes:
>> 
>>> On 10/10/24 12:01, Markus Armbruster wrote:
>>>> The error message for a "stepping" value that is out of bounds is a
>>>> bit odd:
>>>>       $ qemu-system-x86_64 -cpu qemu64,stepping=16
>>>>       qemu-system-x86_64: can't apply global qemu64-x86_64-cpu.stepping=16: Property .stepping doesn't take value 16 (minimum: 0, maximum: 15)
>>>> The "can't apply global" part is an unfortunate artifact of -cpu's
>>>> implementation.  Left for another day.
>>>> The remainder feels overly verbose.  Change it to
>>>>       qemu64-x86_64-cpu: can't apply global qemu64-x86_64-cpu.stepping=16: parameter 'stepping' can be at most 15
>>>> Likewise for "family", "model", and "tsc-frequency".
>>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>

[...]

>>> Confusing:
>>>
>>>      qemu64-x86_64-cpu: can't apply global qemu64-x86_64-cpu.stepping=-1: parameter 'stepping' can be at most 15
>>
>> For better or worse, visit_type_uint64() with the string input visitor
>> parses -1 modulo 2^64, i.e. as 2^64-1, just like strtoul() & friends.

I wish we had avoided that design mistake.  Likely too late to fix now.
The JSON parser gets it right.

> Would "parameter 'stepping' must be between 1 and 15" be clearer?

It might be clearer and would be wronger: zero is a valid value.

I could do "must be between 0 and 15".  But "stepping" is a *counter*.
A negative stepping makes no sense to me.

Same for model and family.

More so for tsc-frequency.

Thoughts?