[Qemu-devel] [PATCH 00/50] Convert device_add to QObject / QError

[Markus Armbruster <armbru@redhat.com>]

Why this is such a big job?  There are two issues with a naive
conversion:

* Error message degradation

  The error messages are worded for -device.  They aren't so hot to
  begin with, because we typically have many -device, and to which one
  a message applies is often not obvious.

  Now, QMP wants relatively generic errors.  For instance, "-device:
  no driver specified" becomes "Parameter 'driver' is missing".
  That's just too mean to users.

  However, if you know *where* the parameter is missing, the generic
  message is perfectly adequate: "-device a=b: Parameter 'driver' is
  missing".  In fact, this is even superior to the old message.

  So the first part of the patch series is about error locations.  I
  feel it's very useful all by itself.  I can split it off into its
  own patch series.  But then the rest of this series depends on it,
  so I'm not sure splitting is all that useful.  However, if you think
  the first part is ready for commit while the other parts are not,
  please feel free to commit just the first part.

  We may still encounter cases where a generic message is not adequate
  even with precise location information.  We'll solve that problem
  when we hit it.

* String argument with option syntax, i.e. NAME=VALUE,...

  QMP uses JSON to encode collections of name/value pairs.  Adding a
  second encoding for the same thing would be a mistake, in my
  opinion.

  Note that we already have two competing dictionary data types in our
  code, though: QDict and QemuOpts.  We should not permit that to leak
  into an external protocol like QMP.

  QemuOpts originated in the command line and spread from there into a
  few monitor commands, including device_add, and a few internal
  interfaces.

  QDict originated in the monitor.  It sits right at the interface
  between monitor core and command handlers.

  In the long run, we might be better off standardizing on a single
  dictionary data type in our code, but that's clearly out of scope
  for this series.

  My proposed solution is modest and pragmatic:

  * Lift parsing of option strings into QemuOpts from monitor handlers
    up into the human monitor core.  This removes QemuOpts from the
    handler interface, and thus avoids leaking it into QMP.  It's
    exactly what we've done for other argument types with syntax
    inappropriate for QMP, such as arguments of migrate_set_speed and
    migrate_set_downtime.

  * Monitor handlers that need to pass their arguments in
    QemuOpts-form to internal interfaces use a converter function to
    translate from QDict to QemuOpts.

  This is what makes up the bulk of the patch series' last part.  If
  you'd prefer a different solution, let's talk.  I can split it off
  into its own patch series if that helps.  However, the patches
  before it aren't all that useful without it, so I'm not sure
  splitting buys us much.  As this series is struggling with obesity,
  I refrained from covering the general case in a few places where we
  don't need it yet.  They are all clearly marked in commit messages
  and code.

  A possible alternative is to add the concept of optional named
  arguments to the monitor.  Instead of encoding multiple optional
  named arguments in a single positional argument, we encode them as
  multiple named arguments.  For instance, "device_add
  ide-drive,drive=hda,bus=ide.0,unit=0" becomes "device_add ide-drive
  drive=hda bus=ide.0 unit=0".

  Of course, if you think that adding a second encoding for
  collections of name/value pairs to QMP is fine, then this last part
  becomes much simpler.

So, the series starts with error locations (part I), and ends with the
conversion of device_add to QObject, taking care to keep QemuOpts out
of QMP (part III).  Wedged in between is the conversion of device_add
to QError (part II).  In more detail:

Part I: Error Locations

  [01-07] Preliminary cleanup & fixes
     [08] Separate "default monitor" and "current monitor" cleanly
  [09-17] More cleanup
  [18-22] Error Locations

Part II: Convert device_add to QError

  [23-26] Preliminary qdev cleanup & fixes
  [27-44] Convert device_add to QError

Part III Convert device_add to QObject

     [45] Conversions between QDict and QemuOpts
  [46-48] New monitor argument type O
  [49-50] Convert device_add to QObject


Not much changed since the RFC:

* Restore accidentally lost newline in scsi_hot_add() [PATCH 15].

* Rename qemu_error() and qemu_error_new() to error_report() and
  qerror_report() [PATCH 16-17].  Requested by Luiz & Paolo.

* Prettier struct Location [PATCH 18,20,22].  Requested by Luiz.

* Name new function monitor_cur_is_qmp() instead of in_qmp_mon()
  [PATCH 27,38,50].  Requested by Luiz.

* Let converted handlers print in human monitor [PATCH 28].

* Make QMP accept non-string arguments for monitor argument type 'O'
  [PATCH 45].

* Restrict new monitor argument type 'O' to lists with empty desc for
  now, because that's all that is exercised in this series [PATCH 48].

* Fix typo in device_add help text [PATCH 49].

* More comments.

* Rebased.  Also reordered for better legibility (affects PATCH
  13-16).


Markus Armbruster (50):
  usb: Remove disabled monitor_printf() in usb_read_file()
  savevm: Fix -loadvm to report errors to stderr, not the monitor
  pc: Fix error reporting for -boot once
  pc: Factor common code out of pc_boot_set() and cmos_init()
  tools: Remove unused cur_mon from qemu-tool.c
  monitor: Separate "default monitor" and "current monitor" cleanly
  block: Simplify usb_msd_initfn() test for "can read bdrv key"
  monitor: Factor monitor_set_error() out of qemu_error_internal()
  error: Move qemu_error() & friends from monitor.c to own file
  error: Simplify error sink setup
  error: Move qemu_error & friends into their own header
  error: New error_printf() and error_vprintf()
  error: Don't abuse qemu_error() for non-error in qdev_device_help()
  error: Don't abuse qemu_error() for non-error in qbus_find()
  error: Don't abuse qemu_error() for non-error in scsi_hot_add()
  error: Replace qemu_error() by error_report()
  error: Rename qemu_error_new() to qerror_report()
  error: Infrastructure to track locations for error reporting
  error: Include the program name in error messages to stderr
  error: Track locations in configuration files
  QemuOpts: Fix qemu_config_parse() to catch file read errors
  error: Track locations on command line
  qdev: Fix -device and device_add to handle unsuitable bus gracefully
  qdev: Factor qdev_create_from_info() out of qdev_create()
  qdev: Hide "no_user" devices from users
  qdev: Hide "ptr" properties from users
  monitor: New monitor_cur_is_qmp()
  error: Let converted handlers print in human monitor
  error: Polish human-readable error descriptions
  error: New QERR_PROPERTY_NOT_FOUND
  error: New QERR_PROPERTY_VALUE_BAD
  qdev: convert setting device properties to QError
  qdev: Relax parsing of bus option
  error: New QERR_BUS_NOT_FOUND
  error: New QERR_DEVICE_MULTIPLE_BUSSES
  error: New QERR_DEVICE_NO_BUS
  qdev: Convert qbus_find() to QError
  error: New error_printf_unless_qmp()
  error: New QERR_BAD_BUS_FOR_DEVICE
  error: New QERR_BUS_NO_HOTPLUG
  error: New QERR_DEVICE_INIT_FAILED
  error: New QERR_NO_BUS_FOR_DEVICE
  Revert "qdev: Use QError for 'device not found' error"
  error: Convert do_device_add() to QError
  qemu-option: Functions to convert to/from QDict
  qemu-option: Move the implied first name into QemuOptsList
  qemu-option: Rename find_list() to qemu_find_opts() & external
    linkage
  monitor: New argument type 'O'
  monitor: Use argument type 'O' for device_add
  monitor: convert do_device_add() to QObject

 Makefile.target        |    1 +
 audio/audio.c          |    4 +-
 hw/pc.c                |   35 ++----
 hw/pci-hotplug.c       |   14 +-
 hw/pci.c               |   14 +-
 hw/qdev-properties.c   |   27 ++---
 hw/qdev.c              |  236 ++++++++++++++++++++--------------
 hw/qdev.h              |    2 +-
 hw/scsi-bus.c          |    4 +-
 hw/scsi-disk.c         |    5 +-
 hw/scsi-generic.c      |    9 +-
 hw/usb-bus.c           |    4 +-
 hw/usb-msd.c           |    4 +-
 hw/usb-net.c           |    2 +-
 hw/usb-serial.c        |    9 +-
 hw/virtio-net.c        |    5 +-
 hw/virtio-pci.c        |    4 +-
 hw/virtio-serial-bus.c |    2 +-
 monitor.c              |  337 +++++++++++++++++++++---------------------------
 monitor.h              |    7 +
 net.c                  |   32 +++---
 net/dump.c             |    5 +-
 net/slirp.c            |   28 ++--
 net/socket.c           |   12 +-
 net/tap-bsd.c          |    7 +-
 net/tap-linux.c        |    9 +-
 net/tap-solaris.c      |    4 +-
 net/tap-win32.c        |    2 +-
 net/tap.c              |    3 +-
 qemu-config.c          |   56 +++++---
 qemu-config.h          |    3 +-
 qemu-error.c           |  227 ++++++++++++++++++++++++++++++++
 qemu-error.h           |   47 +++++++
 qemu-monitor.hx        |    7 +-
 qemu-option.c          |   93 +++++++++++++-
 qemu-option.h          |    6 +-
 qemu-tool.c            |   30 ++++-
 qerror.c               |   75 ++++++++---
 qerror.h               |   45 ++++++-
 savevm.c               |   27 ++--
 slirp/misc.c           |    2 +-
 sysemu.h               |   13 +--
 usb-linux.c            |    8 -
 vl.c                   |   44 ++++---
 vnc.c                  |    5 +-
 45 files changed, 987 insertions(+), 528 deletions(-)
 create mode 100644 qemu-error.c
 create mode 100644 qemu-error.h