| Message ID | 1328201142-26145-5-git-send-email-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
On 02/02/2012 10:45 AM, Paolo Bonzini wrote: > Move the creation of QmpInputVisitor and QmpOutputVisitor from > qmp.c to qom/object.c, since it's the only practical way to access > object properties. > > Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> > --- > include/qemu/object.h | 24 ++++++++++++++++++++++++ > qmp.c | 17 ++--------------- > qom/object.c | 29 +++++++++++++++++++++++++++++ > 3 files changed, 55 insertions(+), 15 deletions(-) I don't want object.h to have a dependency on QObject. We need to phase out QObject. Couple things: 1) We shouldn't use generic interfaces to read/write properties from objects. We should use type-safe accessors provided by the types themselves. 2) If we want to get fancy, we can add property_set_int, etc. and then implement (1) via header files that just call these functions. Regards, Anthony Liguori > > diff --git a/include/qemu/object.h b/include/qemu/object.h > index 947cf29..71090f2 100644 > --- a/include/qemu/object.h > +++ b/include/qemu/object.h > @@ -542,6 +542,18 @@ void object_property_get(Object *obj, struct Visitor *v, const char *name, > struct Error **errp); > > /** > + * object_property_get_qobject: > + * @obj: the object > + * @name: the name of the property > + * @errp: returns an error if this function fails > + * > + * Returns: the value of the property, converted to QObject, or NULL if > + * an error occurs. > + */ > +struct QObject *object_property_get_qobject(Object *obj, const char *name, > + struct Error **errp); > + > +/** > * object_property_set: > * @obj: the object > * @v: the visitor that will be used to write the property value. This should > @@ -556,6 +568,18 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name, > struct Error **errp); > > /** > + * object_property_set_qobject: > + * @obj: the object > + * @ret: The value that will be written to the property. > + * @name: the name of the property > + * @errp: returns an error if this function fails > + * > + * Writes a property to a object. > + */ > +void object_property_set_qobject(Object *obj, struct QObject *qobj, > + const char *name, struct Error **errp); > + > +/** > * @object_property_get_type: > * @obj: the object > * @name: the name of the property > diff --git a/qmp.c b/qmp.c > index 45052cc..c7a81cc 100644 > --- a/qmp.c > +++ b/qmp.c > @@ -21,8 +21,6 @@ > #include "kvm.h" > #include "arch_init.h" > #include "hw/qdev.h" > -#include "qapi/qmp-input-visitor.h" > -#include "qapi/qmp-output-visitor.h" > #include "blockdev.h" > > NameInfo *qmp_query_name(Error **errp) > @@ -198,7 +196,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret) > const char *property = qdict_get_str(qdict, "property"); > QObject *value = qdict_get(qdict, "value"); > Error *local_err = NULL; > - QmpInputVisitor *mi; > Object *obj; > > obj = object_resolve_path(path, NULL); > @@ -207,10 +204,7 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret) > goto out; > } > > - mi = qmp_input_visitor_new(value); > - object_property_set(obj, qmp_input_get_visitor(mi), property,&local_err); > - > - qmp_input_visitor_cleanup(mi); > + object_property_set_qobject(obj, value, property,&local_err); > > out: > if (local_err) { > @@ -227,7 +221,6 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret) > const char *path = qdict_get_str(qdict, "path"); > const char *property = qdict_get_str(qdict, "property"); > Error *local_err = NULL; > - QmpOutputVisitor *mo; > Object *obj; > > obj = object_resolve_path(path, NULL); > @@ -236,13 +229,7 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret) > goto out; > } > > - mo = qmp_output_visitor_new(); > - object_property_get(obj, qmp_output_get_visitor(mo), property,&local_err); > - if (!local_err) { > - *ret = qmp_output_get_qobject(mo); > - } > - > - qmp_output_visitor_cleanup(mo); > + *ret = object_property_get_qobject(obj, property,&local_err); > > out: > if (local_err) { > diff --git a/qom/object.c b/qom/object.c > index 299e146..13c8bec 100644 > --- a/qom/object.c > +++ b/qom/object.c > @@ -13,6 +13,8 @@ > #include "qemu/object.h" > #include "qemu-common.h" > #include "qapi/qapi-visit-core.h" > +#include "qapi/qmp-input-visitor.h" > +#include "qapi/qmp-output-visitor.h" > > #define MAX_INTERFACES 32 > > @@ -646,6 +648,33 @@ void object_property_set(Object *obj, Visitor *v, const char *name, > } > } > > +void object_property_set_qobject(Object *obj, QObject *value, > + const char *name, Error **errp) > +{ > + QmpInputVisitor *mi; > + mi = qmp_input_visitor_new(value); > + object_property_set(obj, qmp_input_get_visitor(mi), name, errp); > + > + qmp_input_visitor_cleanup(mi); > +} > + > +QObject *object_property_get_qobject(Object *obj, const char *name, > + Error **errp) > +{ > + QObject *ret = NULL; > + Error *local_err = NULL; > + QmpOutputVisitor *mo; > + > + mo = qmp_output_visitor_new(); > + object_property_get(obj, qmp_output_get_visitor(mo), name,&local_err); > + if (!local_err) { > + ret = qmp_output_get_qobject(mo); > + } > + error_propagate(errp, local_err); > + qmp_output_visitor_cleanup(mo); > + return ret; > +} > + > const char *object_property_get_type(Object *obj, const char *name, Error **errp) > { > ObjectProperty *prop = object_property_find(obj, name);
Am 02.02.2012 20:06, schrieb Anthony Liguori: > On 02/02/2012 10:45 AM, Paolo Bonzini wrote: >> Move the creation of QmpInputVisitor and QmpOutputVisitor from >> qmp.c to qom/object.c, since it's the only practical way to access >> object properties. >> >> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> >> --- >> include/qemu/object.h | 24 ++++++++++++++++++++++++ >> qmp.c | 17 ++--------------- >> qom/object.c | 29 +++++++++++++++++++++++++++++ >> 3 files changed, 55 insertions(+), 15 deletions(-) > > I don't want object.h to have a dependency on QObject. We need to phase > out QObject. We did get that dependency though by your move of the property code to object.c. As you will see shortly, we now need qobject-obj-y and qapi-obj-y plus some stubs to make the user emulators compile with QOM. Andreas
On 02/02/2012 08:06 PM, Anthony Liguori wrote: > I don't want object.h to have a dependency on QObject. We need to phase > out QObject. The header doesn't. > Couple things: > > 1) We shouldn't use generic interfaces to read/write properties from > objects. We should use type-safe accessors provided by the types > themselves. > > 2) If we want to get fancy, we can add property_set_int, etc. and then > implement (1) via header files that just call these functions. That's what patch 5 does. But writing visitors in C is a royal PITA. The only sane way to do so is via QObject. Paolo
On 02/02/2012 08:24 PM, Paolo Bonzini wrote: >> >> 1) We shouldn't use generic interfaces to read/write properties from >> objects. We should use type-safe accessors provided by the types >> themselves. That doesn't change the fact that we need simple wrappers using C types (at various levels: object_property_set_qobject, object_property_set, qdev_set_*) to implement these type-safe accessors on top of dynamic properties. >> 2) If we want to get fancy, we can add property_set_int, etc. and then >> implement (1) via header files that just call these functions. > > That's what patch 5 does. But writing visitors in C is a royal PITA. > The only sane way to do so is via QObject. BTW, I don't really think it's possible to proceed on this except by accepting compromises. We need to be the #1 QOM client, _now_ or it will remain buggy & bitrot. Paolo
On 02/02/2012 01:24 PM, Paolo Bonzini wrote: > On 02/02/2012 08:06 PM, Anthony Liguori wrote: >> I don't want object.h to have a dependency on QObject. We need to phase >> out QObject. > > The header doesn't. > >> Couple things: >> >> 1) We shouldn't use generic interfaces to read/write properties from >> objects. We should use type-safe accessors provided by the types >> themselves. >> >> 2) If we want to get fancy, we can add property_set_int, etc. and then >> implement (1) via header files that just call these functions. > > That's what patch 5 does. But writing visitors in C is a royal PITA. The only > sane way to do so is via QObject. You just need a variant visitor. It's pretty simple to do, essentially: typedef struct VariantVisitor { Visitor parent; enum { VV_INT, VV_STR } kind; union { int64_t v_int; char *v_str }; } VariantVisitor; /* input */ static void visit_int(...) { v->kind = TYPE_INT; v->v_int = *value; } /* output */ static void visit_int(...) { assert(v->kind == TYPE_INT); *value = v->v_int; } void variant_visitor_set_int(VariantVisitor *v, int64_t value) { v->kind = TYPE_INT; v->v_int = value; } The only types that matter are int and string so the variant visitor is pretty simple. Regards, Anthony Liguori > > Paolo > >
On 02/02/2012 01:29 PM, Paolo Bonzini wrote: > On 02/02/2012 08:24 PM, Paolo Bonzini wrote: >>> >>> 1) We shouldn't use generic interfaces to read/write properties from >>> objects. We should use type-safe accessors provided by the types >>> themselves. > > That doesn't change the fact that we need simple wrappers using C types (at > various levels: object_property_set_qobject, object_property_set, qdev_set_*) to > implement these type-safe accessors on top of dynamic properties. > >>> 2) If we want to get fancy, we can add property_set_int, etc. and then >>> implement (1) via header files that just call these functions. >> >> That's what patch 5 does. But writing visitors in C is a royal PITA. >> The only sane way to do so is via QObject. > > BTW, I don't really think it's possible to proceed on this except by accepting > compromises. We need to be the #1 QOM client, _now_ or it will remain buggy & > bitrot. Not disagreeing at all with the goal, just the implementation :-) We can pretty easily avoid a QObject dependency. I can throw together that patch if you'd like. Regards, Anthony Liguori > > Paolo >
On 02/02/2012 08:36 PM, Anthony Liguori wrote: > The only types that matter are int and string so the variant visitor is > pretty simple. Sure, only ~150 lines of code. I also do not disagree with the goals (mine and yours), just with the priorities. :) Paolo
On 02/02/2012 01:21 PM, Andreas Färber wrote: > Am 02.02.2012 20:06, schrieb Anthony Liguori: >> On 02/02/2012 10:45 AM, Paolo Bonzini wrote: >>> Move the creation of QmpInputVisitor and QmpOutputVisitor from >>> qmp.c to qom/object.c, since it's the only practical way to access >>> object properties. >>> >>> Signed-off-by: Paolo Bonzini<pbonzini@redhat.com> >>> --- >>> include/qemu/object.h | 24 ++++++++++++++++++++++++ >>> qmp.c | 17 ++--------------- >>> qom/object.c | 29 +++++++++++++++++++++++++++++ >>> 3 files changed, 55 insertions(+), 15 deletions(-) >> >> I don't want object.h to have a dependency on QObject. We need to phase >> out QObject. > > We did get that dependency though by your move of the property code to > object.c. As you will see shortly, we now need qobject-obj-y and > qapi-obj-y plus some stubs to make the user emulators compile with QOM. > That's an implementation detail of Error, that's not because QObject is used anywhere in QOM. Regards, Anthony Liguori > Andreas >
On 02/02/2012 02:08 PM, Paolo Bonzini wrote: > On 02/02/2012 08:36 PM, Anthony Liguori wrote: >> The only types that matter are int and string so the variant visitor is >> pretty simple. > > Sure, only ~150 lines of code. I also do not disagree with the goals (mine and > yours), just with the priorities. :) That's fine, it's a priority for me, so I'm happy to send a patch to your series. I think it's important to maintain strict modularity at the core layer of QOM. Regards, Anthony Liguori > > Paolo >
diff --git a/include/qemu/object.h b/include/qemu/object.h index 947cf29..71090f2 100644 --- a/include/qemu/object.h +++ b/include/qemu/object.h @@ -542,6 +542,18 @@ void object_property_get(Object *obj, struct Visitor *v, const char *name, struct Error **errp); /** + * object_property_get_qobject: + * @obj: the object + * @name: the name of the property + * @errp: returns an error if this function fails + * + * Returns: the value of the property, converted to QObject, or NULL if + * an error occurs. + */ +struct QObject *object_property_get_qobject(Object *obj, const char *name, + struct Error **errp); + +/** * object_property_set: * @obj: the object * @v: the visitor that will be used to write the property value. This should @@ -556,6 +568,18 @@ void object_property_set(Object *obj, struct Visitor *v, const char *name, struct Error **errp); /** + * object_property_set_qobject: + * @obj: the object + * @ret: The value that will be written to the property. + * @name: the name of the property + * @errp: returns an error if this function fails + * + * Writes a property to a object. + */ +void object_property_set_qobject(Object *obj, struct QObject *qobj, + const char *name, struct Error **errp); + +/** * @object_property_get_type: * @obj: the object * @name: the name of the property diff --git a/qmp.c b/qmp.c index 45052cc..c7a81cc 100644 --- a/qmp.c +++ b/qmp.c @@ -21,8 +21,6 @@ #include "kvm.h" #include "arch_init.h" #include "hw/qdev.h" -#include "qapi/qmp-input-visitor.h" -#include "qapi/qmp-output-visitor.h" #include "blockdev.h" NameInfo *qmp_query_name(Error **errp) @@ -198,7 +196,6 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret) const char *property = qdict_get_str(qdict, "property"); QObject *value = qdict_get(qdict, "value"); Error *local_err = NULL; - QmpInputVisitor *mi; Object *obj; obj = object_resolve_path(path, NULL); @@ -207,10 +204,7 @@ int qmp_qom_set(Monitor *mon, const QDict *qdict, QObject **ret) goto out; } - mi = qmp_input_visitor_new(value); - object_property_set(obj, qmp_input_get_visitor(mi), property, &local_err); - - qmp_input_visitor_cleanup(mi); + object_property_set_qobject(obj, value, property, &local_err); out: if (local_err) { @@ -227,7 +221,6 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret) const char *path = qdict_get_str(qdict, "path"); const char *property = qdict_get_str(qdict, "property"); Error *local_err = NULL; - QmpOutputVisitor *mo; Object *obj; obj = object_resolve_path(path, NULL); @@ -236,13 +229,7 @@ int qmp_qom_get(Monitor *mon, const QDict *qdict, QObject **ret) goto out; } - mo = qmp_output_visitor_new(); - object_property_get(obj, qmp_output_get_visitor(mo), property, &local_err); - if (!local_err) { - *ret = qmp_output_get_qobject(mo); - } - - qmp_output_visitor_cleanup(mo); + *ret = object_property_get_qobject(obj, property, &local_err); out: if (local_err) { diff --git a/qom/object.c b/qom/object.c index 299e146..13c8bec 100644 --- a/qom/object.c +++ b/qom/object.c @@ -13,6 +13,8 @@ #include "qemu/object.h" #include "qemu-common.h" #include "qapi/qapi-visit-core.h" +#include "qapi/qmp-input-visitor.h" +#include "qapi/qmp-output-visitor.h" #define MAX_INTERFACES 32 @@ -646,6 +648,33 @@ void object_property_set(Object *obj, Visitor *v, const char *name, } } +void object_property_set_qobject(Object *obj, QObject *value, + const char *name, Error **errp) +{ + QmpInputVisitor *mi; + mi = qmp_input_visitor_new(value); + object_property_set(obj, qmp_input_get_visitor(mi), name, errp); + + qmp_input_visitor_cleanup(mi); +} + +QObject *object_property_get_qobject(Object *obj, const char *name, + Error **errp) +{ + QObject *ret = NULL; + Error *local_err = NULL; + QmpOutputVisitor *mo; + + mo = qmp_output_visitor_new(); + object_property_get(obj, qmp_output_get_visitor(mo), name, &local_err); + if (!local_err) { + ret = qmp_output_get_qobject(mo); + } + error_propagate(errp, local_err); + qmp_output_visitor_cleanup(mo); + return ret; +} + const char *object_property_get_type(Object *obj, const char *name, Error **errp) { ObjectProperty *prop = object_property_find(obj, name);
Move the creation of QmpInputVisitor and QmpOutputVisitor from qmp.c to qom/object.c, since it's the only practical way to access object properties. Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- include/qemu/object.h | 24 ++++++++++++++++++++++++ qmp.c | 17 ++--------------- qom/object.c | 29 +++++++++++++++++++++++++++++ 3 files changed, 55 insertions(+), 15 deletions(-)