Contribute/BiteSizedTasks: Difference between revisions

From QEMU
(Remove old task regarding use qemu/sockets.h functions in net_socket_listen_init()/net_socket_connect_init(). This task was attempted and reverted again in 2017, see commit 6701e5514beab7b781a10424a.)
(Removed done code-transition tasks)
Line 9: Line 9:
* Replace calls to object_child_foreach() with object_child_foreach_recursive() when applicable: nvdimm_device_list, nmi_monitor_handle, find_sysbus_device, pc_dimm_slot2bitmap, build_dimm_list.
* Replace calls to object_child_foreach() with object_child_foreach_recursive() when applicable: nvdimm_device_list, nmi_monitor_handle, find_sysbus_device, pc_dimm_slot2bitmap, build_dimm_list.
* The [[ToDo/CodeTransitions]] page tracks ongoing internal QEMU API transitions. Most are not trivial but it's a good source of ideas, and some items should be doable for newcomers, too, e.g.:
* The [[ToDo/CodeTransitions]] page tracks ongoing internal QEMU API transitions. Most are not trivial but it's a good source of ideas, and some items should be doable for newcomers, too, e.g.:
** Modern shell scripting (use $() instead of ``)
** Coding whitespace style: Some files in the util/ and other folders use TABs for indentation instead of spaces, so sending patches that affect these files usually trigger a warning by the checkpatch.pl script. Identify the files with the wrong indentations and send a patch to adapt the files to the QEMU coding style conventions (i.e. indent with 4 spaces instead of TABs). (contact: THH)
** Use DeviceClass::realize rather than SysBusDeviceClass::init
** Coding whitespace style: Some files in the util/ and slirp/ folder use TABs for indentation instead of spaces, so sending patches that affect these files usually trigger a warning by the checkpatch.pl script. Identify the files with the wrong indentations and send a patch to adapt the files to the QEMU coding style conventions (i.e. indent with 4 spaces instead of TABs). (contact: THH)
* Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h. FALSE and TRUE (with capital letters) are the constants defined by glib for being used with the "gboolean" type of glib. So code that uses TRUE or FALSE for "gboolean" variables or calls functions from glib is fine. But some parts of the code also use TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>). These code parts should be using ''true'' and ''false'' (without capital letters) instead. (contact: THH)
* Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h. FALSE and TRUE (with capital letters) are the constants defined by glib for being used with the "gboolean" type of glib. So code that uses TRUE or FALSE for "gboolean" variables or calls functions from glib is fine. But some parts of the code also use TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>). These code parts should be using ''true'' and ''false'' (without capital letters) instead. (contact: THH)


Revision as of 04:50, 29 March 2019

Note: Before starting on one of these tasks, it would be wise to double-check the list archives to ensure no one else has recently submitted similar cleanups for the same task. And before submitting a patch to the mailing list, please make sure that you've read and understood the Contribute/SubmitAPatch page.

API conversion

  • Remove leading underscores from #defines: Identifiers with leading underscores followed by another underscore or a capital letter are reserved by the C standard. Use something like grep 'define[ \t]*_[A-Z_]' $(find -name \*.h | grep -v roms | grep -v linux) to find files that might violate this rule, double-check whether the file should be cleaned up or not (some files that are derived from other sources like the Linux headers should not be changed), and then send patches to remove the leading underscores if it is OK there. (contact: THH)
  • Look for uses of malloc, and convert them to g_new or similar (see HACKING for more details on allocation interface usage). Likewise, convert calloc to g_new0 and friends. Drop return value checks unless using g_try_new/g_try_new0. (This needs to be done for one data structure at a time, because an allocation with malloc must be freed with free but an allocation with g_malloc must be freed with g_free. So you need to do a bit of analysis of the code to see where an allocation may be later freed.) Please ignore the mallocs in libdecnumber and tests/tcg/.
  • Associate external libraries with the object files that actually use them
  • Replace calls to functions named cpu_physical_memory_* with address_space_*.
  • Replace calls to object_child_foreach() with object_child_foreach_recursive() when applicable: nvdimm_device_list, nmi_monitor_handle, find_sysbus_device, pc_dimm_slot2bitmap, build_dimm_list.
  • The ToDo/CodeTransitions page tracks ongoing internal QEMU API transitions. Most are not trivial but it's a good source of ideas, and some items should be doable for newcomers, too, e.g.:
    • Coding whitespace style: Some files in the util/ and other folders use TABs for indentation instead of spaces, so sending patches that affect these files usually trigger a warning by the checkpatch.pl script. Identify the files with the wrong indentations and send a patch to adapt the files to the QEMU coding style conventions (i.e. indent with 4 spaces instead of TABs). (contact: THH)
  • Clean up wrong usage of FALSE and TRUE in places that use "bool" from stdbool.h. FALSE and TRUE (with capital letters) are the constants defined by glib for being used with the "gboolean" type of glib. So code that uses TRUE or FALSE for "gboolean" variables or calls functions from glib is fine. But some parts of the code also use TRUE and FALSE for variables that are declared as "bool" (the type from <stdbool.h>). These code parts should be using true and false (without capital letters) instead. (contact: THH)

Header cleanups

Device models

  • Categorize devices: Run "qemu-system-x86_64 -device help" or "qemu-system-arm -M none -device help" and look for devices that are in the "uncategorized devices" section. Ideally, each device should have a category, even if it's just the "misc devices" category. Identify the source file of such an uncategorized devices and set an appropriate DEVICE_CATEGORY bit in its device class categories field (use "grep -r DEVICE_CATEGORY hw/" to see some examples). (contact: THH)
  • Look for invocations of qemu_system_reset_request() in hw/. Whenever they correspond to some kind of watchdog that has triggered, change to watchdog_perform_action().
  • "QOMify" devices: Some devices are still not adapted to the recent QEMU Object Model (QOM) yet. Try to identify such devices and convert them to the recent QOM device model. Details can be found in Andreas Färber's talk during KVM Forum 2013: https://www.youtube.com/watch?v=9LXvZOrHwjw . Note that this is a rather advanced task already. (contact: THH)
  • Convert qdev pointer properties (defined with DEFINE_PROP_PTR) to QOM links. Example: commit 873b4d3.
  • Add support for attribute((bitwise)), tag structure fields and use that for static checking of structure endian-ness accesses

Error checking

  • Add checks for negative return value to uses of load_image_targphys, get_image_size, event_notifier_init, msix_init.
  • Use qemu_strtol/qemu_strtoul/qemu_strtoll/qemu_strtoull more. (contact: eblake)
  • Make qemu_thread_create return a flag to indicate if it succeeded rather than failing with an error; make all callers check it.
  • Using "NULL" for errp in case "it cannot fail" should be avoided. If there is an unexpected error generated, it will simply be ignored. Making use of "&error_abort" instead allows us to catch unexpected errors.
  • Dereferencing errp to check for errors is wrong "because errp may be NULL!". Making use of a local Error * and call error_propagate() instead, as explained in "qapi/error.h".
  • Look for code that checks for errno == EINTR. If the code is simply doing something like 'do { x = expr(); } while (x == -1 && errno == EINTR);', you can replace the surrounding loop with the TFR() macro from include/qemu-common.h instead.
  • strerror() is not thread safe (completely unsafe in FreeBSD, while on glibc it threadsafe for defined errno values but non-safe for out-of-range values); the code base needs to be audited for a conversion to strerror_r(), with the further wrinkle that glibc strerror_r() is a different signature if _GNU_SOURCE is defined

Large frames

  • The following functions have very large stack frames (as obtained with -Wstack-usage=16383), mostly due to huge arrays. Make the stack array smaller and allocate on the heap in the rare case that the data does not fit in the small array:
hw/dma/xilinx_axidma.c               stream_process_mem2s             16480 bytes
hw/net/virtio-net.c                  virtio_net_receive               16544 bytes
net/tap.c                            net_init_tap                     16752 bytes
hw/display/vmware_vga.c              vmsvga_fifo_run                  20688 bytes
hw/virtio/virtio.c                   virtqueue_pop                    24768 bytes
hw/net/virtio-net.c                  virtio_net_flush_tx              32928 bytes
hw/virtio/virtio.c                   qemu_get_virtqueue_element       49216 bytes
hw/virtio/virtio.c                   qemu_put_virtqueue_element       49216 bytes
hw/net/opencores_eth.c               open_eth_start_xmit              65664 bytes
hw/net/spapr_llan.c                  h_send_logical_lan               ~64Kbytes (uses alloca)
hw/arm/nseries.c                     n8x0_init                        65728 bytes
net/net.c                            nc_sendv_compat                  69680 bytes
net/socket.c                         net_socket_send                  69712 bytes
For a larger list of functions with stack sizes of 1K or bigger, see http://article.gmane.org/gmane.comp.emulators.qemu/314061 (dead link)

Dead code removal

Compiler-driver cleanups

  • Use of -Wshadow while compiling can prevent legitimate bugs, but can't be enabled until we first clean up the code base to avoid shadowed declarations.
  • Use of -Wvla and -Wframe-larger-than=4096 can prevent the use of potential security flaws caused by stack overflows made possible with variable-length arrays or other over-large stack allocations.

Tracing

  • Add tracepoints. All functions that are named something_helper, and all functions mentioned in MemoryRegionOps are good candidates.

Bitrot prevention

  • Files with conditional debug statements should ensure that the printf is always compiled, and merely hidden behind if (0) when not debugging, rather than #ifdef'd out. This prevents bitrot of the format string of the debug statement. See this (commit c691320faa6) for an example. Or, go one step further and convert the debug statements to tracepoints. (contact: eblake)

Functions that should be static

  • For example: vga_sync_dirty_bitmap, WC_CONFIG_STRING, WC_FULL_CONFIG_STRING, WC_MODEL_STRING, timer_put, vfio_region_ops, vfio_reset_handler, cpu_gen_init, monitor_read_bdrv_key_start, pdu_marshal, pdu_unmarshal, qcrypto_hmac_alg_map (crypto/hmap-nettle.c), spapr_tce_set_need_vfio, virtqueue_map, vnc_disconnect_finish, vnc_display_init, gicv3_full_update_noirqset, cpu_disable_ticks, cpu_enable_ticks, qemu_thread_exit (actually unused in thread-posix.c), qemu_egl_init_dpy_mesa, monitor_defs (3 occurrences), ppc405cr_init (unused?), apic_deliver_irq, pcie_host_mmcfg_map, pcie_host_mmcfg_unmap
  • tracetool-generated arrays (e.g. hw_mem_trace_events in the generated file hw/mem/trace.c) should be static.
  • The property types defined in hw/core/qdev-properties-system.c should be moved to other directories (e.g. hw/net for network-related property types). After doing this, some functions probably could become static, too.

Consistent option usage in documentation

qemu uses getopt_long_only(), which means that '-help' and '--help' are parsed identically; however, the short form is considered a crutch. Meanwhile, other binaries in the qemu package use getopt_long(), which requires the double-dash form. For consistency, all documentation examples should mention the long spelling with two dashes, especially since some options (like --object) have the same syntax between multiple binaries. (contact: eblake)

Potentially easy bugs

These are bugs that are really easy to reproduce, don't require any complicated setup and are probably some simple missing check - but there again we've not looked at them yet!

Coroutines

QEMU uses coroutines for asynchronous operations, especially in the block layer (disk I/O, disk image formats, network storage protocols, etc). The basics of coroutines are explained here.

  • All functions that may yield (either directly or indirectly through a function they call) must be marked coroutine_fn in their function signature. There is no build-time check for coroutine_fn, so it is accidentally missing in some places. Search the code for function names that contain _co_ or end with _co() or functions that call qemu_coroutine_yield(), qemu_co_queue_wait(), or any other coroutine_fn. Add coroutine_fn to the function signature if it is missing.

Contact persons

Some of these tasks are described very briefly and somewhat cryptically. If you're not sure what a task involves, then feel free to ask for clarification on the qemu-devel mailing list, or the contact person for the task (if the task has been marked with a contact):

  • THH: Thomas Huth (mail: thuth AT redhat.com -- IRC: th_huth)
  • eblake: Eric Blake (mail: eblake AT redhat.com -- IRC: eblake)
Retrieved from "https://wiki.qemu.org/index.php?title=Contribute/BiteSizedTasks&oldid=8490"