| Message ID | 20210719104033.185109-3-pbonzini@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | qapi/qom: use correct field name when getting/setting alias properties | expand |
On 7/19/21 12:40 PM, Paolo Bonzini wrote: > Alias targets have a different name than the alias property itself > (e.g. a machine's pflash0 might be an alias of a property named 'drive'). > When the target's getter or setter invokes the visitor, it will use > a different name than what the caller expects, and the visitor will > not be able to find it (or will consume erroneously). > > The solution is for alias getters and setters to wrap the incoming > visitor, and forward the sole field that the target is expecting while > renaming it appropriately. > > This bug has been there forever, but it was exposed after -M parsing > switched from QemuOptions and StringInputVisitor to keyval and > QObjectInputVisitor. Before, the visitor ignored the name. Now, it > checks "drive" against what was passed on the command line and finds > that no such property exists. > > Fixes: #484 Per https://www.mail-archive.com/qemu-devel@nongnu.org/msg821579.html: Fixes: https://gitlab.com/qemu-project/qemu/-/issues/484 > Reported-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > qom/object.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-)
On Mon, Jul 19, 2021 at 12:40:33PM +0200, Paolo Bonzini wrote: > Alias targets have a different name than the alias property itself > (e.g. a machine's pflash0 might be an alias of a property named 'drive'). > When the target's getter or setter invokes the visitor, it will use > a different name than what the caller expects, and the visitor will > not be able to find it (or will consume erroneously). > > The solution is for alias getters and setters to wrap the incoming > visitor, and forward the sole field that the target is expecting while > renaming it appropriately. > > This bug has been there forever, but it was exposed after -M parsing > switched from QemuOptions and StringInputVisitor to keyval and > QObjectInputVisitor. Before, the visitor ignored the name. Now, it > checks "drive" against what was passed on the command line and finds > that no such property exists. > > Fixes: #484 > Reported-by: Alex Williamson <alex.williamson@redhat.com> > Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> > --- > qom/object.c | 9 +++++++-- > 1 file changed, 7 insertions(+), 2 deletions(-) Deceptively simple; all the work was in the previous patch writing up the forwarding visitor. I still wonder if Kevin's QAPI aliases will do this more gracefully, but if we're trying to justify this as a bug fix worthy of 6.1, this is certainly a smaller approach than Kevin's. Reviewed-by: Eric Blake <eblake@redhat.com>
On 20/07/21 03:00, Eric Blake wrote: > Deceptively simple; all the work was in the previous patch writing up > the forwarding visitor. I still wonder if Kevin's QAPI aliases will > do this more gracefully, but if we're trying to justify this as a bug > fix worthy of 6.1, this is certainly a smaller approach than Kevin's. > > Reviewed-by: Eric Blake<eblake@redhat.com> As discussed on IRC, this is unrelated to QAPI aliases; QOM alias properties typically target a property *on a different object*. This is a regression, so it certainly has to be fixed in 6.1 one way or the other. Paolo
Paolo Bonzini <pbonzini@redhat.com> writes: > On 20/07/21 03:00, Eric Blake wrote: >> Deceptively simple; all the work was in the previous patch writing up >> the forwarding visitor. I still wonder if Kevin's QAPI aliases will >> do this more gracefully, but if we're trying to justify this as a bug >> fix worthy of 6.1, this is certainly a smaller approach than Kevin's. >> Reviewed-by: Eric Blake<eblake@redhat.com> > > As discussed on IRC, this is unrelated to QAPI aliases; QOM alias > properties typically target a property *on a different object*. Yes, these are different beasts. A QAPI alias provides an alternate name for a member. The member may be nested. It's still within the same QAPI object. Can be useful for maintaining backwards compatibility, in particular for replacing (flat) QemuOpts by QAPI-based dotted keys. A QOM alias property is a proxy for a property of an *arbitrary* QOM object. Not limited to the alias's object and its sub-objects. Strictly more powerful. QOM alias properties are created at run time: creation requires the target object. QAPI aliases are completely defined at compile-time. > This is a regression, so it certainly has to be fixed in 6.1 one way > or the other. Understood.
diff --git a/qom/object.c b/qom/object.c index 6a01d56546..e86cb05b84 100644 --- a/qom/object.c +++ b/qom/object.c @@ -20,6 +20,7 @@ #include "qapi/string-input-visitor.h" #include "qapi/string-output-visitor.h" #include "qapi/qobject-input-visitor.h" +#include "qapi/forward-visitor.h" #include "qapi/qapi-builtin-visit.h" #include "qapi/qmp/qerror.h" #include "qapi/qmp/qjson.h" @@ -2683,16 +2684,20 @@ static void property_get_alias(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { AliasProperty *prop = opaque; + Visitor *alias_v = visitor_forward_field(v, prop->target_name, name); - object_property_get(prop->target_obj, prop->target_name, v, errp); + object_property_get(prop->target_obj, prop->target_name, alias_v, errp); + visit_free(alias_v); } static void property_set_alias(Object *obj, Visitor *v, const char *name, void *opaque, Error **errp) { AliasProperty *prop = opaque; + Visitor *alias_v = visitor_forward_field(v, prop->target_name, name); - object_property_set(prop->target_obj, prop->target_name, v, errp); + object_property_set(prop->target_obj, prop->target_name, alias_v, errp); + visit_free(alias_v); } static Object *property_resolve_alias(Object *obj, void *opaque,
Alias targets have a different name than the alias property itself (e.g. a machine's pflash0 might be an alias of a property named 'drive'). When the target's getter or setter invokes the visitor, it will use a different name than what the caller expects, and the visitor will not be able to find it (or will consume erroneously). The solution is for alias getters and setters to wrap the incoming visitor, and forward the sole field that the target is expecting while renaming it appropriately. This bug has been there forever, but it was exposed after -M parsing switched from QemuOptions and StringInputVisitor to keyval and QObjectInputVisitor. Before, the visitor ignored the name. Now, it checks "drive" against what was passed on the command line and finds that no such property exists. Fixes: #484 Reported-by: Alex Williamson <alex.williamson@redhat.com> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com> --- qom/object.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)