ToDo/CodeTransitions: Difference between revisions

From QEMU
(→‎Error reporting: Functions that use Error should return a value)
 
(25 intermediate revisions by 6 users not shown)
Line 17: Line 17:
=== Making all devices QOM objects ===
=== Making all devices QOM objects ===


Ideally all device models should be QOM objects (usually deriving from DeviceState or one of its subclasses). However we still have a fairly large number of devices which are coded in an ad-hoc way. These should all be converted to QOM but it is a big job. See [[QOMConventions]] for the latest guidance on how to structure a QOM device.
Ideally all device models should be QOM objects (usually deriving from DeviceState or one of its subclasses). However we still have a fairly large number of devices which are coded in an ad-hoc way. These should all be converted to QOM but it is a big job. See [[Documentation/QOMConventions]] for the latest guidance on how to structure a QOM device.
* The QEMUMachine is being replaced with a QOM hierarchy, details on Features/QOM/Machine.
 
=== Use DeviceClass::realize rather than SysBusDeviceClass::init ===
 
All new devices derived from '''SysBusDevice''' should [http://lists.gnu.org/archive/html/qemu-devel/2013-07/msg00041.html use QOM realize] (DeviceClass::'''realize''') rather than SysBusDeviceClass::init. Work is under way to convert existing devices. It is permissable to implement neither realize [http://git.qemu.org/?p=qemu.git;a=commit;h=4ce5dae88ecf2bafa0cd663de7e923728b1b3672 nor init].
 
(QOM realize was first [http://lists.gnu.org/archive/html/qemu-devel/2012-01/msg04025.html presented] by [[User:AnthonyLiguori|Anthony]] on the 2012-01-31 KVM call and after much debate of scope [http://git.qemu.org/?p=qemu.git;a=commit;h=249d41720b7dfbb5951b430b9eefdbee7464f515 minimally implemented] for DeviceState by [[User:AF|Andreas]] for v1.4. Some extensions by [[User:Paolo Bonzini|Paolo]] (recursive realization) are still pending device preparations.)
 
=== MemoryRegionOps old_mmio ===
 
The MemoryRegionOps struct has an old_mmio field which was added to make it simpler to convert devices to the MemoryRegion API (it allows use of separate byte, halfword and word read and write functions, rather than combined read and write functions which take the access size as a parameter). This is still being used by about 40 devices -- it would be nice to convert those to the new style and remove old_mmio entirely.


=== Using VMStateDescription rather than load and save functions ===
=== Using VMStateDescription rather than load and save functions ===
Line 45: Line 34:


* The QError macros QERR_ are a hack to help with the transition to the current error.h API (human-readable message rather than JSON argument, see commit df1e608).  Avoid them in new code, use simple message strings instead.
* The QError macros QERR_ are a hack to help with the transition to the current error.h API (human-readable message rather than JSON argument, see commit df1e608).  Avoid them in new code, use simple message strings instead.
* qerror_report() is a transitional interface to help with converting existing HMP commands to QMP.  It should not be used elsewhere.  Use Error objects instead with error_propagate() and error_setg() instead.


* Use error_report() & friends instead of fprintf(stderr, ...).  Unlike fprintf(), it does the right thing in human monitor context.  It also adds program name and error location when appropriate, and supports timestamps (-msg timestamp=on).
* Use error_report() & friends instead of fprintf(stderr, ...).  Unlike fprintf(), it does the right thing in human monitor context.  It also adds program name and error location when appropriate, and supports timestamps (-msg timestamp=on).


* Avoid error_is_set(errp); it is simpler to just test for non-NULL *errp in the first place.
* Functions that use Error to report errors via an Error **errp parameter should also return a value that indicates success / failure whenever practical, as explained in include/qapi/error.h.


=== QMP legacy interface to the QAPI ===
=== QMP legacy interface to the QAPI ===
Line 56: Line 43:
There are only a few QMP commands missing to be converted from the QMP legacy interface to the QAPI. The most important of them are do_device_add() and do_qmp_capabilities() (which may or may not need session support to be converted). You can find more details in the [http://wiki.qemu.org/QMP#TODO QMP TODO page].
There are only a few QMP commands missing to be converted from the QMP legacy interface to the QAPI. The most important of them are do_device_add() and do_qmp_capabilities() (which may or may not need session support to be converted). You can find more details in the [http://wiki.qemu.org/QMP#TODO QMP TODO page].


=== Makefile ===
=== .gitignore ===
 
While CVS required an ignore file per directory, git allows for a single ignore file at the top of the tree to describe the entire project.  We have an unfortunate mix of using the top file to ignore files in subdirectories, while using nested files to ignore other files.  Ideally, there should be only a single top-level gitignore that covers everything for the project.
 
=== Modern shell scripting ===


Although object specific options are traditionally implemented with [http://www.gnu.org/software/make/manual/make.html#Target_002dspecific Target-specific Variable Values], such as
Various shell files contain a mix between obsolete `` and modern $(); use of `` is only required when using /bin/sh on Solaris. It would be nice to convert to using $() everywhere, or at least in all bash scripts, as well as in all scripts that are known to not be run on Solaris. While at it, there are some other places we can simplify to rely on POSIX shell semantics, such as using $PWD instead of $(pwd).


    $(obj)/arm-a64.o: QEMU_CFLAGS += -I$(libvixldir)
=== I/O error reporting ===


, currently the cflags and libs should be specified as per-object variable in Makefile.objs and friends, which will be automatically expanded into the compiling and the linking commands. E.g. in block/Makefile.objs:
[rw]error lets management take action upon I/O error, for example pausing the VM or taking some action in the host to enlarge a thin-provisioned volume. Not yet supported by qdevified devices with a qdev_prop_drive: isa-fdc, sysbus-fdc, SUNW,fdtwo, nand, onenand, cfi.pflash01, cfi.pflash02, spapr-nvram, scsi-generic, nvme. SD isn't in this list, because it still hasn't been qdevified.  There may be more.


    ...
Even devices that have been qdevified have bugs: SCSI calls bdrv_error_action() when UNMAP fails, but IDE doesn't call it when TRIM fails. IDE and virtio-blk call it for I/O beyond the end of the medium, but SCSI doesn't.
    iscsi.o-cflags    := $(LIBISCSI_CFLAGS)
    iscsi.o-libs      := $(LIBISCSI_LIBS)
    curl.o-cflags      := $(CURL_CFLAGS)
    curl.o-libs        := $(CURL_LIBS)
    ...


where $(obj)/ is not needed compared to the old way.
This is also useful because rerror/werror on block jobs requires rerror/werror on the corresponding device.  rerror/werror are needed to make reporting of block job error events robust (otherwise, block jobs just disappear and, if management misses BLOCK_JOB_COMPLETED events, it cannot poll to know if the job has completed successfully or not). It is not clear why this is a requirement, though.


=== .gitignore ===
=== I/O accounting ===


While CVS required an ignore file per directory, git allows for a single ignore file at the top of the tree to describe the entire projectWe have an unfortunate mix of using the top file to ignore files in subdirectories, while using nested files to ignore other filesIdeally, there should be only a single top-level gitignore that covers everything for the project.
I/O accounting collects data for query-blockstatsDevice models should call bdrv_acct_start() and bdrv_acct_done() to make this workMost of them still don't.


=== Modern shell scripting ===
=== Command line option --readconfig ===


Various shell files contain a mix between obsolete `` and modern $(); use of `` is only required when using /bin/sh on Solaris.  It would be nice to convert to using $() everywhere, or at least in all bash scripts, as well as in all scripts that are known to not be run on Solaris.  While at it, there are some other places we can simplify to rely on POSIX shell semantics, such as using $PWD instead of $(pwd).
The config file doesn't cover all the command line options so far, only the ones implemented with QemuOpts.


== Completed transitions ==
== Completed transitions ==


* QEMUMachine has been replaced with a QOM hierarchy, details on [[Features/QOM/Machine]].
* Character devices have been replaced with a QOM hierarchy.
* Everything is using the MemoryRegion API now!
* Everything is using the MemoryRegion API now!
* Users of old_portio have all been updated and the support removed
* Users of old_portio have all been updated and the support removed
* error_is_set(errp) has been dropped
* MemoryRegionOps old_mmio has been removed
* SysBusDevice::init has been removed (all devices now use instance_init/realize instead)
* ptimer timers no longer use QEMUBH bottom halves
* Guest CPUs implement do_transaction_failed, not do_unassigned_access

Latest revision as of 06:57, 17 March 2022

Code Transitions

This page exists to keep a record of API and style transitions in the QEMU codebase. It's quite common for us to introduce a new API or design guideline, but not be able to convert the whole of QEMU's existing code over to it at once. So at any point in time some of QEMU will still be using the old deprecated approaches.

The aim of this page is twofold:

  • as a quick reference for less frequent contributors, to give an idea of things to avoid copying from old code
  • to embarrass ourselves into maybe finishing off some of these transitions

If you add a new transition to this page, please also briefly mention it in the DeveloperNews page with a link to here.

Ongoing transitions

Making all devices QOM objects

Ideally all device models should be QOM objects (usually deriving from DeviceState or one of its subclasses). However we still have a fairly large number of devices which are coded in an ad-hoc way. These should all be converted to QOM but it is a big job. See Documentation/QOMConventions for the latest guidance on how to structure a QOM device.

Using VMStateDescription rather than load and save functions

The preferred way to implement state save/load for migration is to describe the device state using a VMStateDescription struct. Some devices are still using the old vmstate_register() API, however; these all need converting. (This is often going to involve also converting the device to use QOM.)

Coding whitespace and brace style

New QEMU code should follow the style guidelines in CODING_STYLE, and in particular this means 4-space indent, no hardcoded tab, braces on all if() statements. However much of the codebase is old and doesn't follow this. Changes to areas of existing code should generally update the lines of code they're touching anyway, but we prefer to avoid wholesale "fix coding style" patches because they obscure the change history for tools like "git blame".

Error reporting

Several transitions are in flight here:

  • Avoid ErrorClass values other than ERROR_CLASS_GENERIC_ERROR unless you have a specific reason. Prefer error_setg() & friends over error_set() & friends.
  • The QError macros QERR_ are a hack to help with the transition to the current error.h API (human-readable message rather than JSON argument, see commit df1e608). Avoid them in new code, use simple message strings instead.
  • Use error_report() & friends instead of fprintf(stderr, ...). Unlike fprintf(), it does the right thing in human monitor context. It also adds program name and error location when appropriate, and supports timestamps (-msg timestamp=on).
  • Functions that use Error to report errors via an Error **errp parameter should also return a value that indicates success / failure whenever practical, as explained in include/qapi/error.h.

QMP legacy interface to the QAPI

There are only a few QMP commands missing to be converted from the QMP legacy interface to the QAPI. The most important of them are do_device_add() and do_qmp_capabilities() (which may or may not need session support to be converted). You can find more details in the QMP TODO page.

.gitignore

While CVS required an ignore file per directory, git allows for a single ignore file at the top of the tree to describe the entire project. We have an unfortunate mix of using the top file to ignore files in subdirectories, while using nested files to ignore other files. Ideally, there should be only a single top-level gitignore that covers everything for the project.

Modern shell scripting

Various shell files contain a mix between obsolete `` and modern $(); use of `` is only required when using /bin/sh on Solaris. It would be nice to convert to using $() everywhere, or at least in all bash scripts, as well as in all scripts that are known to not be run on Solaris. While at it, there are some other places we can simplify to rely on POSIX shell semantics, such as using $PWD instead of $(pwd).

I/O error reporting

[rw]error lets management take action upon I/O error, for example pausing the VM or taking some action in the host to enlarge a thin-provisioned volume. Not yet supported by qdevified devices with a qdev_prop_drive: isa-fdc, sysbus-fdc, SUNW,fdtwo, nand, onenand, cfi.pflash01, cfi.pflash02, spapr-nvram, scsi-generic, nvme. SD isn't in this list, because it still hasn't been qdevified. There may be more.

Even devices that have been qdevified have bugs: SCSI calls bdrv_error_action() when UNMAP fails, but IDE doesn't call it when TRIM fails. IDE and virtio-blk call it for I/O beyond the end of the medium, but SCSI doesn't.

This is also useful because rerror/werror on block jobs requires rerror/werror on the corresponding device. rerror/werror are needed to make reporting of block job error events robust (otherwise, block jobs just disappear and, if management misses BLOCK_JOB_COMPLETED events, it cannot poll to know if the job has completed successfully or not). It is not clear why this is a requirement, though.

I/O accounting

I/O accounting collects data for query-blockstats. Device models should call bdrv_acct_start() and bdrv_acct_done() to make this work. Most of them still don't.

Command line option --readconfig

The config file doesn't cover all the command line options so far, only the ones implemented with QemuOpts.

Completed transitions

  • QEMUMachine has been replaced with a QOM hierarchy, details on Features/QOM/Machine.
  • Character devices have been replaced with a QOM hierarchy.
  • Everything is using the MemoryRegion API now!
  • Users of old_portio have all been updated and the support removed
  • error_is_set(errp) has been dropped
  • MemoryRegionOps old_mmio has been removed
  • SysBusDevice::init has been removed (all devices now use instance_init/realize instead)
  • ptimer timers no longer use QEMUBH bottom halves
  • Guest CPUs implement do_transaction_failed, not do_unassigned_access