| Message ID | 20241010150144.986655-6-armbru@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | error: Eliminate QERR_PROPERTY_VALUE_OUT_OF_RANGE | expand |
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> > --- > target/i386/cpu.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > @@ -5623,16 +5622,15 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > X86CPU *cpu = X86_CPU(obj); > - const int64_t min = 0; > const int64_t max = INT64_MAX; > int64_t value; > > if (!visit_type_int(v, name, &value, errp)) { > return; > } > - if (value < min || value > max) { > - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", > - name ? name : "null", value, min, max); > + if (value < 0 || value > max) { > + error_setg(errp, "parameter '%s' can be at most %" PRId64, > + name ? name : "null", max); Confusing: qemu64-x86_64-cpu: can't apply global qemu64-x86_64-cpu.stepping=-1: parameter 'stepping' can be at most 15 > return; > } >
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> >> --- >> target/i386/cpu.c | 20 +++++++++----------- >> 1 file changed, 9 insertions(+), 11 deletions(-) > > >> @@ -5623,16 +5622,15 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, >> void *opaque, Error **errp) >> { >> X86CPU *cpu = X86_CPU(obj); >> - const int64_t min = 0; >> const int64_t max = INT64_MAX; >> int64_t value; >> if (!visit_type_int(v, name, &value, errp)) { >> return; >> } >> - if (value < min || value > max) { >> - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", >> - name ? name : "null", value, min, max); >> + if (value < 0 || value > max) { >> + error_setg(errp, "parameter '%s' can be at most %" PRId64, >> + name ? name : "null", max); > > 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. >> return; >> } >>
On Thu, 10 Oct 2024 17:01:42 +0200 Markus Armbruster <armbru@redhat.com> 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> Reviewed-by: Igor Mammedov <imammedo@redhat.com> > --- > target/i386/cpu.c | 20 +++++++++----------- > 1 file changed, 9 insertions(+), 11 deletions(-) > > diff --git a/target/i386/cpu.c b/target/i386/cpu.c > index 4f8fa60432..de2c7041c5 100644 > --- a/target/i386/cpu.c > +++ b/target/i386/cpu.c > @@ -31,7 +31,6 @@ > #include "qapi/error.h" > #include "qemu/error-report.h" > #include "qapi/qapi-visit-machine.h" > -#include "qapi/qmp/qerror.h" > #include "standard-headers/asm-x86/kvm_para.h" > #include "hw/qdev-properties.h" > #include "hw/i386/topology.h" > @@ -5455,8 +5454,8 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, > return; > } > if (value > max) { > - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", > - name ? name : "null", value, (int64_t)0, (int64_t)max); > + error_setg(errp, "parameter '%s' can be at most %" PRIu64, > + name ? name : "null", max); > return; > } > > @@ -5494,8 +5493,8 @@ static void x86_cpuid_version_set_model(Object *obj, Visitor *v, > return; > } > if (value > max) { > - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", > - name ? name : "null", value, (int64_t)0, (int64_t)max); > + error_setg(errp, "parameter '%s' can be at most %" PRIu64, > + name ? name : "null", max); > return; > } > > @@ -5528,8 +5527,8 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v, > return; > } > if (value > max) { > - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", > - name ? name : "null", value, (int64_t)0, (int64_t)max); > + error_setg(errp, "parameter '%s' can be at most %" PRIu64, > + name ? name : "null", max); > return; > } > > @@ -5623,16 +5622,15 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, > void *opaque, Error **errp) > { > X86CPU *cpu = X86_CPU(obj); > - const int64_t min = 0; > const int64_t max = INT64_MAX; > int64_t value; > > if (!visit_type_int(v, name, &value, errp)) { > return; > } > - if (value < min || value > max) { > - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", > - name ? name : "null", value, min, max); > + if (value < 0 || value > max) { > + error_setg(errp, "parameter '%s' can be at most %" PRId64, > + name ? name : "null", max); > return; > } >
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> >>> --- >>> target/i386/cpu.c | 20 +++++++++----------- >>> 1 file changed, 9 insertions(+), 11 deletions(-) >> >> >>> @@ -5623,16 +5622,15 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, >>> void *opaque, Error **errp) >>> { >>> X86CPU *cpu = X86_CPU(obj); >>> - const int64_t min = 0; >>> const int64_t max = INT64_MAX; >>> int64_t value; >>> if (!visit_type_int(v, name, &value, errp)) { >>> return; >>> } >>> - if (value < min || value > max) { >>> - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", >>> - name ? name : "null", value, min, max); >>> + if (value < 0 || value > max) { >>> + error_setg(errp, "parameter '%s' can be at most %" PRId64, >>> + name ? name : "null", max); >> >> 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. Would "parameter 'stepping' must be between 1 and 15" be clearer?
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?
diff --git a/target/i386/cpu.c b/target/i386/cpu.c index 4f8fa60432..de2c7041c5 100644 --- a/target/i386/cpu.c +++ b/target/i386/cpu.c @@ -31,7 +31,6 @@ #include "qapi/error.h" #include "qemu/error-report.h" #include "qapi/qapi-visit-machine.h" -#include "qapi/qmp/qerror.h" #include "standard-headers/asm-x86/kvm_para.h" #include "hw/qdev-properties.h" #include "hw/i386/topology.h" @@ -5455,8 +5454,8 @@ static void x86_cpuid_version_set_family(Object *obj, Visitor *v, return; } if (value > max) { - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, (int64_t)0, (int64_t)max); + error_setg(errp, "parameter '%s' can be at most %" PRIu64, + name ? name : "null", max); return; } @@ -5494,8 +5493,8 @@ static void x86_cpuid_version_set_model(Object *obj, Visitor *v, return; } if (value > max) { - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, (int64_t)0, (int64_t)max); + error_setg(errp, "parameter '%s' can be at most %" PRIu64, + name ? name : "null", max); return; } @@ -5528,8 +5527,8 @@ static void x86_cpuid_version_set_stepping(Object *obj, Visitor *v, return; } if (value > max) { - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, (int64_t)0, (int64_t)max); + error_setg(errp, "parameter '%s' can be at most %" PRIu64, + name ? name : "null", max); return; } @@ -5623,16 +5622,15 @@ static void x86_cpuid_set_tsc_freq(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { X86CPU *cpu = X86_CPU(obj); - const int64_t min = 0; const int64_t max = INT64_MAX; int64_t value; if (!visit_type_int(v, name, &value, errp)) { return; } - if (value < min || value > max) { - error_setg(errp, QERR_PROPERTY_VALUE_OUT_OF_RANGE, "", - name ? name : "null", value, min, max); + if (value < 0 || value > max) { + error_setg(errp, "parameter '%s' can be at most %" PRId64, + name ? name : "null", max); return; }
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> --- target/i386/cpu.c | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-)