Contribute/BiteSizedTasks: Difference between revisions

Revision as of 06:22, 22 February 2017

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.

Note: 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.

API conversion

Look for uses of malloc, and convert them to either g_malloc, g_new (more rarely g_try_malloc or g_try_new if a lot of memory is being allocated). Likewise, convert calloc to either g_new0 or g_try_new0. 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.)
Associate external libraries with the object files that actually use them
For all "QEMUTimer*" variables that are initialized with timer_new, change them to "QEMUTimer" and initialize them with timer_init. Drop any timer_free calls (there aren't many, so this patch would fix small memory leaks too). (not quite bite-sized).
Replace calls to functions named cpu_physical_memory_* with address_space_*.
Change net/socket.c to use the functions in include/qemu/sockets.h instead of parse_host_port/bind/connect/listen.
Change QemuMutex and QemuCond to CompatGMutex and CompatGCond (these are the same as GMutex and GCond, just with a different type). With this change, qemu_mutex_init/qemu_cond_init becomes optional for global variables.
Replace calls to object_child_foreach() with object_child_foreach_recursive() when applicable.
Change basename and dirname to g_path_get_basename() and g_path_get_dirname() respectively.
Devices which create MemoryRegions with the old_mmio accessors should be converted to new style mmio accessors.
The ToDo/CodeTransitions page tracks ongoing internal QEMU API transitions. Most are not trivial but it's a good source of ideas.

Header cleanups

avoid including files from include/exec/cpu-common.h.
move declarations out of qemu-common.h for functions declared in utils/ files: e.g. include/qemu/path.h for utils/path.c, include/qemu/id.h for utils/id.c, etc.
Some files include header files multiple times. Try to find such files (e.g. with the shell magic mentioned here: https://lists.nongnu.org/archive/html/qemu-ppc/2016-07/msg00402.html) and prepare patches to remove the duplicated "#include" statements.

Device models

Include SDState by value instead of allocating it in sd_init (hw/sd/).
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
Convert qdev pointer properties (defined with DEFINE_PROP_PTR) to

QOM links. Example: commit 873b4d3.

Error checking

Add checks for NULL return value to uses of qemu_find_file.
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.
Make qemu_thread_create return a flag to indicate if it succeeded rather than failing with an error; make all callers check it.
Make vmstate_save_state return a flag to indicate if it succeeded
Route errors up from the ->put methods that vmstate_save_state calls.

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
block/vmdk.c                         vmdk_write_cid.isra.8            20544 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 code removal

hw/display contains files named *_template.h. These are included many times with different values of the DEPTH macro. However, only the DEPTH == 32 case is used. Remove support for DEPTH != 32 in the template headers and in the file that include them.
Look for functions that are named *_exit or *_exitfn in hw/ and that return int. They should all return zero. Make them return void, and remove the checks for the callers.
Once the above change is done, remove the "Error **" argument from functions named *_unrealize in hw/
Use the technique described at https://blog.flameeyes.eu/2008/01/today-how-to-identify-unused-exported-functions-and-variables to identify unused global functions and variables, try to decide whether it's a good idea to remove the detected code fragments and send (single) patches to remove them if applicable.

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.

Developer conveniences

C11 supports _Static_assert() for implementing compile-time assertions. We currently use a QEMU_BUILD_BUG_ON macro for this which works everywhere but produces somewhat confusing error messages. Add a check to configure for whether the compiler supports _Static_assert() and if so make QEMU_BUILD_BUG_ON() use it. Include examples of before-and-after assertion failure messages in your patch commit message.

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!

./qemu-system-x86_64 -nographic -device cfi.pflash01
(GTK) ./qemu-system-x86_64 -nodefaults and then pick view->show tabs from the menu
(GTK) ./qemu-system-x86_64 -nodefaults and then pick view->zoom in from the menu
Start QEMU with "qemu-system-x86_64 -nographic -M isapc -serial none -monitor stdio" and enter "info lapic" at the monitor prompt ==> Segmentation fault
Start QEMU with "qemu-system-tricore -nographic -M tricore_testboard -S" and enter "x 0" at the monitor prompt ==> Segmentation fault
Starting qemu-system-unicore32 without the -kernel parameter results in an assert()ion ... that should not happen and should be replaced by a proper error message + exit() instead.
Start QEMU with "qemu-system-unicore32 -M puv3,accel=qtest -S -nographic" and enter "x 0 " at the monitor prompt ==> QEMU aborts - this should not happen, QEMU should only print an error message and continue running
Potentially easy bugs from the bugtracker:

@@ Line 25: / Line 25: @@
 * 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
+* Convert qdev pointer properties (defined with DEFINE_PROP_PTR) to
+QOM links.  Example: commit 873b4d3.
 == Error checking ==