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(-)

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,
-- 
2.31.1

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>

-- 
Eric Blake, Principal Software Engineer
Red Hat, Inc.           +1-919-301-3266
Virtualization:  qemu.org | libvirt.org

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.