Failing property setters + hardwired devices + -global = a bad day

* Failing property setters + hardwired devices + -global = a bad day
@ 2020-04-29 15:28 Markus Armbruster
  2020-04-29 15:39 ` Paolo Bonzini
  2020-04-29 15:57 ` Daniel P. Berrangé
  0 siblings, 2 replies; 21+ messages in thread
From: Markus Armbruster @ 2020-04-29 15:28 UTC (permalink / raw)
  To: qemu-devel
  Cc: Kevin Wolf, Peter Maydell, Daniel P. Berrangé,
	Eduardo Habkost, Jason Wang, Max Reitz, Marc-André Lureau,
	Paolo Bonzini

Here's how we represent QOM properties:

    struct ObjectProperty
    {
        gchar *name;
        [...]
        ObjectPropertyAccessor *get;
        ObjectPropertyAccessor *set;
        ObjectPropertyInit *init;
        void *opaque;
        QObject *defval;
    };

= The setter =

A ->set() method looks like this:

    void TYPE_set_PROP(Object *obj, Visitor *v,
                       const char *name, void *opaque, Error **errp)

The bit to remember for later is that it can fail.

= Default defaults =

When an object gets created, its memory is zeroed, and then any class
properties with an ->init() are initialized with

    obj->init(obj, prop);

We have just one ->init():

    void object_property_init_defval(Object *obj, ObjectProperty *prop)

It initializes from ->defval using ->set().

Aside: feels overengineered, but let's move on.

For TYPE_DEVICE objects, "static" properties get initialized the same.

Aside: if I know what "static" means, I'll be hanged.

The bit to remember for later is that properties start at zero, and some
have their zero overwritten with ->set().

= "Compat" defaults =

TYPE_DEVICE objects and the accelerator (the thing you configure with
-accel) next undergo object_apply_compat_props().

TYPE_DEVICE objects do this from ->instance_post_init().

For the accelerator, we do it where we create it, in
do_configure_accelerator().

This provides for defaults that depend on the accelerator, the machine
type, or certain legacy command line option desugaring (don't ask).

These default values are stored as strings, and applied with
object_property_parse(), which uses ->set().

The bit to remember for later is that some properties have their default
default overwritten with ->set() one or more times.

= -global defaults =

TYPE_DEVICE objects next undergo qdev_prop_set_globals(), still from
->instance_post_init().  This applies the user's -global options.

These default values are also stored as strings, and also applied with
object_property_parse(), which uses ->set().

The bit to remember for later is that some properties have their
built-in default overwritten with ->set() one more time.

= Configuration =

Now we come to a fork in the road: devices created with configuration
taken from -device / device_add, and devices the code creates by itself.

== -device / device_add ==

This is qdev_device_add().  Works as follows:

    1. create object (this applies the various defaults)
    2. Pass the users KEY=VALUE,... to object_property_parse() one by
       one
    3. realize

The bit to remember for later is that properties of user-creatable
devices get overwritten with ->set one final time.

== Hardwired devices ==

Common pattern:

    1. create object (this applies the various defaults)
    2. set properties with qdev_prop_set_FOO() one by one
    3. realize

= Handling ->set() failure =

Here's how the uses of ->set() we've seen so far handle failure:

* Default defaults

  An ->init() method cannot fail; object_property_init_defval() passes
  &error_abort.

  Rationale: if the defaults we bake into QEMU don't work, it's a
  programming error.

* "Compat" defaults

  do_configure_accelerator() can fail, but an ->instance_post_init()
  method can't; object_apply_compat_props() uses &error_abort, except
  for the legacy command line option desugaring, where it uses
  &error_fatal.

  Rationale: if the defaults we bake into QEMU don't work, it's a
  programming error.  If legacy CLI options can't be applied, it's a
  fatal error.

* -global defaults

  An ->instance_post_init() method can't fail; qdev_prop_set_globals()
  uses &error_fatal.

  Rationale: if the user's -global can't be applied, it's a fatal error.

  Except it *ignores* errors afterward initial configuration (after
  qdev_machine_creation_done()).

  Rationale: silently ignoring the user's -global for hot-plugged
  devices is bad, killing the VM is worse.  We suck.

* -device / device_add

  qdev_device_add() fails cleanly.

  Rationale: sometimes we manage not to suck.

* Hardwired devices

  The qdev_prop_set_FOO() can't fail; they pass &error_abort.

  Rationale: if the configuration we bake into QEMU doesn't work, it's a
  programming error.

  Except qdev_prop_set_drive() can fail.  Of its 43 callers, 9 pass
  &error_abort, 27 pass &error_fatal, and 7 try to handle failure.
  Of these seven, several are obviously wrong.

  Rationale: uh, we like exceptions?

= Can this get more depressing? =

Yes!

->set() can run many times, yet I found three ->set() methods that can't
handle overwriting non-zero:

* set_netdev() fails with "Property NETDEV doesn't take value VALUE"

  Reproducer:

    $ upstream-qemu -nodefaults -S -monitor stdio -display none -nic user -netdev user,id=foo -global e1000.netdev=foo
    QEMU 5.0.0 monitor - type 'help' for more information
    (qemu) Unexpected error in error_set_from_qdev_prop_error() at /work/armbru/qemu/hw/core/qdev-properties.c:1105:
    upstream-qemu: Property 'e1000.netdev' doesn't take value '__org.qemu.nic0
    '
    Aborted (core dumped)

  The abort is courtesy qdev_prop_set_net().

* set_drive() leaves the old value attached to the device, which is wrong

  Reproducer:

    $ upstream-qemu -S -monitor stdio -display none -fda tmp.qcow2 -drive if=none,file=test.qcow2 -global isa-fdc.driveA=none0
    QEMU 5.0.0 monitor - type 'help' for more information
    (qemu) info block
    floppy0 (#block162): tmp.qcow2 (qcow2)
  --->  Attached to:      /machine/unattached/device[16]
        Removable device: not locked, tray closed
        Cache mode:       writeback
        Backing file:     tmp.img (chain depth: 1)

    none0 (#block535): test.qcow2 (qcow2)
  --->  Attached to:      /machine/unattached/device[15]
        Cache mode:       writeback
        Backing file:     f29.raw (chain depth: 1)

    ide1-cd0: [not inserted]
        Attached to:      /machine/unattached/device[23]
        Removable device: not locked, tray closed

    sd0: [not inserted]
        Removable device: not locked, tray closed

  device[15] is "isa-fdc".

  device[16] is the "floppy" hanging of "isa-fdc".  Magic in fdc.c moved
  it from "isa-fdc" (don't ask).

* set_chr() calls qemu_chr_fe_init() again without qemu_chr_fe_deinit(),
  which is wrong

  Reproducer:

    $ upstream-qemu -nodefaults -S -monitor stdio -display none -chardev null,id=foo -chardev null,id=bar -serial chardev:foo -global isa-serial.chardev=bar

The "if the configuration we bake into QEMU doesn't work, it's a
programming error" idea needs amending: "doesn't work with any -global
shenanigans users may come up with".

I guess these ->set() flaws are fixable.

The knee-jerk fix is to have ->set() fail rather than screw up.  This is
what set_netdev() does.  It founders on the amended "if the
configuration we bake into QEMU don't work, it's a programming error"
idea: ->set() can then fail there because -global set it up for failure.

Making all the qdev_prop_set_FOO() users handle errors would be a lot of
work: in addition to the 43 users of qdev_prop_set_drive(), we have more
than 50 of qdev_prop_set_chr() and almost 40 of
qdev_set_nic_properties().  Infeasible for me.  Bye, bye knee-jerk fix.

Is there any sane use for configuring backends via any of the default
mechanisms?

I'm aware of one, but it's outdated: -global isa-fdc.driveA=...  Use
-device floppy instead.

I'd love to deprecate -global wholesale, but we can't as long as we
don't have better means to configure onboard devices.  Can we deprecate
its use with backend properties at least?

While we're talking about deprecation: what about use of -global with
hot-plugged devices?  Remember, bad -global gets silently ignored there.

^ permalink raw reply	[flat|nested] 21+ messages in thread