[0/2] qapi/qom: use correct field name when getting/setting alias properties

Switching -M parsing from QemuOptions and StringInputVisitor to keyval and
QObjectInputVisitor exposed a latent bug in alias properties.
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
the "wrong" name compared to what was passed on the command line,
and the visitor will not be able to find it.

The solution that is implemented here 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.

Patch 1 implements the visitor adapter, while patch 2 applies it in QOM.

Paolo


Paolo Bonzini (2):
  qapi: introduce forwarding visitor
  qom: use correct field name when getting/setting alias properties

 include/qapi/forward-visitor.h    |  27 +++
 qapi/meson.build                  |   1 +
 qapi/qapi-forward-visitor.c       | 307 ++++++++++++++++++++++++++++++
 qom/object.c                      |   9 +-
 tests/unit/meson.build            |   1 +
 tests/unit/test-forward-visitor.c | 165 ++++++++++++++++
 6 files changed, 508 insertions(+), 2 deletions(-)
 create mode 100644 include/qapi/forward-visitor.h
 create mode 100644 qapi/qapi-forward-visitor.c
 create mode 100644 tests/unit/test-forward-visitor.c

Running out of time for today, so I'm sending what I have, with real
review to follow.


First, let me describe what's wrong in my own words, because that's how
I understand stuff.

Reproducer (hidden behind the link to the bug tracker):

    $ qemu-system-x86_64 \
     -blockdev '{"driver":"file","filename":"/usr/share/edk2/ovmf/OVMF_CODE.fd","node-name":"pflash0-storage","auto-read-only":true,"discard":"unmap"}' \
     -blockdev '{"node-name":"pflash0-format","read-only":true,"driver":"raw","file":"pflash0-storage"}' \
     -machine pc,pflash0=pflash0-format

The -machine gets (keyval-)parsed into a QDict, which is then processed
by object_set_properties_from_qdict() with the (keyval) QObject input
visitor.  Makes sense.

object_set_properties_from_qdict() performs a virtual struct visit
guided by the QDict's keys.  It calls object_property_set() for each
key.

For "pflash0", this calls object_property_set(obj, "pflash0", v, &err).
Since "pflash0" is a QOM alias property, this in turn calls
object_property_set(prop->target_obj, "drive0", v, &err), where prop is
the alias property, and prop->target_obj is the "cfi.pflash01" device.

Since "drive" is a drive property, this calls set_drive(), which calls
visit_type_str(v, "drive", &str, errp).  Fails, because the QDict
wrapped in visitor @v does not have a "drive" member, it has a "pflash0"
member.


Next, the solution.  I get the idea of a wrapper visitor which gives you
"pflash0" when you ask for "drive", but oh boy do I wish we could fix
the bug with a lot less code.

On 20/07/21 17:54, Markus Armbruster wrote:
> First, let me describe what's wrong in my own words, because that's how
> I understand stuff.
> 
> [snip]

All correct.

> Next, the solution.  I get the idea of a wrapper visitor which gives you
> "pflash0" when you ask for "drive", but oh boy do I wish we could fix
> the bug with a lot less code.

Yeah, if QOM didn't use visitors and just went with QObject as the 
argument to getters/setters, all this wouldn't be needed.

That said, 1/3rd of this patch is tests, and visitors do have a hidden 
advantage: they give type checking for free.  So all in all I'm not sure 
it would be better, especially now that we're starting to get more 
benefit from them (e.g. with compound properties replacing 
special-purpose command line parsing code).

Paolo

Since patch submitters tend to submit code that works for the success
case, I like to test a few failure cases before anything else.  Gotcha:

    $ qemu-system-x86_64 -machine pc,pflash0=xxx qemu-system-x86_64:
    Property 'cfi.pflash01.drive' can't find value 'xxx'

The error message is misleading.

This is not a "must not commit" issue.  Fixing a regression in time for
the release at the price of a bad error message is still a win.  The bad
error message needs fixing all the same, just not necessarily before the
release.

Since mere thinking doesn't rock the release boat: any ideas on how this
could be fixed?

On 22/07/21 15:25, Markus Armbruster wrote:
> Since patch submitters tend to submit code that works for the success
> case, I like to test a few failure cases before anything else.  Gotcha:
> 
>      $ qemu-system-x86_64 -machine pc,pflash0=xxx
>      qemu-system-x86_64: Property 'cfi.pflash01.drive' can't find value 'xxx'
> 
> The error message is misleading.

Indeed I knew about this, and even thought briefly about how to fix it 
before realizing it is not a regression (which is also why I didn't 
think of including it in the commit message).

All the ways I could think about for a fix involved looking at the error 
class, and possibly even adding a dictionary of key-value pairs for some 
error classes.  I know you don't really like error classes and you 
probably would like the idea of key-value pairs even less---and to be 
honest I didn't really have a plan to implement any of that.

Paolo

> This is not a "must not commit" issue.  Fixing a regression in time for
> the release at the price of a bad error message is still a win.  The bad
> error message needs fixing all the same, just not necessarily before the
> release.
> 
> Since mere thinking doesn't rock the release boat: any ideas on how this
> could be fixed?
> 
>