mbox series

[00/16] Crazy shit around -global (pardon my french)

Message ID 20200605145625.2920920-1-armbru@redhat.com (mailing list archive)
Headers show
Series Crazy shit around -global (pardon my french) | expand

Message

Markus Armbruster June 5, 2020, 2:56 p.m. UTC
There are three ways to configure backends:

* -nic, -serial, -drive, ... (onboard devices)

* Set the property with -device, or, if you feel masochistic, with
  -set device (pluggable devices)

* Set the property with -global (both)

The trouble is -global is terrible.

It gets applied in object_new(), which can't fail.  We treat failure
to apply -global as fatal error, except when hot-plugging, where we
treat it as warning *boggle*.  I'm not addressing that today.

Some code falls apart when you use both -global and the other way.

To make life more interesting, we gave -drive two roles: with
interface type other than none, it's for configuring onboard devices,
and with interface type none, it's for defining backends for use with
-device and such.  Since we neglect to require interface type none for
the latter, you can use one -drive in both roles.  This confuses the
code about as much as you, dear reader, probably are by now.

Because this still isn't interesting enough, there's yet another way
to configure backends, just for floppies: set the floppy controller's
property.  Goes back to the time when floppy wasn't a separate device,
and involves some Bad Magic.  Now -global can interact with itself!

Digging through all this took me an embarrassing amount of time.
Hair, too.

My patches reject some the silliest uses outright, and deprecate some
not so silly ones that have replacements.

Apply on top of my "[PATCH v2 00/58] qdev: Rework how we plug into the
parent bus".

Enjoy!

Based-on: <20200529134523.8477-1-armbru@redhat.com>

Markus Armbruster (16):
  iotests/172: Include "info block" in test output
  iotests/172: Cover empty filename and multiple use of drives
  iotests/172: Cover -global floppy.drive=...
  fdc: Reject clash between -drive if=floppy and -global isa-fdc
  fdc: Open-code fdctrl_init_isa()
  fdc: Deprecate configuring floppies with -global isa-fdc
  docs/qdev-device-use.txt: Update section "Default Devices"
  blockdev: Deprecate -drive with bogus interface type
  qdev: Eliminate get_pointer(), set_pointer()
  qdev: Improve netdev property override error a bit
  qdev: Reject drive property override
  qdev: Reject chardev property override
  qdev: Make qdev_prop_set_drive() match the other helpers
  arm/aspeed: Drop aspeed_board_init_flashes() parameter @errp
  sd/pxa2xx_mmci: Don't crash on pxa2xx_mmci_init() error
  sd/milkymist-memcard: Fix error API violation

 docs/qdev-device-use.txt            |  17 +-
 docs/system/deprecated.rst          |  34 ++
 include/hw/block/fdc.h              |   2 +-
 include/hw/qdev-properties.h        |  18 +-
 include/sysemu/blockdev.h           |   2 +
 blockdev.c                          |  27 +-
 hw/arm/aspeed.c                     |  16 +-
 hw/arm/cubieboard.c                 |   2 +-
 hw/arm/exynos4210.c                 |   2 +-
 hw/arm/imx25_pdk.c                  |   2 +-
 hw/arm/mcimx6ul-evk.c               |   2 +-
 hw/arm/mcimx7d-sabre.c              |   2 +-
 hw/arm/msf2-som.c                   |   4 +-
 hw/arm/nseries.c                    |   4 +-
 hw/arm/orangepi.c                   |   2 +-
 hw/arm/raspi.c                      |   2 +-
 hw/arm/sabrelite.c                  |   6 +-
 hw/arm/vexpress.c                   |   3 +-
 hw/arm/xilinx_zynq.c                |   7 +-
 hw/arm/xlnx-versal-virt.c           |   2 +-
 hw/arm/xlnx-zcu102.c                |  10 +-
 hw/block/fdc.c                      |  82 ++--
 hw/block/nand.c                     |   2 +-
 hw/block/pflash_cfi01.c             |   6 +-
 hw/block/pflash_cfi02.c             |   2 +-
 hw/core/qdev-properties-system.c    | 151 ++++---
 hw/core/qdev-properties.c           |  17 +
 hw/i386/pc.c                        |   8 +-
 hw/ide/qdev.c                       |   4 +-
 hw/isa/isa-superio.c                |  18 +-
 hw/m68k/q800.c                      |   3 +-
 hw/microblaze/petalogix_ml605_mmu.c |   5 +-
 hw/ppc/pnv.c                        |   3 +-
 hw/ppc/spapr.c                      |   4 +-
 hw/scsi/scsi-bus.c                  |   2 +-
 hw/sd/milkymist-memcard.c           |   2 +-
 hw/sd/pxa2xx_mmci.c                 |  15 +-
 hw/sd/sd.c                          |   2 +-
 hw/sd/ssi-sd.c                      |   3 +-
 hw/sparc64/sun4u.c                  |   9 +-
 hw/xtensa/xtfpga.c                  |   3 +-
 softmmu/vl.c                        |   8 +
 tests/qemu-iotests/172              |  27 +-
 tests/qemu-iotests/172.out          | 656 +++++++++++++++++++++++++---
 44 files changed, 928 insertions(+), 270 deletions(-)

Comments

John Snow June 10, 2020, 2:21 p.m. UTC | #1
On 6/5/20 10:56 AM, Markus Armbruster wrote:
> There are three ways to configure backends:
> 
> * -nic, -serial, -drive, ... (onboard devices)
> 
> * Set the property with -device, or, if you feel masochistic, with
>   -set device (pluggable devices)
> 
> * Set the property with -global (both)
> 
> The trouble is -global is terrible.
> 
> It gets applied in object_new(), which can't fail.  We treat failure
> to apply -global as fatal error, except when hot-plugging, where we
> treat it as warning *boggle*.  I'm not addressing that today.
> 
> Some code falls apart when you use both -global and the other way.
> 
> To make life more interesting, we gave -drive two roles: with
> interface type other than none, it's for configuring onboard devices,
> and with interface type none, it's for defining backends for use with
> -device and such.  Since we neglect to require interface type none for
> the latter, you can use one -drive in both roles.  This confuses the
> code about as much as you, dear reader, probably are by now.
> 
> Because this still isn't interesting enough, there's yet another way
> to configure backends, just for floppies: set the floppy controller's
> property.  Goes back to the time when floppy wasn't a separate device,
> and involves some Bad Magic.  Now -global can interact with itself!
> 
> Digging through all this took me an embarrassing amount of time.
> Hair, too.
> 
> My patches reject some the silliest uses outright, and deprecate some
> not so silly ones that have replacements.
> 
> Apply on top of my "[PATCH v2 00/58] qdev: Rework how we plug into the
> parent bus".
> 
> Enjoy!
> 
> Based-on: <20200529134523.8477-1-armbru@redhat.com>
> 
> Markus Armbruster (16):
>   iotests/172: Include "info block" in test output
>   iotests/172: Cover empty filename and multiple use of drives
>   iotests/172: Cover -global floppy.drive=...
>   fdc: Reject clash between -drive if=floppy and -global isa-fdc
>   fdc: Open-code fdctrl_init_isa()
>   fdc: Deprecate configuring floppies with -global isa-fdc
>   docs/qdev-device-use.txt: Update section "Default Devices"
>   blockdev: Deprecate -drive with bogus interface type
>   qdev: Eliminate get_pointer(), set_pointer()
>   qdev: Improve netdev property override error a bit
>   qdev: Reject drive property override
>   qdev: Reject chardev property override
>   qdev: Make qdev_prop_set_drive() match the other helpers
>   arm/aspeed: Drop aspeed_board_init_flashes() parameter @errp
>   sd/pxa2xx_mmci: Don't crash on pxa2xx_mmci_init() error
>   sd/milkymist-memcard: Fix error API violation
> 
>  docs/qdev-device-use.txt            |  17 +-
>  docs/system/deprecated.rst          |  34 ++
>  include/hw/block/fdc.h              |   2 +-
>  include/hw/qdev-properties.h        |  18 +-
>  include/sysemu/blockdev.h           |   2 +
>  blockdev.c                          |  27 +-
>  hw/arm/aspeed.c                     |  16 +-
>  hw/arm/cubieboard.c                 |   2 +-
>  hw/arm/exynos4210.c                 |   2 +-
>  hw/arm/imx25_pdk.c                  |   2 +-
>  hw/arm/mcimx6ul-evk.c               |   2 +-
>  hw/arm/mcimx7d-sabre.c              |   2 +-
>  hw/arm/msf2-som.c                   |   4 +-
>  hw/arm/nseries.c                    |   4 +-
>  hw/arm/orangepi.c                   |   2 +-
>  hw/arm/raspi.c                      |   2 +-
>  hw/arm/sabrelite.c                  |   6 +-
>  hw/arm/vexpress.c                   |   3 +-
>  hw/arm/xilinx_zynq.c                |   7 +-
>  hw/arm/xlnx-versal-virt.c           |   2 +-
>  hw/arm/xlnx-zcu102.c                |  10 +-
>  hw/block/fdc.c                      |  82 ++--
>  hw/block/nand.c                     |   2 +-
>  hw/block/pflash_cfi01.c             |   6 +-
>  hw/block/pflash_cfi02.c             |   2 +-
>  hw/core/qdev-properties-system.c    | 151 ++++---
>  hw/core/qdev-properties.c           |  17 +
>  hw/i386/pc.c                        |   8 +-
>  hw/ide/qdev.c                       |   4 +-
>  hw/isa/isa-superio.c                |  18 +-
>  hw/m68k/q800.c                      |   3 +-
>  hw/microblaze/petalogix_ml605_mmu.c |   5 +-
>  hw/ppc/pnv.c                        |   3 +-
>  hw/ppc/spapr.c                      |   4 +-
>  hw/scsi/scsi-bus.c                  |   2 +-
>  hw/sd/milkymist-memcard.c           |   2 +-
>  hw/sd/pxa2xx_mmci.c                 |  15 +-
>  hw/sd/sd.c                          |   2 +-
>  hw/sd/ssi-sd.c                      |   3 +-
>  hw/sparc64/sun4u.c                  |   9 +-
>  hw/xtensa/xtfpga.c                  |   3 +-
>  softmmu/vl.c                        |   8 +
>  tests/qemu-iotests/172              |  27 +-
>  tests/qemu-iotests/172.out          | 656 +++++++++++++++++++++++++---
>  44 files changed, 928 insertions(+), 270 deletions(-)
> 

I'll be honest that I'm a little pre-occupied and possibly unable to
review the fdc related changes in-depth. I generally trust your
judgment, and will try to give it a quick scan.

You may treat any further silence as an ACK. Any breakage due to this
policy is therefore assumed to be the liability of the maintainer.

--js