* [PATCH 1/4] target: Simplify type checks for CpuModelInfo member @props
2024-03-05 14:59 [PATCH 0/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
@ 2024-03-05 14:59 ` Markus Armbruster
2024-03-06 13:05 ` Daniel Henrique Barboza
2024-03-05 14:59 ` [PATCH 2/4] target/i386: Fix query-cpu-model-expansion to reject props Markus Armbruster
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2024-03-05 14:59 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, pbonzini, gaosong, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu, richard.henderson,
david, iii, thuth, qemu-arm, qemu-riscv, qemu-s390x
CpuModelInfo member @props is semantically a mapping from name to
value, and syntactically a JSON object on the wire. This translates
to QDict in C. Since the QAPI schema language lacks the means to
express 'object', we use 'any' instead. This is QObject in C.
Commands taking a CpuModelInfo argument need to check the QObject is a
QDict.
For arm, riscv, and s390x, the code checks right before passing the
QObject to visit_start_struct(). visit_start_struct() then checks
again.
Delete the first check.
The error message for @props that are not an object changes slightly
to the the message we get for this kind of type error in other
contexts. Minor improvement.
Additionally, error messages about members of @props now refer to
'props.prop-name' instead of just 'prop-name'. Another minor
improvement.
Both changes are visible in tests/qtest/arm-cpu-features.c.
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
target/arm/arm-qmp-cmds.c | 15 ++++-----------
target/riscv/riscv-qmp-cmds.c | 19 +++++--------------
target/s390x/cpu_models_sysemu.c | 15 ++++-----------
tests/qtest/arm-cpu-features.c | 12 ++++++------
4 files changed, 19 insertions(+), 42 deletions(-)
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 2250cd7ddf..0167759730 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -104,7 +104,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
Error **errp)
{
CpuModelExpansionInfo *expansion_info;
- const QDict *qdict_in = NULL;
+ const QDict *qdict_in;
QDict *qdict_out;
ObjectClass *oc;
Object *obj;
@@ -151,27 +151,20 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
}
}
- if (model->props) {
- qdict_in = qobject_to(QDict, model->props);
- if (!qdict_in) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
- return NULL;
- }
- }
-
obj = object_new(object_class_get_name(oc));
- if (qdict_in) {
+ if (model->props) {
Visitor *visitor;
Error *err = NULL;
visitor = qobject_input_visitor_new(model->props);
- if (!visit_start_struct(visitor, NULL, NULL, 0, errp)) {
+ if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
visit_free(visitor);
object_unref(obj);
return NULL;
}
+ qdict_in = qobject_to(QDict, model->props);
i = 0;
while ((name = cpu_model_advertised_features[i++]) != NULL) {
if (qdict_get(qdict_in, name)) {
diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index c48b9cfa67..69dde0c3e7 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -129,18 +129,19 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out)
}
static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
- const QDict *qdict_in,
Error **errp)
{
+ const QDict *qdict_in;
const QDictEntry *qe;
Visitor *visitor;
Error *local_err = NULL;
visitor = qobject_input_visitor_new(props);
- if (!visit_start_struct(visitor, NULL, NULL, 0, &local_err)) {
+ if (!visit_start_struct(visitor, "props", NULL, 0, &local_err)) {
goto err;
}
+ qdict_in = qobject_to(QDict, props);
for (qe = qdict_first(qdict_in); qe; qe = qdict_next(qdict_in, qe)) {
object_property_find_err(obj, qe->key, &local_err);
if (local_err) {
@@ -170,7 +171,6 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
Error **errp)
{
CpuModelExpansionInfo *expansion_info;
- const QDict *qdict_in = NULL;
QDict *qdict_out;
ObjectClass *oc;
Object *obj;
@@ -188,14 +188,6 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return NULL;
}
- if (model->props) {
- qdict_in = qobject_to(QDict, model->props);
- if (!qdict_in) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
- return NULL;
- }
- }
-
obj = object_new(object_class_get_name(oc));
riscv_check_if_cpu_available(RISCV_CPU(obj), &local_err);
@@ -205,9 +197,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return NULL;
}
- if (qdict_in) {
- riscv_cpuobj_validate_qdict_in(obj, model->props, qdict_in,
- &local_err);
+ if (model->props) {
+ riscv_cpuobj_validate_qdict_in(obj, model->props, &local_err);
if (local_err) {
error_propagate(errp, local_err);
object_unref(obj);
diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index 63981bf36b..ef19724888 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -101,21 +101,13 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
Error **errp)
{
Error *err = NULL;
- const QDict *qdict = NULL;
+ const QDict *qdict;
const QDictEntry *e;
Visitor *visitor;
ObjectClass *oc;
S390CPU *cpu;
Object *obj;
- if (info->props) {
- qdict = qobject_to(QDict, info->props);
- if (!qdict) {
- error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
- return;
- }
- }
-
oc = cpu_class_by_name(TYPE_S390_CPU, info->name);
if (!oc) {
error_setg(errp, "The CPU definition \'%s\' is unknown.", info->name);
@@ -135,13 +127,14 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
return;
}
- if (qdict) {
+ if (info->props) {
visitor = qobject_input_visitor_new(info->props);
- if (!visit_start_struct(visitor, NULL, NULL, 0, errp)) {
+ if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
visit_free(visitor);
object_unref(obj);
return;
}
+ qdict = qobject_to(QDict, info->props);
for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
if (!object_property_set(obj, e->key, visitor, &err)) {
break;
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index a8a4c668ad..1daceb2e31 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -79,7 +79,7 @@ static const char *resp_get_error(QDict *resp)
g_assert(_resp); \
_error = resp_get_error(_resp); \
g_assert(_error); \
- g_assert(g_str_equal(_error, expected_error)); \
+ g_assert_cmpstr(_error, ==, expected_error); \
qobject_unref(_resp); \
})
@@ -194,8 +194,8 @@ static void assert_type_full(QTestState *qts)
g_assert(resp);
error = resp_get_error(resp);
g_assert(error);
- g_assert(g_str_equal(error,
- "The requested expansion type is not supported"));
+ g_assert_cmpstr(error, ==,
+ "The requested expansion type is not supported");
qobject_unref(resp);
}
@@ -212,8 +212,8 @@ static void assert_bad_props(QTestState *qts, const char *cpu_type)
g_assert(resp);
error = resp_get_error(resp);
g_assert(error);
- g_assert(g_str_equal(error,
- "Invalid parameter type for 'props', expected: dict"));
+ g_assert_cmpstr(error, ==,
+ "Invalid parameter type for 'props', expected: object");
qobject_unref(resp);
}
@@ -446,7 +446,7 @@ static void test_query_cpu_model_expansion(const void *data)
assert_bad_props(qts, "max");
assert_error(qts, "foo", "The CPU type 'foo' is not a recognized "
"ARM CPU type", NULL);
- assert_error(qts, "max", "Parameter 'not-a-prop' is unexpected",
+ assert_error(qts, "max", "Parameter 'props.not-a-prop' is unexpected",
"{ 'not-a-prop': false }");
assert_error(qts, "host", "The CPU type 'host' requires KVM", NULL);
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 1/4] target: Simplify type checks for CpuModelInfo member @props
2024-03-05 14:59 ` [PATCH 1/4] target: Simplify type checks " Markus Armbruster
@ 2024-03-06 13:05 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2024-03-06 13:05 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: peter.maydell, pbonzini, gaosong, palmer, alistair.francis,
bin.meng, liwei1518, zhiwei_liu, richard.henderson, david, iii,
thuth, qemu-arm, qemu-riscv, qemu-s390x
On 3/5/24 11:59, Markus Armbruster wrote:
> CpuModelInfo member @props is semantically a mapping from name to
> value, and syntactically a JSON object on the wire. This translates
> to QDict in C. Since the QAPI schema language lacks the means to
> express 'object', we use 'any' instead. This is QObject in C.
> Commands taking a CpuModelInfo argument need to check the QObject is a
> QDict.
>
> For arm, riscv, and s390x, the code checks right before passing the
> QObject to visit_start_struct(). visit_start_struct() then checks
> again.
>
> Delete the first check.
>
> The error message for @props that are not an object changes slightly
> to the the message we get for this kind of type error in other
> contexts. Minor improvement.
>
> Additionally, error messages about members of @props now refer to
> 'props.prop-name' instead of just 'prop-name'. Another minor
> improvement.
>
> Both changes are visible in tests/qtest/arm-cpu-features.c.
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
For RISC-V changes:
Acked-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/arm/arm-qmp-cmds.c | 15 ++++-----------
> target/riscv/riscv-qmp-cmds.c | 19 +++++--------------
> target/s390x/cpu_models_sysemu.c | 15 ++++-----------
> tests/qtest/arm-cpu-features.c | 12 ++++++------
> 4 files changed, 19 insertions(+), 42 deletions(-)
>
> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index 2250cd7ddf..0167759730 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -104,7 +104,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> Error **errp)
> {
> CpuModelExpansionInfo *expansion_info;
> - const QDict *qdict_in = NULL;
> + const QDict *qdict_in;
> QDict *qdict_out;
> ObjectClass *oc;
> Object *obj;
> @@ -151,27 +151,20 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> }
> }
>
> - if (model->props) {
> - qdict_in = qobject_to(QDict, model->props);
> - if (!qdict_in) {
> - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> - return NULL;
> - }
> - }
> -
> obj = object_new(object_class_get_name(oc));
>
> - if (qdict_in) {
> + if (model->props) {
> Visitor *visitor;
> Error *err = NULL;
>
> visitor = qobject_input_visitor_new(model->props);
> - if (!visit_start_struct(visitor, NULL, NULL, 0, errp)) {
> + if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
> visit_free(visitor);
> object_unref(obj);
> return NULL;
> }
>
> + qdict_in = qobject_to(QDict, model->props);
> i = 0;
> while ((name = cpu_model_advertised_features[i++]) != NULL) {
> if (qdict_get(qdict_in, name)) {
> diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
> index c48b9cfa67..69dde0c3e7 100644
> --- a/target/riscv/riscv-qmp-cmds.c
> +++ b/target/riscv/riscv-qmp-cmds.c
> @@ -129,18 +129,19 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out)
> }
>
> static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
> - const QDict *qdict_in,
> Error **errp)
> {
> + const QDict *qdict_in;
> const QDictEntry *qe;
> Visitor *visitor;
> Error *local_err = NULL;
>
> visitor = qobject_input_visitor_new(props);
> - if (!visit_start_struct(visitor, NULL, NULL, 0, &local_err)) {
> + if (!visit_start_struct(visitor, "props", NULL, 0, &local_err)) {
> goto err;
> }
>
> + qdict_in = qobject_to(QDict, props);
> for (qe = qdict_first(qdict_in); qe; qe = qdict_next(qdict_in, qe)) {
> object_property_find_err(obj, qe->key, &local_err);
> if (local_err) {
> @@ -170,7 +171,6 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> Error **errp)
> {
> CpuModelExpansionInfo *expansion_info;
> - const QDict *qdict_in = NULL;
> QDict *qdict_out;
> ObjectClass *oc;
> Object *obj;
> @@ -188,14 +188,6 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> return NULL;
> }
>
> - if (model->props) {
> - qdict_in = qobject_to(QDict, model->props);
> - if (!qdict_in) {
> - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> - return NULL;
> - }
> - }
> -
> obj = object_new(object_class_get_name(oc));
>
> riscv_check_if_cpu_available(RISCV_CPU(obj), &local_err);
> @@ -205,9 +197,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> return NULL;
> }
>
> - if (qdict_in) {
> - riscv_cpuobj_validate_qdict_in(obj, model->props, qdict_in,
> - &local_err);
> + if (model->props) {
> + riscv_cpuobj_validate_qdict_in(obj, model->props, &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> object_unref(obj);
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index 63981bf36b..ef19724888 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -101,21 +101,13 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
> Error **errp)
> {
> Error *err = NULL;
> - const QDict *qdict = NULL;
> + const QDict *qdict;
> const QDictEntry *e;
> Visitor *visitor;
> ObjectClass *oc;
> S390CPU *cpu;
> Object *obj;
>
> - if (info->props) {
> - qdict = qobject_to(QDict, info->props);
> - if (!qdict) {
> - error_setg(errp, QERR_INVALID_PARAMETER_TYPE, "props", "dict");
> - return;
> - }
> - }
> -
> oc = cpu_class_by_name(TYPE_S390_CPU, info->name);
> if (!oc) {
> error_setg(errp, "The CPU definition \'%s\' is unknown.", info->name);
> @@ -135,13 +127,14 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
> return;
> }
>
> - if (qdict) {
> + if (info->props) {
> visitor = qobject_input_visitor_new(info->props);
> - if (!visit_start_struct(visitor, NULL, NULL, 0, errp)) {
> + if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
> visit_free(visitor);
> object_unref(obj);
> return;
> }
> + qdict = qobject_to(QDict, info->props);
> for (e = qdict_first(qdict); e; e = qdict_next(qdict, e)) {
> if (!object_property_set(obj, e->key, visitor, &err)) {
> break;
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index a8a4c668ad..1daceb2e31 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -79,7 +79,7 @@ static const char *resp_get_error(QDict *resp)
> g_assert(_resp); \
> _error = resp_get_error(_resp); \
> g_assert(_error); \
> - g_assert(g_str_equal(_error, expected_error)); \
> + g_assert_cmpstr(_error, ==, expected_error); \
> qobject_unref(_resp); \
> })
>
> @@ -194,8 +194,8 @@ static void assert_type_full(QTestState *qts)
> g_assert(resp);
> error = resp_get_error(resp);
> g_assert(error);
> - g_assert(g_str_equal(error,
> - "The requested expansion type is not supported"));
> + g_assert_cmpstr(error, ==,
> + "The requested expansion type is not supported");
> qobject_unref(resp);
> }
>
> @@ -212,8 +212,8 @@ static void assert_bad_props(QTestState *qts, const char *cpu_type)
> g_assert(resp);
> error = resp_get_error(resp);
> g_assert(error);
> - g_assert(g_str_equal(error,
> - "Invalid parameter type for 'props', expected: dict"));
> + g_assert_cmpstr(error, ==,
> + "Invalid parameter type for 'props', expected: object");
> qobject_unref(resp);
> }
>
> @@ -446,7 +446,7 @@ static void test_query_cpu_model_expansion(const void *data)
> assert_bad_props(qts, "max");
> assert_error(qts, "foo", "The CPU type 'foo' is not a recognized "
> "ARM CPU type", NULL);
> - assert_error(qts, "max", "Parameter 'not-a-prop' is unexpected",
> + assert_error(qts, "max", "Parameter 'props.not-a-prop' is unexpected",
> "{ 'not-a-prop': false }");
> assert_error(qts, "host", "The CPU type 'host' requires KVM", NULL);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 2/4] target/i386: Fix query-cpu-model-expansion to reject props
2024-03-05 14:59 [PATCH 0/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
2024-03-05 14:59 ` [PATCH 1/4] target: Simplify type checks " Markus Armbruster
@ 2024-03-05 14:59 ` Markus Armbruster
2024-03-05 14:59 ` [PATCH 3/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
2024-03-05 14:59 ` [PATCH 4/4] target/loongarch: Fix query-cpu-model-expansion to reject props Markus Armbruster
3 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2024-03-05 14:59 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, pbonzini, gaosong, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu, richard.henderson,
david, iii, thuth, qemu-arm, qemu-riscv, qemu-s390x
CpuModelInfo member @props is semantically a mapping from name to
value, and syntactically a JSON object on the wire. This translates
to QDict in C. Since the QAPI schema language lacks the means to
express 'object', we use 'any' instead. This is QObject in C.
Commands taking a CpuModelInfo argument need to check the QObject is a
QDict.
The i386 version of qmp_query_cpu_model_expansion() fails to check.
Instead, @props is silently ignored when it's not an object. For
instance,
{"execute": "query-cpu-model-expansion", "arguments": {"type": "full", "model": {"name": "qemu64", "props": null}}}
succeeds.
Fix by refactoring the code to match the other targets. Now the
command fails as it should:
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'props', expected: object"}}
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
target/i386/cpu-sysemu.c | 31 +++++++++++++++++++++++--------
1 file changed, 23 insertions(+), 8 deletions(-)
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index 7422096737..df91f97827 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -25,6 +25,7 @@
#include "qapi/error.h"
#include "qapi/qapi-visit-run-state.h"
#include "qapi/qmp/qdict.h"
+#include "qapi/qobject-input-visitor.h"
#include "qom/qom-qobject.h"
#include "qapi/qapi-commands-machine-target.h"
#include "hw/qdev-properties.h"
@@ -129,20 +130,35 @@ static void x86_cpu_to_dict_full(X86CPU *cpu, QDict *props)
}
}
-static void object_apply_props(Object *obj, QDict *props, Error **errp)
+static void object_apply_props(Object *obj, QObject *props, Error **errp)
{
+ Visitor *visitor;
+ QDict *qdict;
const QDictEntry *prop;
- for (prop = qdict_first(props); prop; prop = qdict_next(props, prop)) {
- if (!object_property_set_qobject(obj, qdict_entry_key(prop),
- qdict_entry_value(prop), errp)) {
- break;
+ visitor = qobject_input_visitor_new(props);
+ if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
+ visit_free(visitor);
+ return;
+ }
+
+ qdict = qobject_to(QDict, props);
+ for (prop = qdict_first(qdict); prop; prop = qdict_next(qdict, prop)) {
+ if (!object_property_set(obj, qdict_entry_key(prop),
+ visitor, errp)) {
+ goto out;
}
}
+
+ visit_check_struct(visitor, errp);
+out:
+ visit_end_struct(visitor, NULL);
+ visit_free(visitor);
}
/* Create X86CPU object according to model+props specification */
-static X86CPU *x86_cpu_from_model(const char *model, QDict *props, Error **errp)
+static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
+ Error **errp)
{
X86CPU *xc = NULL;
X86CPUClass *xcc;
@@ -187,8 +203,7 @@ qmp_query_cpu_model_expansion(CpuModelExpansionType type,
QDict *props = NULL;
const char *base_name;
- xc = x86_cpu_from_model(model->name, qobject_to(QDict, model->props),
- &err);
+ xc = x86_cpu_from_model(model->name, model->props, &err);
if (err) {
goto out;
}
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* [PATCH 3/4] target: Improve error reporting for CpuModelInfo member @props
2024-03-05 14:59 [PATCH 0/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
2024-03-05 14:59 ` [PATCH 1/4] target: Simplify type checks " Markus Armbruster
2024-03-05 14:59 ` [PATCH 2/4] target/i386: Fix query-cpu-model-expansion to reject props Markus Armbruster
@ 2024-03-05 14:59 ` Markus Armbruster
2024-03-06 13:05 ` Daniel Henrique Barboza
2024-03-05 14:59 ` [PATCH 4/4] target/loongarch: Fix query-cpu-model-expansion to reject props Markus Armbruster
3 siblings, 1 reply; 7+ messages in thread
From: Markus Armbruster @ 2024-03-05 14:59 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, pbonzini, gaosong, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu, richard.henderson,
david, iii, thuth, qemu-arm, qemu-riscv, qemu-s390x
query-cpu-model-comparison, query-cpu-model-baseline, and
query-cpu-model-expansion take CpuModelInfo arguments. Errors in
@props members of these arguments are reported for 'props', without
further context. For instance, s390x rejects
{"execute": "query-cpu-model-comparison", "arguments": {"modela": {"name": "z13", "props": {}}, "modelb": {"name": "z14", "props": []}}}
with
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'props', expected: object"}}
This is unusual; the common QAPI unmarshaling machinery would complain
about 'modelb.props'. Our hand-written code to visit the @props
member neglects to provide the context.
Tweak it so it provides it. The command above now fails with
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'modelb.props', expected: dict"}}
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
target/arm/arm-qmp-cmds.c | 2 +-
target/i386/cpu-sysemu.c | 11 ++++++-----
target/riscv/riscv-qmp-cmds.c | 6 ++++--
target/s390x/cpu_models_sysemu.c | 17 ++++++++++-------
tests/qtest/arm-cpu-features.c | 5 +++--
5 files changed, 24 insertions(+), 17 deletions(-)
diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
index 0167759730..0efbee8af8 100644
--- a/target/arm/arm-qmp-cmds.c
+++ b/target/arm/arm-qmp-cmds.c
@@ -158,7 +158,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
Error *err = NULL;
visitor = qobject_input_visitor_new(model->props);
- if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
+ if (!visit_start_struct(visitor, "model.props", NULL, 0, errp)) {
visit_free(visitor);
object_unref(obj);
return NULL;
diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
index df91f97827..3f9093d285 100644
--- a/target/i386/cpu-sysemu.c
+++ b/target/i386/cpu-sysemu.c
@@ -130,14 +130,15 @@ static void x86_cpu_to_dict_full(X86CPU *cpu, QDict *props)
}
}
-static void object_apply_props(Object *obj, QObject *props, Error **errp)
+static void object_apply_props(Object *obj, QObject *props,
+ const char *props_arg_name, Error **errp)
{
Visitor *visitor;
QDict *qdict;
const QDictEntry *prop;
visitor = qobject_input_visitor_new(props);
- if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
+ if (!visit_start_struct(visitor, props_arg_name, NULL, 0, errp)) {
visit_free(visitor);
return;
}
@@ -158,7 +159,7 @@ out:
/* Create X86CPU object according to model+props specification */
static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
- Error **errp)
+ const char *props_arg_name, Error **errp)
{
X86CPU *xc = NULL;
X86CPUClass *xcc;
@@ -172,7 +173,7 @@ static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
if (props) {
- object_apply_props(OBJECT(xc), props, &err);
+ object_apply_props(OBJECT(xc), props, props_arg_name, &err);
if (err) {
goto out;
}
@@ -203,7 +204,7 @@ qmp_query_cpu_model_expansion(CpuModelExpansionType type,
QDict *props = NULL;
const char *base_name;
- xc = x86_cpu_from_model(model->name, model->props, &err);
+ xc = x86_cpu_from_model(model->name, model->props, "model.props", &err);
if (err) {
goto out;
}
diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
index 69dde0c3e7..ee7465297d 100644
--- a/target/riscv/riscv-qmp-cmds.c
+++ b/target/riscv/riscv-qmp-cmds.c
@@ -129,6 +129,7 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out)
}
static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
+ const char *props_arg_name,
Error **errp)
{
const QDict *qdict_in;
@@ -137,7 +138,7 @@ static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
Error *local_err = NULL;
visitor = qobject_input_visitor_new(props);
- if (!visit_start_struct(visitor, "props", NULL, 0, &local_err)) {
+ if (!visit_start_struct(visitor, props_arg_name, NULL, 0, &local_err)) {
goto err;
}
@@ -198,7 +199,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
}
if (model->props) {
- riscv_cpuobj_validate_qdict_in(obj, model->props, &local_err);
+ riscv_cpuobj_validate_qdict_in(obj, model->props, "model.props",
+ &local_err);
if (local_err) {
error_propagate(errp, local_err);
object_unref(obj);
diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
index ef19724888..8338084d00 100644
--- a/target/s390x/cpu_models_sysemu.c
+++ b/target/s390x/cpu_models_sysemu.c
@@ -98,7 +98,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
}
static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
- Error **errp)
+ const char *info_arg_name, Error **errp)
{
Error *err = NULL;
const QDict *qdict;
@@ -128,8 +128,11 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
}
if (info->props) {
+ g_autofree const char *props_name = g_strdup_printf("%s.props",
+ info_arg_name);
+
visitor = qobject_input_visitor_new(info->props);
- if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
+ if (!visit_start_struct(visitor, props_name, NULL, 0, errp)) {
visit_free(visitor);
object_unref(obj);
return;
@@ -216,7 +219,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
bool delta_changes = false;
/* convert it to our internal representation */
- cpu_model_from_info(&s390_model, model, &err);
+ cpu_model_from_info(&s390_model, model, "model", &err);
if (err) {
error_propagate(errp, err);
return NULL;
@@ -254,12 +257,12 @@ CpuModelCompareInfo *qmp_query_cpu_model_comparison(CpuModelInfo *infoa,
S390CPUModel modela, modelb;
/* convert both models to our internal representation */
- cpu_model_from_info(&modela, infoa, &err);
+ cpu_model_from_info(&modela, infoa, "modela", &err);
if (err) {
error_propagate(errp, err);
return NULL;
}
- cpu_model_from_info(&modelb, infob, &err);
+ cpu_model_from_info(&modelb, infob, "modelb", &err);
if (err) {
error_propagate(errp, err);
return NULL;
@@ -331,13 +334,13 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
uint8_t max_gen;
/* convert both models to our internal representation */
- cpu_model_from_info(&modela, infoa, &err);
+ cpu_model_from_info(&modela, infoa, "modela", &err);
if (err) {
error_propagate(errp, err);
return NULL;
}
- cpu_model_from_info(&modelb, infob, &err);
+ cpu_model_from_info(&modelb, infob, "modelb", &err);
if (err) {
error_propagate(errp, err);
return NULL;
diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
index 1daceb2e31..9d6e6190d5 100644
--- a/tests/qtest/arm-cpu-features.c
+++ b/tests/qtest/arm-cpu-features.c
@@ -213,7 +213,8 @@ static void assert_bad_props(QTestState *qts, const char *cpu_type)
error = resp_get_error(resp);
g_assert(error);
g_assert_cmpstr(error, ==,
- "Invalid parameter type for 'props', expected: object");
+ "Invalid parameter type for 'model.props',"
+ " expected: object");
qobject_unref(resp);
}
@@ -446,7 +447,7 @@ static void test_query_cpu_model_expansion(const void *data)
assert_bad_props(qts, "max");
assert_error(qts, "foo", "The CPU type 'foo' is not a recognized "
"ARM CPU type", NULL);
- assert_error(qts, "max", "Parameter 'props.not-a-prop' is unexpected",
+ assert_error(qts, "max", "Parameter 'model.props.not-a-prop' is unexpected",
"{ 'not-a-prop': false }");
assert_error(qts, "host", "The CPU type 'host' requires KVM", NULL);
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread* Re: [PATCH 3/4] target: Improve error reporting for CpuModelInfo member @props
2024-03-05 14:59 ` [PATCH 3/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
@ 2024-03-06 13:05 ` Daniel Henrique Barboza
0 siblings, 0 replies; 7+ messages in thread
From: Daniel Henrique Barboza @ 2024-03-06 13:05 UTC (permalink / raw)
To: Markus Armbruster, qemu-devel
Cc: peter.maydell, pbonzini, gaosong, palmer, alistair.francis,
bin.meng, liwei1518, zhiwei_liu, richard.henderson, david, iii,
thuth, qemu-arm, qemu-riscv, qemu-s390x
On 3/5/24 11:59, Markus Armbruster wrote:
> query-cpu-model-comparison, query-cpu-model-baseline, and
> query-cpu-model-expansion take CpuModelInfo arguments. Errors in
> @props members of these arguments are reported for 'props', without
> further context. For instance, s390x rejects
>
> {"execute": "query-cpu-model-comparison", "arguments": {"modela": {"name": "z13", "props": {}}, "modelb": {"name": "z14", "props": []}}}
>
> with
>
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'props', expected: object"}}
>
> This is unusual; the common QAPI unmarshaling machinery would complain
> about 'modelb.props'. Our hand-written code to visit the @props
> member neglects to provide the context.
>
> Tweak it so it provides it. The command above now fails with
>
> {"error": {"class": "GenericError", "desc": "Invalid parameter type for 'modelb.props', expected: dict"}}
>
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
For RISC-V changes:
Acked-by: Daniel Henrique Barboza <dbarboza@ventanamicro.com>
> target/arm/arm-qmp-cmds.c | 2 +-
> target/i386/cpu-sysemu.c | 11 ++++++-----
> target/riscv/riscv-qmp-cmds.c | 6 ++++--
> target/s390x/cpu_models_sysemu.c | 17 ++++++++++-------
> tests/qtest/arm-cpu-features.c | 5 +++--
> 5 files changed, 24 insertions(+), 17 deletions(-)
>
> diff --git a/target/arm/arm-qmp-cmds.c b/target/arm/arm-qmp-cmds.c
> index 0167759730..0efbee8af8 100644
> --- a/target/arm/arm-qmp-cmds.c
> +++ b/target/arm/arm-qmp-cmds.c
> @@ -158,7 +158,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> Error *err = NULL;
>
> visitor = qobject_input_visitor_new(model->props);
> - if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
> + if (!visit_start_struct(visitor, "model.props", NULL, 0, errp)) {
> visit_free(visitor);
> object_unref(obj);
> return NULL;
> diff --git a/target/i386/cpu-sysemu.c b/target/i386/cpu-sysemu.c
> index df91f97827..3f9093d285 100644
> --- a/target/i386/cpu-sysemu.c
> +++ b/target/i386/cpu-sysemu.c
> @@ -130,14 +130,15 @@ static void x86_cpu_to_dict_full(X86CPU *cpu, QDict *props)
> }
> }
>
> -static void object_apply_props(Object *obj, QObject *props, Error **errp)
> +static void object_apply_props(Object *obj, QObject *props,
> + const char *props_arg_name, Error **errp)
> {
> Visitor *visitor;
> QDict *qdict;
> const QDictEntry *prop;
>
> visitor = qobject_input_visitor_new(props);
> - if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
> + if (!visit_start_struct(visitor, props_arg_name, NULL, 0, errp)) {
> visit_free(visitor);
> return;
> }
> @@ -158,7 +159,7 @@ out:
>
> /* Create X86CPU object according to model+props specification */
> static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
> - Error **errp)
> + const char *props_arg_name, Error **errp)
> {
> X86CPU *xc = NULL;
> X86CPUClass *xcc;
> @@ -172,7 +173,7 @@ static X86CPU *x86_cpu_from_model(const char *model, QObject *props,
>
> xc = X86_CPU(object_new_with_class(OBJECT_CLASS(xcc)));
> if (props) {
> - object_apply_props(OBJECT(xc), props, &err);
> + object_apply_props(OBJECT(xc), props, props_arg_name, &err);
> if (err) {
> goto out;
> }
> @@ -203,7 +204,7 @@ qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> QDict *props = NULL;
> const char *base_name;
>
> - xc = x86_cpu_from_model(model->name, model->props, &err);
> + xc = x86_cpu_from_model(model->name, model->props, "model.props", &err);
> if (err) {
> goto out;
> }
> diff --git a/target/riscv/riscv-qmp-cmds.c b/target/riscv/riscv-qmp-cmds.c
> index 69dde0c3e7..ee7465297d 100644
> --- a/target/riscv/riscv-qmp-cmds.c
> +++ b/target/riscv/riscv-qmp-cmds.c
> @@ -129,6 +129,7 @@ static void riscv_obj_add_profiles_qdict(Object *obj, QDict *qdict_out)
> }
>
> static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
> + const char *props_arg_name,
> Error **errp)
> {
> const QDict *qdict_in;
> @@ -137,7 +138,7 @@ static void riscv_cpuobj_validate_qdict_in(Object *obj, QObject *props,
> Error *local_err = NULL;
>
> visitor = qobject_input_visitor_new(props);
> - if (!visit_start_struct(visitor, "props", NULL, 0, &local_err)) {
> + if (!visit_start_struct(visitor, props_arg_name, NULL, 0, &local_err)) {
> goto err;
> }
>
> @@ -198,7 +199,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> }
>
> if (model->props) {
> - riscv_cpuobj_validate_qdict_in(obj, model->props, &local_err);
> + riscv_cpuobj_validate_qdict_in(obj, model->props, "model.props",
> + &local_err);
> if (local_err) {
> error_propagate(errp, local_err);
> object_unref(obj);
> diff --git a/target/s390x/cpu_models_sysemu.c b/target/s390x/cpu_models_sysemu.c
> index ef19724888..8338084d00 100644
> --- a/target/s390x/cpu_models_sysemu.c
> +++ b/target/s390x/cpu_models_sysemu.c
> @@ -98,7 +98,7 @@ CpuDefinitionInfoList *qmp_query_cpu_definitions(Error **errp)
> }
>
> static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
> - Error **errp)
> + const char *info_arg_name, Error **errp)
> {
> Error *err = NULL;
> const QDict *qdict;
> @@ -128,8 +128,11 @@ static void cpu_model_from_info(S390CPUModel *model, const CpuModelInfo *info,
> }
>
> if (info->props) {
> + g_autofree const char *props_name = g_strdup_printf("%s.props",
> + info_arg_name);
> +
> visitor = qobject_input_visitor_new(info->props);
> - if (!visit_start_struct(visitor, "props", NULL, 0, errp)) {
> + if (!visit_start_struct(visitor, props_name, NULL, 0, errp)) {
> visit_free(visitor);
> object_unref(obj);
> return;
> @@ -216,7 +219,7 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
> bool delta_changes = false;
>
> /* convert it to our internal representation */
> - cpu_model_from_info(&s390_model, model, &err);
> + cpu_model_from_info(&s390_model, model, "model", &err);
> if (err) {
> error_propagate(errp, err);
> return NULL;
> @@ -254,12 +257,12 @@ CpuModelCompareInfo *qmp_query_cpu_model_comparison(CpuModelInfo *infoa,
> S390CPUModel modela, modelb;
>
> /* convert both models to our internal representation */
> - cpu_model_from_info(&modela, infoa, &err);
> + cpu_model_from_info(&modela, infoa, "modela", &err);
> if (err) {
> error_propagate(errp, err);
> return NULL;
> }
> - cpu_model_from_info(&modelb, infob, &err);
> + cpu_model_from_info(&modelb, infob, "modelb", &err);
> if (err) {
> error_propagate(errp, err);
> return NULL;
> @@ -331,13 +334,13 @@ CpuModelBaselineInfo *qmp_query_cpu_model_baseline(CpuModelInfo *infoa,
> uint8_t max_gen;
>
> /* convert both models to our internal representation */
> - cpu_model_from_info(&modela, infoa, &err);
> + cpu_model_from_info(&modela, infoa, "modela", &err);
> if (err) {
> error_propagate(errp, err);
> return NULL;
> }
>
> - cpu_model_from_info(&modelb, infob, &err);
> + cpu_model_from_info(&modelb, infob, "modelb", &err);
> if (err) {
> error_propagate(errp, err);
> return NULL;
> diff --git a/tests/qtest/arm-cpu-features.c b/tests/qtest/arm-cpu-features.c
> index 1daceb2e31..9d6e6190d5 100644
> --- a/tests/qtest/arm-cpu-features.c
> +++ b/tests/qtest/arm-cpu-features.c
> @@ -213,7 +213,8 @@ static void assert_bad_props(QTestState *qts, const char *cpu_type)
> error = resp_get_error(resp);
> g_assert(error);
> g_assert_cmpstr(error, ==,
> - "Invalid parameter type for 'props', expected: object");
> + "Invalid parameter type for 'model.props',"
> + " expected: object");
> qobject_unref(resp);
> }
>
> @@ -446,7 +447,7 @@ static void test_query_cpu_model_expansion(const void *data)
> assert_bad_props(qts, "max");
> assert_error(qts, "foo", "The CPU type 'foo' is not a recognized "
> "ARM CPU type", NULL);
> - assert_error(qts, "max", "Parameter 'props.not-a-prop' is unexpected",
> + assert_error(qts, "max", "Parameter 'model.props.not-a-prop' is unexpected",
> "{ 'not-a-prop': false }");
> assert_error(qts, "host", "The CPU type 'host' requires KVM", NULL);
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 4/4] target/loongarch: Fix query-cpu-model-expansion to reject props
2024-03-05 14:59 [PATCH 0/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
` (2 preceding siblings ...)
2024-03-05 14:59 ` [PATCH 3/4] target: Improve error reporting for CpuModelInfo member @props Markus Armbruster
@ 2024-03-05 14:59 ` Markus Armbruster
3 siblings, 0 replies; 7+ messages in thread
From: Markus Armbruster @ 2024-03-05 14:59 UTC (permalink / raw)
To: qemu-devel
Cc: peter.maydell, pbonzini, gaosong, palmer, alistair.francis,
bin.meng, liwei1518, dbarboza, zhiwei_liu, richard.henderson,
david, iii, thuth, qemu-arm, qemu-riscv, qemu-s390x
query-cpu-model-expansion takes a CpuModelInfo argument. The
loongarch version of the command silently ignores the argument's
member @props. For instance,
{"execute": "query-cpu-model-expansion", "arguments": {"type": "static", "model": {"name": "la464", "props": null}}}
and
{"execute": "query-cpu-model-expansion", "arguments": {"type": "static", "model": {"name": "la464", "props": {"prop": null}}}}
succeed.
Add skeleton code for property processing that recognizes no
properties. Now the two commands fail as they should:
{"error": {"class": "GenericError", "desc": "Invalid parameter type for 'model.props', expected: object"}}
and
{"error": {"class": "GenericError", "desc": "Parameter 'model.props.prop' is unexpected"}}
Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
target/loongarch/loongarch-qmp-cmds.c | 17 +++++++++++++++++
1 file changed, 17 insertions(+)
diff --git a/target/loongarch/loongarch-qmp-cmds.c b/target/loongarch/loongarch-qmp-cmds.c
index ec33ce81f0..44d1f089b6 100644
--- a/target/loongarch/loongarch-qmp-cmds.c
+++ b/target/loongarch/loongarch-qmp-cmds.c
@@ -48,6 +48,8 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
CpuModelInfo *model,
Error **errp)
{
+ Visitor *visitor;
+ bool ok;
CpuModelExpansionInfo *expansion_info;
QDict *qdict_out;
ObjectClass *oc;
@@ -60,6 +62,21 @@ CpuModelExpansionInfo *qmp_query_cpu_model_expansion(CpuModelExpansionType type,
return NULL;
}
+ if (model->props) {
+ visitor = qobject_input_visitor_new(model->props);
+ if (!visit_start_struct(visitor, "model.props", NULL, 0, errp)) {
+ visit_free(visitor);
+ return NULL;
+ }
+
+ ok = visit_check_struct(visitor, errp);
+ visit_end_struct(visitor, NULL);
+ visit_free(visitor);
+ if (!ok) {
+ return NULL;
+ }
+ }
+
oc = cpu_class_by_name(TYPE_LOONGARCH_CPU, model->name);
if (!oc) {
error_setg(errp, "The CPU type '%s' is not a recognized LoongArch CPU type",
--
2.44.0
^ permalink raw reply related [flat|nested] 7+ messages in thread