Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
Philippe Mathieu-Daudé writes:
> On 10/26/21 11:46, Markus Armbruster wrote:
>> Philippe Mathieu-Daudé writes:
>>
>>> On 10/25/21 07:25, Markus Armbruster wrote:
The code to check policy for handling deprecated input is triplicated.
Factor it out into compat_policy_input_ok() before I mess with it in
the next commit.
Signed-off-by: Markus Armbruster
---
include/qapi/compat-policy.h | 7 +
qapi/qapi-visit-core.c | 18 +
qapi/qmp-dispatch.c | 51 +++-
qapi/qobject-input-visitor.c | 19 +++---
4 files changed, 55 insertions(+), 40 deletions(-)
>>>
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 8cca18c891..e29ade134c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -28,6 +28,40 @@
CompatPolicy compat_policy;
+static bool compat_policy_input_ok1(const char *adjective,
+CompatPolicyInput policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp)
+{
+switch (policy) {
+case COMPAT_POLICY_INPUT_ACCEPT:
+return true;
+case COMPAT_POLICY_INPUT_REJECT:
+error_set(errp, error_class, "%s %s %s disabled by policy",
+ adjective, kind, name);
+return false;
+case COMPAT_POLICY_INPUT_CRASH:
+default:
+abort();
>>>
>>> g_assert_not_reached() provides a nicer user experience.
>>
>> I find it hard to care for making the experience of a crash that should
>> never ever happen nicer :)
>
> Well COMPAT_POLICY_INPUT_CRASH can happen... What about:
Point.
>case COMPAT_POLICY_INPUT_CRASH:
>error_printf("%s %s %s disabled by policy",
> adjective, kind, name);
>abort();
>default:
>g_assert_not_reached();
Separate patch. I'd prefer to delay it a bit, to avoid rocking the boat
so close to the soft freeze.
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
On 10/26/21 11:46, Markus Armbruster wrote:
> Philippe Mathieu-Daudé writes:
>
>> On 10/25/21 07:25, Markus Armbruster wrote:
>>> The code to check policy for handling deprecated input is triplicated.
>>> Factor it out into compat_policy_input_ok() before I mess with it in
>>> the next commit.
>>>
>>> Signed-off-by: Markus Armbruster
>>> ---
>>> include/qapi/compat-policy.h | 7 +
>>> qapi/qapi-visit-core.c | 18 +
>>> qapi/qmp-dispatch.c | 51 +++-
>>> qapi/qobject-input-visitor.c | 19 +++---
>>> 4 files changed, 55 insertions(+), 40 deletions(-)
>>
>>> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>>> index 8cca18c891..e29ade134c 100644
>>> --- a/qapi/qmp-dispatch.c
>>> +++ b/qapi/qmp-dispatch.c
>>> @@ -28,6 +28,40 @@
>>>
>>> CompatPolicy compat_policy;
>>>
>>> +static bool compat_policy_input_ok1(const char *adjective,
>>> +CompatPolicyInput policy,
>>> +ErrorClass error_class,
>>> +const char *kind, const char *name,
>>> +Error **errp)
>>> +{
>>> +switch (policy) {
>>> +case COMPAT_POLICY_INPUT_ACCEPT:
>>> +return true;
>>> +case COMPAT_POLICY_INPUT_REJECT:
>>> +error_set(errp, error_class, "%s %s %s disabled by policy",
>>> + adjective, kind, name);
>>> +return false;
>>> +case COMPAT_POLICY_INPUT_CRASH:
>>> +default:
>>> +abort();
>>
>> g_assert_not_reached() provides a nicer user experience.
>
> I find it hard to care for making the experience of a crash that should
> never ever happen nicer :)
Well COMPAT_POLICY_INPUT_CRASH can happen... What about:
case COMPAT_POLICY_INPUT_CRASH:
error_printf("%s %s %s disabled by policy",
adjective, kind, name);
abort();
default:
g_assert_not_reached();
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
Philippe Mathieu-Daudé writes:
> On 10/25/21 07:25, Markus Armbruster wrote:
>> The code to check policy for handling deprecated input is triplicated.
>> Factor it out into compat_policy_input_ok() before I mess with it in
>> the next commit.
>>
>> Signed-off-by: Markus Armbruster
>> ---
>> include/qapi/compat-policy.h | 7 +
>> qapi/qapi-visit-core.c | 18 +
>> qapi/qmp-dispatch.c | 51 +++-
>> qapi/qobject-input-visitor.c | 19 +++---
>> 4 files changed, 55 insertions(+), 40 deletions(-)
>
>> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
>> index 8cca18c891..e29ade134c 100644
>> --- a/qapi/qmp-dispatch.c
>> +++ b/qapi/qmp-dispatch.c
>> @@ -28,6 +28,40 @@
>>
>> CompatPolicy compat_policy;
>>
>> +static bool compat_policy_input_ok1(const char *adjective,
>> +CompatPolicyInput policy,
>> +ErrorClass error_class,
>> +const char *kind, const char *name,
>> +Error **errp)
>> +{
>> +switch (policy) {
>> +case COMPAT_POLICY_INPUT_ACCEPT:
>> +return true;
>> +case COMPAT_POLICY_INPUT_REJECT:
>> +error_set(errp, error_class, "%s %s %s disabled by policy",
>> + adjective, kind, name);
>> +return false;
>> +case COMPAT_POLICY_INPUT_CRASH:
>> +default:
>> +abort();
>
> g_assert_not_reached() provides a nicer user experience.
I find it hard to care for making the experience of a crash that should
never ever happen nicer :)
>> +}
>> +}
>> +
>> +bool compat_policy_input_ok(unsigned special_features,
>> +const CompatPolicy *policy,
>> +ErrorClass error_class,
>> +const char *kind, const char *name,
>> +Error **errp)
>> +{
>> +if ((special_features & 1u << QAPI_DEPRECATED)
>
> Matter of taste, I find code using extract() easier to review:
>
> extract64(special_features, QAPI_DEPRECATED, 1)
I agree for width > 1.
>> +&& !compat_policy_input_ok1("Deprecated",
>> +policy->deprecated_input,
>> +error_class, kind, name, errp)) {
>> +return false;
>> +}
>> +return true;
>> +}
>
> Reviewed-by: Philippe Mathieu-Daudé
Thanks!
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
On Mon, Oct 25, 2021 at 1:25 AM Markus Armbruster wrote: > The code to check policy for handling deprecated input is triplicated. > Factor it out into compat_policy_input_ok() before I mess with it in > the next commit. > > Signed-off-by: Markus Armbruster > (Skipping C-only patches for quick review. I'll trust you on these.) --js
Re: [PATCH 8/9] qapi: Factor out compat_policy_input_ok()
On 10/25/21 07:25, Markus Armbruster wrote:
> The code to check policy for handling deprecated input is triplicated.
> Factor it out into compat_policy_input_ok() before I mess with it in
> the next commit.
>
> Signed-off-by: Markus Armbruster
> ---
> include/qapi/compat-policy.h | 7 +
> qapi/qapi-visit-core.c | 18 +
> qapi/qmp-dispatch.c | 51 +++-
> qapi/qobject-input-visitor.c | 19 +++---
> 4 files changed, 55 insertions(+), 40 deletions(-)
> diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
> index 8cca18c891..e29ade134c 100644
> --- a/qapi/qmp-dispatch.c
> +++ b/qapi/qmp-dispatch.c
> @@ -28,6 +28,40 @@
>
> CompatPolicy compat_policy;
>
> +static bool compat_policy_input_ok1(const char *adjective,
> +CompatPolicyInput policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp)
> +{
> +switch (policy) {
> +case COMPAT_POLICY_INPUT_ACCEPT:
> +return true;
> +case COMPAT_POLICY_INPUT_REJECT:
> +error_set(errp, error_class, "%s %s %s disabled by policy",
> + adjective, kind, name);
> +return false;
> +case COMPAT_POLICY_INPUT_CRASH:
> +default:
> +abort();
g_assert_not_reached() provides a nicer user experience.
> +}
> +}
> +
> +bool compat_policy_input_ok(unsigned special_features,
> +const CompatPolicy *policy,
> +ErrorClass error_class,
> +const char *kind, const char *name,
> +Error **errp)
> +{
> +if ((special_features & 1u << QAPI_DEPRECATED)
Matter of taste, I find code using extract() easier to review:
extract64(special_features, QAPI_DEPRECATED, 1)
> +&& !compat_policy_input_ok1("Deprecated",
> +policy->deprecated_input,
> +error_class, kind, name, errp)) {
> +return false;
> +}
> +return true;
> +}
Reviewed-by: Philippe Mathieu-Daudé
[PATCH 8/9] qapi: Factor out compat_policy_input_ok()
The code to check policy for handling deprecated input is triplicated.
Factor it out into compat_policy_input_ok() before I mess with it in
the next commit.
Signed-off-by: Markus Armbruster
---
include/qapi/compat-policy.h | 7 +
qapi/qapi-visit-core.c | 18 +
qapi/qmp-dispatch.c | 51 +++-
qapi/qobject-input-visitor.c | 19 +++---
4 files changed, 55 insertions(+), 40 deletions(-)
diff --git a/include/qapi/compat-policy.h b/include/qapi/compat-policy.h
index 1083f95122..8b7b25c0b5 100644
--- a/include/qapi/compat-policy.h
+++ b/include/qapi/compat-policy.h
@@ -13,10 +13,17 @@
#ifndef QAPI_COMPAT_POLICY_H
#define QAPI_COMPAT_POLICY_H
+#include "qapi/error.h"
#include "qapi/qapi-types-compat.h"
extern CompatPolicy compat_policy;
+bool compat_policy_input_ok(unsigned special_features,
+const CompatPolicy *policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp);
+
/*
* Create a QObject input visitor for @obj for use with QMP
*
diff --git a/qapi/qapi-visit-core.c b/qapi/qapi-visit-core.c
index 5572d90efb..a1ddfe8831 100644
--- a/qapi/qapi-visit-core.c
+++ b/qapi/qapi-visit-core.c
@@ -13,6 +13,7 @@
*/
#include "qemu/osdep.h"
+#include "qapi/compat-policy.h"
#include "qapi/error.h"
#include "qapi/qmp/qerror.h"
#include "qapi/visitor.h"
@@ -408,18 +409,11 @@ static bool input_type_enum(Visitor *v, const char *name,
int *obj,
}
if (lookup->special_features
-&& (lookup->special_features[value] & QAPI_DEPRECATED)) {
-switch (v->compat_policy.deprecated_input) {
-case COMPAT_POLICY_INPUT_ACCEPT:
-break;
-case COMPAT_POLICY_INPUT_REJECT:
-error_setg(errp, "Deprecated value '%s' disabled by policy",
- enum_str);
-return false;
-case COMPAT_POLICY_INPUT_CRASH:
-default:
-abort();
-}
+&& !compat_policy_input_ok(lookup->special_features[value],
+ >compat_policy,
+ ERROR_CLASS_GENERIC_ERROR,
+ "value", enum_str, errp)) {
+return false;
}
*obj = value;
diff --git a/qapi/qmp-dispatch.c b/qapi/qmp-dispatch.c
index 8cca18c891..e29ade134c 100644
--- a/qapi/qmp-dispatch.c
+++ b/qapi/qmp-dispatch.c
@@ -28,6 +28,40 @@
CompatPolicy compat_policy;
+static bool compat_policy_input_ok1(const char *adjective,
+CompatPolicyInput policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp)
+{
+switch (policy) {
+case COMPAT_POLICY_INPUT_ACCEPT:
+return true;
+case COMPAT_POLICY_INPUT_REJECT:
+error_set(errp, error_class, "%s %s %s disabled by policy",
+ adjective, kind, name);
+return false;
+case COMPAT_POLICY_INPUT_CRASH:
+default:
+abort();
+}
+}
+
+bool compat_policy_input_ok(unsigned special_features,
+const CompatPolicy *policy,
+ErrorClass error_class,
+const char *kind, const char *name,
+Error **errp)
+{
+if ((special_features & 1u << QAPI_DEPRECATED)
+&& !compat_policy_input_ok1("Deprecated",
+policy->deprecated_input,
+error_class, kind, name, errp)) {
+return false;
+}
+return true;
+}
+
Visitor *qobject_input_visitor_new_qmp(QObject *obj)
{
Visitor *v = qobject_input_visitor_new(obj);
@@ -176,19 +210,10 @@ QDict *qmp_dispatch(const QmpCommandList *cmds, QObject
*request,
"The command %s has not been found", command);
goto out;
}
-if (cmd->special_features & 1u << QAPI_DEPRECATED) {
-switch (compat_policy.deprecated_input) {
-case COMPAT_POLICY_INPUT_ACCEPT:
-break;
-case COMPAT_POLICY_INPUT_REJECT:
-error_set(, ERROR_CLASS_COMMAND_NOT_FOUND,
- "Deprecated command %s disabled by policy",
- command);
-goto out;
-case COMPAT_POLICY_INPUT_CRASH:
-default:
-abort();
-}
+if (!compat_policy_input_ok(cmd->special_features, _policy,
+ERROR_CLASS_COMMAND_NOT_FOUND,
+"command", command, )) {
+goto out;
}
if (!cmd->enabled) {
error_set(, ERROR_CLASS_COMMAND_NOT_FOUND,
diff --git a/qapi/qobject-input-visitor.c b/qapi/qobject-input-visitor.c
index fda485614b..f0b4c7ca9d 100644
--- a/qapi/qobject-input-visitor.c
+++
