BiteSizedTasks

From Libvirt Wiki
Revision as of 23:42, 5 April 2019 by ColeRobinson (talk | contribs) (Link to some '#pragma once' examples)
(diff) ←Older revision | view current revision (diff) | Newer revision→ (diff)
Jump to: navigation, search

This page tracks introductory tasks for new libvirt contributors. Code cleanups, bugs, and small features are listed with difficulty ranging from trivial to intermediate.


Beginner tasks

These tasks are good first introductions to libvirt code. You will be required to get things building and make sure the test suite passes, but not necessarily need to understand the details of what libvirt is doing. Mostly these are following existing example changes that are already in the code. Contact the listed mentor with any questions

Move validation checks out of domain XML parsing

Mentor: Cole Robinson <crobinso@redhat.com>

The code that handles domain/VM XML parsing (virDomainDefParseXML in src/domain/domain_conf.c) has various validation checks sprinkled within it. Many of these checks should be moved out of the parse step and into virDomainDefPostParse, so they can be shared by various places in the code that build a domain definition without XML, such as converting from native vmware/virtualbox/xen formats.

Not all validation checks can or should be moved. You'll need to determine if the check is specific to the actual XML parsing, or is it general for all VM configurations. So anything that throws an error if it hits unexpected XML should not be moved.

Just a few of the candidates for moving to virDomainDefPostParse:

  • All the checks against dom->os.type and dom->virtType in virDomainInputDefParseXML should go into a virDomainInputDefValidate function
  • The error "tray is only valid for cdrom and floppy" in virDomainDiskDefParseXML
  • 'disk vendor' and 'disk product' errors in virDomainDiskDefParseXML
  • host->socket checks in virDomainStorageNetworkParseHost
  • all the option collision 'total and...' error messages in virDomainDiskDefIotuneParse
  • 'pci-root and pcie-root controllers should not' in virDomainControllerDefParseXML
  • PCI chassisNr, chassis, port, busNr, numaNode range checks in virDomainControllerDefParseXML
  • "Controllers must use the 'ccid' address type" in virDomainSmartcardDefParseXML
  • any 'not available' or 'valid only' errors in virDomainGraphicsListenDefParseXML
  • All the 'only supported' errors in virDomainVideoDefParseXML
  • size validation in virDomainMemoryTargetDefParseXML
  • 'must be in range' errors in virDomainDefParseBootXML
  • 'You must map the root user of container' in virDomainIdmapDefParseXML
  • 'duplicate blkio device path' error
  • 'Only one primary video device is supported' error

You can see an example commit here: https://github.com/libvirt/libvirt/commit/265e6924f569431d431981c4c0d8d68baf0fb2a2

Centralize duplicate code for validating URI paths

Mentor: Cole Robinson <crobinso@redhat.com>

Many drivers have duplicate code for validating whether the passed in libvirt URI uses a proper '/session' or '/system' path. For example look at code flagged by this command: git grep '_("un.*:///session'

A shared function could be added to src/driver.c, virConnectValidateURIPath(virConnectPtr conn), that will centralize the duplicate checking and error reporting. This will help remove some strings that translators need to adjust, and remove ~100 lines of duplicate code.

More usage of virStringParseYesNo

Mentor: Cole Robinson <crobinso@redhat.com>

There's a common pattern in our XML parsing to convert string 'yes' to true and 'no' to false, and error if we receive anything other than those values. To find instances, try git grep "(STREQ(.*, \"yes\")" and git grep "(STREQ(.*, \"no\")". A function virStringParseYesNo was added to do this conversion, and several conversions were already done:

https://github.com/libvirt/libvirt/commit/612c0d4bb80bf40f82d64057838dd610b78ad306

For most of the remaining usages in domain_conf.c, only "yes" is explicitly checked for. This means all other values are implictly handled as 'false'. In this case, use virStringParseYesNo, but if it returns -1, don't raise an error but set the bool value to false.

Clean up virBufferCheckError() callers

Mentor: Martin Kletzander <mkletzan@redhat.com>

There is a concept of dynamically allocated buffer in libvirt, called virBuffer. One of its advantages is that functions working with this buffer do not report error or have any return value. The error is stored in the structure itself. While long-winded manipulations with virBuffer are easy and clean thanks to this (no need to check for error after every single append of data), the disadvantage is that every time the data is received from the buffer there needs to be check for the possibility of an error and the data received. These two things are not always needed both or done together, however in most places it is. While this handling needed a condition and a check, it is now a little bit easier to do this and older code should be cleaned up to use the new approach.

Example patch: https://www.redhat.com/archives/libvir-list/2017-August/msg00562.html

More conversions to virErrorPreserveLast/virErrorRestore

Mentor: Cole Robinson <crobinso@redhat.com>

virErrorPreserveLast + virErrorRestore are newer internal APIs for saving and restoring error reports. 'git grep virSaveLastError' and most usages will be candidates for converting.

See this commit for example conversions in qemu_hotplug.c: https://github.com/libvirt/libvirt/commit/6f18150f7b61c3671622d29dba816247454616a1

Clean up variables in tools/libvirt-guests.sh.in

Mentor: Martin Kletzander <mkletzan@redhat.com>

The script has many functions that just redeclare variables all over the place instead of making them local to the function using the `local` keyword. There might be some errors due to the rewriting of the variables and it's also prone to error, so all such variables should be marked `local`. There is at least one function that returns an information in a variable. Such occurrences might be fixed up or kept that way.

Change testutils.[ch] referencing in tests/Makefile.am

Mentor: Martin Kletzander <mkletzan@redhat.com>

Go through the rules in the file and see how can the files `testutils.[ch]` be referenced so that the rules are smaller. There are various ways how to do that and it is left as an exercise to the implementer to figure out which one is the best. And to possibly discuss that on the list.

Ideas include:

  • Create object file of testutils and add that to each tests' testname_LDADD
  • Add it to LDADD in that file so that it's included in all the tests
  • Create a variable TESTUTILS and use that instead of testutils.[ch]
  • etc.

More conversions to VIR_AUTO(FREE|PTR|CLOSE) aka automatic freeing of resources

Mentor: Erik Skultety <eskultet@redhat.com>

Both gcc and clang implement the __attribute__((cleanup)) which triggers the defined function call whenever variable marked with this attribute goes out of the scope. This can be leveraged in a number of scenarios, but in libvirt this is mainly about memory deallocation in an automated manner. Internally we use macros named VIR_AUTOFREE, VIR_AUTOPTR, and VIR_AUTOCLOSE to use these features.

Some example patches: https://github.com/libvirt/libvirt/commit/86d1f08669225dcd565d291d9a952dab95d681b6 https://www.redhat.com/archives/libvir-list/2018-August/msg00432.html

Switch headers to use #pragma once

Header files currently have a conditional around them

 #ifndef LIBVIRT_QEMU_DRIVER_H
 # define LIBVIRT_QEMU_DRIVER_H
 ...snip...
 #endif /* LIBVIRT_QEMU_DRIVER_H */

Since libvirt mandates GCC or CLang, we can take advantage of this to replace the conditionals with "#pragma once".

This also means all other conditionals can have their indent reduced one level ie "# define FOO" goes to "#define FOO"

A couple example patches:

Slightly-less-beginner tasks

These tasks are slightly more involved than the beginner tasks, but still don't require extensive libvirt knowledge. If any of the beginner tasks seem too simple, maybe try one of these. Talk with the listed mentor if you have questions.

Move virDomainVideoDefaultType logic to individual drivers

Mentor: Cole Robinson <crobinso@redhat.com>

The virDomainVideoDefaultType function in src/conf/domain_conf.c shouldn't be there :) The logic setting a device default should be in the post parse function of individual driver code.

See this example commit that moves logic out of that function and into the src/qemu: https://github.com/libvirt/libvirt/commit/ef08a545388f388b7c76b99a3f3d2584daf05145

Similar changes need to be made for the following drivers:

  • src/bhyve: move the BHYVE logic out of virDomainVideoDefaultType and into bhyveDomainDeviceDefPostParse
  • src/libxl: move the XEN logic out of virDomainVideoDefaultType and into libxlDomainDeviceDefPostParse
  • src/vz: move the VZ and PARALLELS logic out of virDomainVideoDefaultType and into vzDomainDeviceDefPostParse
  • src/vmx and src/vmware: move the VMWARE logic out of virDomainVideoDefaultType and into
virVMXDomainDevicesDefPostParse and vmwareDomainDeviceDefPostParse

The following drivers need the postparse plumbing added entirely. Your mentor could help with that:

  • src/test
  • src/vbox

Move default Input bus logic to PostParse handling

Mentor: Cole Robinson <crobinso@redhat.com>

Similar to what was started with default video model logic in these commits:

https://github.com/libvirt/libvirt/commit/a2ca7ca52eb624883ac5a7e1d5deebb43981a410 https://github.com/libvirt/libvirt/commit/29a90f071dd7c1764f82c7fcbfdded35d252b174 https://github.com/libvirt/libvirt/commit/ef08a545388f388b7c76b99a3f3d2584daf05145

Break out the default bus setting logic from virDomainInputDefParseXML and move it to PostParse handling. Look for anything that sets a def->bus value in that function, it's grounds for removal

Add additional virsh command line completion functions

Mentor: Michal Privoznik <mprivozn@redhat.com>

Completers are small functions that are called from the virsh command line tool whenever the user started typing something and hit TAB TAB. Their purpose is to offer list of strings that suit entered input. If there is only one item on the list then it's entered in automatically. For instance:

virsh # dom<TAB><TAB> domblkerror domblkstat domcontrol domfsinfo ..

virsh # start --domain <TAB><TAB> fedora fedora.i686 freebsd gentoo ...

Additional functions are needed to improve other commands.

An example patch: https://www.redhat.com/archives/libvir-list/2018-June/msg01672.html A series of example patches: https://www.redhat.com/archives/libvir-list/2018-January/msg00436.html

Share cgroup code that is duplicated between QEMU and LXC

Mentor: Cole Robinson <crobinso@redhat.com>

Both QEMU and LXC drivers have some similar code when dealing with cgroups. See various routines in src/lxc/lxc_cgroup.c and src/qemu/qemu_cgroup.c, there is lots of opportunity for centralizing functionality in src/util/vircgroup.c. For example:

  • virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() are similar
  • virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are similar

Share duplicated daemon code

Mentor: Cole Robinson <crobinso@redhat.com>

Libvirt ships several daemons (libvirtd, virtlogd, virtlockd) that all have similar code around general daemon startup. Right now the code is duplicated for each daemon, but it could be shared, like in a new file src/util/virdaemon.c. The main files to look in are src/logging/log_daemon.c, src/locking/lock_daemon.c, and daemon/libvirtd.c. Some example functions to look at:

  • the *ForkIntoBackground functions
  • the *SetupLogging functions
  • the *SetupSignals functions
  • the *DaemonClientNew functions
  • the *UnixSocketPaths functions

Bugs and feature requests from bugzilla (LibvirtFirstBug)

Some bugs in the libvirt bugzilla tracker are marked as good introductory bug reports or features for new contributors. These bugs have the 'LibvirtFirstBug' tag. Each bug report contains info and sample commits to point you in the right direction. You can open the list in your browser bugzilla here.