Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()
Am 13.03.2021 um 09:41 hat Markus Armbruster geschrieben: > Observation, not objection: > > 1. QMP core parses JSON text into QObject, passes to generated >marshaller. > > 2. Marshaller converts QObject to ObjectOptions with the QObject input >visitor, passes to qmp_object_add(). > > 3. qmp_object_add() wraps around user_creatable_add_qapi(). > > 4. user_creatable_add_qapi() converts right back to QObject with the >QObject output visitor. It splits the result into qom_type, id and >the rest, and passes all three to user_creatable_add_type(). > > 5. user_creatable_add_type() performs a virtual visit with the QObject >input visitor. The outermost object it visits itself, its children >it visits by calling object_property_set(). > > I sure hope we wouldn't write it this way from scratch :) > > I think your patch is a reasonable step towards a QOM that is at peace > with QAPI. But there's plenty of work left. Yes, obviously the conversion back to QObject is not great. There are two reasons why we currently need it: 1. user_creatable_add_type() wants to iterate over all properties without knowing which properties exist. This should be fixed by not visiting the top level in user_creatable_add_type(), but moving this part to object-specific code (in the final state probably code generated from the QAPI schema). 2. We have ObjectOptions, but need to pass a visitor. We don't have a general purpose visitor that visits both sides. The clone visitor seems somewhat similar to what we need, but I seem to remember it was more restricted to its particular use case. Kevin
Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()
On 13/03/21 09:41, Markus Armbruster wrote: Observation, not objection: 1. QMP core parses JSON text into QObject, passes to generated marshaller. 2. Marshaller converts QObject to ObjectOptions with the QObject input visitor, passes to qmp_object_add(). 3. qmp_object_add() wraps around user_creatable_add_qapi(). 4. user_creatable_add_qapi() converts right back to QObject with the QObject output visitor. It splits the result into qom_type, id and the rest, and passes all three to user_creatable_add_type(). 5. user_creatable_add_type() performs a virtual visit with the QObject input visitor. The outermost object it visits itself, its children it visits by calling object_property_set(). I sure hope we wouldn't write it this way from scratch:) All problems in computer science ca be solved by adding another level of indirection, except those that can be solved by adding two more levels of indirection. I think your patch is a reasonable step towards a QOM that is at peace with QAPI. But there's plenty of work left. Absolutely, see https://wiki.qemu.org/Features/QOM-QAPI_integration for some brainstorming about it. Paolo
Re: [PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()
Kevin Wolf writes:
> The implementation for --object can be shared between
> qemu-storage-daemon and other binaries, so move it into a function in
> qom/object_interfaces.c that is accessible from everywhere.
>
> This also requires moving the implementation of qmp_object_add() into a
> new user_creatable_add_qapi(), because qom/qom-qmp-cmds.c is not linked
> for tools.
>
> user_creatable_print_help_from_qdict() can become static now.
>
> Signed-off-by: Kevin Wolf
> Acked-by: Peter Krempa
> Reviewed-by: Eric Blake
> ---
> include/qom/object_interfaces.h | 41 +++
> qom/object_interfaces.c | 50 +++-
> qom/qom-qmp-cmds.c | 20 +--
> storage-daemon/qemu-storage-daemon.c | 24 ++---
> 4 files changed, 80 insertions(+), 55 deletions(-)
>
> diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
> index 5299603f50..1e6c51b541 100644
> --- a/include/qom/object_interfaces.h
> +++ b/include/qom/object_interfaces.h
> @@ -2,6 +2,7 @@
> #define OBJECT_INTERFACES_H
>
> #include "qom/object.h"
> +#include "qapi/qapi-types-qom.h"
> #include "qapi/visitor.h"
>
> #define TYPE_USER_CREATABLE "user-creatable"
> @@ -86,6 +87,18 @@ Object *user_creatable_add_type(const char *type, const
> char *id,
> const QDict *qdict,
> Visitor *v, Error **errp);
>
> +/**
> + * user_creatable_add_qapi:
> + * @options: the object definition
> + * @errp: if an error occurs, a pointer to an area to store the error
> + *
> + * Create an instance of the user creatable object according to the
> + * options passed in @opts as described in the QAPI schema documentation.
> + *
> + * Returns: the newly created object or NULL on error
> + */
> +void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
> +
> /**
> * user_creatable_add_opts:
> * @opts: the object definition
> @@ -131,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const
> char *type);
> int user_creatable_add_opts_foreach(void *opaque,
> QemuOpts *opts, Error **errp);
>
> +/**
> + * user_creatable_process_cmdline:
> + * @optarg: the object definition string as passed on the command line
> + *
> + * Create an instance of the user creatable object by parsing optarg
> + * with a keyval parser and implicit key 'qom-type', converting the
> + * result to ObjectOptions and calling into qmp_object_add().
> + *
> + * If a help option is given, print help instead and exit.
> + *
> + * This function is only meant to be called during command line parsing.
> + * It exits the process on failure or after printing help.
> + */
> +void user_creatable_process_cmdline(const char *optarg);
> +
> /**
> * user_creatable_print_help:
> * @type: the QOM type to be added
> @@ -145,19 +173,6 @@ int user_creatable_add_opts_foreach(void *opaque,
> */
> bool user_creatable_print_help(const char *type, QemuOpts *opts);
>
> -/**
> - * user_creatable_print_help_from_qdict:
> - * @args: options to create
> - *
> - * Prints help considering the other options given in @args (if "qom-type" is
> - * given and valid, print properties for the type, otherwise print valid
> types)
> - *
> - * In contrast to user_creatable_print_help(), this function can't return
> that
> - * no help was requested. It should only be called if we know that help is
> - * requested and it will always print some help.
> - */
> -void user_creatable_print_help_from_qdict(QDict *args);
> -
> /**
> * user_creatable_del:
> * @id: the unique ID for the object
> diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
> index 02c3934329..2eaf9971f5 100644
> --- a/qom/object_interfaces.c
> +++ b/qom/object_interfaces.c
> @@ -2,10 +2,13 @@
>
> #include "qemu/cutils.h"
> #include "qapi/error.h"
> +#include "qapi/qapi-commands-qom.h"
> +#include "qapi/qapi-visit-qom.h"
> #include "qapi/qmp/qdict.h"
> #include "qapi/qmp/qerror.h"
> #include "qapi/qmp/qjson.h"
> #include "qapi/qobject-input-visitor.h"
> +#include "qapi/qobject-output-visitor.h"
> #include "qom/object_interfaces.h"
> #include "qemu/help_option.h"
> #include "qemu/id.h"
> @@ -113,6 +116,29 @@ out:
> return obj;
> }
>
> +void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
> +{
> +Visitor *v;
> +QObject *qobj;
> +QDict *props;
> +Object *obj;
> +
> +v = qobject_output_visitor_new();
> +visit_type_ObjectOptions(v, NULL, , _abort);
> +visit_complete(v, );
> +visit_free(v);
> +
> +props = qobject_to(QDict, qobj);
> +qdict_del(props, "qom-type");
> +qdict_del(props, "id");
> +
> +v = qobject_input_visitor_new(QOBJECT(props));
> +obj = user_creatable_add_type(ObjectType_str(options->qom_type),
> + options->id, props, v, errp);
> +object_unref(obj);
> +visit_free(v);
> +}
> +
Observation,
[PATCH v3 22/30] qom: Factor out user_creatable_process_cmdline()
The implementation for --object can be shared between
qemu-storage-daemon and other binaries, so move it into a function in
qom/object_interfaces.c that is accessible from everywhere.
This also requires moving the implementation of qmp_object_add() into a
new user_creatable_add_qapi(), because qom/qom-qmp-cmds.c is not linked
for tools.
user_creatable_print_help_from_qdict() can become static now.
Signed-off-by: Kevin Wolf
Acked-by: Peter Krempa
Reviewed-by: Eric Blake
---
include/qom/object_interfaces.h | 41 +++
qom/object_interfaces.c | 50 +++-
qom/qom-qmp-cmds.c | 20 +--
storage-daemon/qemu-storage-daemon.c | 24 ++---
4 files changed, 80 insertions(+), 55 deletions(-)
diff --git a/include/qom/object_interfaces.h b/include/qom/object_interfaces.h
index 5299603f50..1e6c51b541 100644
--- a/include/qom/object_interfaces.h
+++ b/include/qom/object_interfaces.h
@@ -2,6 +2,7 @@
#define OBJECT_INTERFACES_H
#include "qom/object.h"
+#include "qapi/qapi-types-qom.h"
#include "qapi/visitor.h"
#define TYPE_USER_CREATABLE "user-creatable"
@@ -86,6 +87,18 @@ Object *user_creatable_add_type(const char *type, const char
*id,
const QDict *qdict,
Visitor *v, Error **errp);
+/**
+ * user_creatable_add_qapi:
+ * @options: the object definition
+ * @errp: if an error occurs, a pointer to an area to store the error
+ *
+ * Create an instance of the user creatable object according to the
+ * options passed in @opts as described in the QAPI schema documentation.
+ *
+ * Returns: the newly created object or NULL on error
+ */
+void user_creatable_add_qapi(ObjectOptions *options, Error **errp);
+
/**
* user_creatable_add_opts:
* @opts: the object definition
@@ -131,6 +144,21 @@ typedef bool (*user_creatable_add_opts_predicate)(const
char *type);
int user_creatable_add_opts_foreach(void *opaque,
QemuOpts *opts, Error **errp);
+/**
+ * user_creatable_process_cmdline:
+ * @optarg: the object definition string as passed on the command line
+ *
+ * Create an instance of the user creatable object by parsing optarg
+ * with a keyval parser and implicit key 'qom-type', converting the
+ * result to ObjectOptions and calling into qmp_object_add().
+ *
+ * If a help option is given, print help instead and exit.
+ *
+ * This function is only meant to be called during command line parsing.
+ * It exits the process on failure or after printing help.
+ */
+void user_creatable_process_cmdline(const char *optarg);
+
/**
* user_creatable_print_help:
* @type: the QOM type to be added
@@ -145,19 +173,6 @@ int user_creatable_add_opts_foreach(void *opaque,
*/
bool user_creatable_print_help(const char *type, QemuOpts *opts);
-/**
- * user_creatable_print_help_from_qdict:
- * @args: options to create
- *
- * Prints help considering the other options given in @args (if "qom-type" is
- * given and valid, print properties for the type, otherwise print valid types)
- *
- * In contrast to user_creatable_print_help(), this function can't return that
- * no help was requested. It should only be called if we know that help is
- * requested and it will always print some help.
- */
-void user_creatable_print_help_from_qdict(QDict *args);
-
/**
* user_creatable_del:
* @id: the unique ID for the object
diff --git a/qom/object_interfaces.c b/qom/object_interfaces.c
index 02c3934329..2eaf9971f5 100644
--- a/qom/object_interfaces.c
+++ b/qom/object_interfaces.c
@@ -2,10 +2,13 @@
#include "qemu/cutils.h"
#include "qapi/error.h"
+#include "qapi/qapi-commands-qom.h"
+#include "qapi/qapi-visit-qom.h"
#include "qapi/qmp/qdict.h"
#include "qapi/qmp/qerror.h"
#include "qapi/qmp/qjson.h"
#include "qapi/qobject-input-visitor.h"
+#include "qapi/qobject-output-visitor.h"
#include "qom/object_interfaces.h"
#include "qemu/help_option.h"
#include "qemu/id.h"
@@ -113,6 +116,29 @@ out:
return obj;
}
+void user_creatable_add_qapi(ObjectOptions *options, Error **errp)
+{
+Visitor *v;
+QObject *qobj;
+QDict *props;
+Object *obj;
+
+v = qobject_output_visitor_new();
+visit_type_ObjectOptions(v, NULL, , _abort);
+visit_complete(v, );
+visit_free(v);
+
+props = qobject_to(QDict, qobj);
+qdict_del(props, "qom-type");
+qdict_del(props, "id");
+
+v = qobject_input_visitor_new(QOBJECT(props));
+obj = user_creatable_add_type(ObjectType_str(options->qom_type),
+ options->id, props, v, errp);
+object_unref(obj);
+visit_free(v);
+}
+
Object *user_creatable_add_opts(QemuOpts *opts, Error **errp)
{
Visitor *v;
@@ -256,7 +282,7 @@ bool user_creatable_print_help(const char *type, QemuOpts
*opts)
return false;
}
-void user_creatable_print_help_from_qdict(QDict *args)
+static void user_creatable_print_help_from_qdict(QDict *args)
{
