Difference between revisions of "BiteSizedTasks"

From Libvirt Wiki
Jump to: navigation, search
(Added: Change testutils.[ch] referencing in tools/Makefile.am)
(GLib replacements)
 
(49 intermediate revisions by 7 users not shown)
Line 1: Line 1:
 
This page tracks introductory tasks for new libvirt contributors. Code cleanups, bugs, and small features are listed with difficulty ranging from trivial to intermediate.
 
This page tracks introductory tasks for new libvirt contributors. Code cleanups, bugs, and small features are listed with difficulty ranging from trivial to intermediate.
  
== Introductory bug reports (LibvirtFirstBug) ==
 
  
Some bugs in the libvirt bugzilla tracker are marked as good introductory bug reports 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 [https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=POST&bug_status=MODIFIED&classification=Community&f1=status_whiteboard&f2=component&list_id=4807856&o1=substring&o2=anywordssubstr&product=Virtualization%20Tools&query_format=advanced&v1=LibvirtFirstBug&v2=libvirt here].
+
== Beginner tasks ==
  
<rss templatename="Template:RSSBugzillaTemplate" max=500>https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=POST&bug_status=MODIFIED&classification=Community&f1=status_whiteboard&f2=component&list_id=4999130&o1=substring&o2=anywordssubstr&order=bug_id&product=Virtualization%20Tools&query_based_on=&query_format=advanced&v1=LibvirtFirstBug&v2=libvirt&ctype=atom</rss>
+
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
== Enhancements ==
+
 
 +
=== 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>
  
==== Many VIR_ERROR()/VIR_WARN() usages should show virGetLastErrorMessage() ====
+
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:
There's many places in the code that call VIR_ERROR and VIR_WARN after a function fails, but they don't report the error message raised by the failing function. virGetLastErrorMessage() will give us the string error from the failing function, so we can use that. Some examples that should be updated:
 
  
* Every VIR_WARN in src/qemu/qemu_capabilities.c function virQEMUCapsInit
+
https://github.com/libvirt/libvirt/commit/612c0d4bb80bf40f82d64057838dd610b78ad306
* Nearly every VIR_WARN and VIR_ERROR in src/qemu/qemu_process.c
 
* Every VIR_WARN in src/lxc/lxc_driver.c
 
  
Example patch: http://www.redhat.com/archives/libvir-list/2016-March/msg00905.html
+
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.
  
==== qemu: Use comma escaping for more command line values ====
+
=== Clean up virBufferCheckError() callers ===
Most qemu command line arguments take the form of -OPTION ARG1=VALUE1,ARG2=VALUE2,... This format presents a problem when one of the values we want to pass has a comma ',' in it. qemu handles this by requiring callers to escape a comma... with another comma! So if we want to pass /path-with-,.foo to qemu, it must be turned into /path-with-,,.foo. We already handle several items like this in libvirt but there's many more that need comma escaping, and there's a convenience function for it called virQEMUBuildBufferEscapeComma.
 
  
Example patches that adds some comma escaping: http://www.redhat.com/archives/libvir-list/2016-October/msg00424.html
+
Mentor: Martin Kletzander <mkletzan@redhat.com>
  
Here's some examples, find their usages in src/qemu/qemu_command.c and add virQEMUBuildBufferEscapeComma calls used in the above patches.
+
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.
  
* info->romfile in qemuBuildRomStr
+
Example patch: https://www.redhat.com/archives/libvir-list/2017-August/msg00562.html
* src->hosts->socket, src->path, src->configFile in qemuBuildNetworkDriveURI
 
* disk->vendor, disk->product in qemuBuildDriveDevStr
 
* fs->src in qemuBuildFSStr
 
* fs->dst in qemuBuildFSDevStr
 
* connect= qemuBuildHostNetStr
 
* fileval handling in qemuBuildChrChardevStr
 
* TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr
 
* TYPE_DEV, TYPE_FILE, TYPE_PIPE, TYPE_UNIX handling in qemuBuildChrArgStr
 
* cfg->vncTLSx509certdir, and cfg->vncTLSx509certdir in qemuBuildGraphicsVNCCommandLine
 
* cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine
 
* data.nix.path in qemuBuildVhostuserCommandLine
 
* places that use strchr in qemuBuildSmartcardCommandLine, can be converted to use virBufferEscape
 
* loader->path, loader->nvram usage qemuBuildDomainLoaderCommandLine
 
  
=== Add *DefNew and functions for all device objects ===
+
=== More conversions to virErrorPreserveLast/virErrorRestore ===
Not all device objects have associated *DefNew functions. For example, in src/conf/domain_conf.c virDomainDiskDefPtr has a virDomainDiskDefNew, but virDomainSmartcardDefPtr doesn't have a similar virDomainSmartcardDefNew. Not all devices strictly require it, but adding it ahead of time simplifies cases in the future when it might be needed.
 
  
As an example, see this commit which added virDomainVideoDefNew: https://github.com/libvirt/libvirt/commit/ac87932ee3776a5c7540b4626cbcccfefb8f8307
+
Mentor: Cole Robinson <crobinso@redhat.com>
  
=== Move virDomainVideoDefaultType logic to individual drivers ===
+
virErrorPreserveLast + virErrorRestore are newer internal APIs for saving and restoring error reports. 'git grep virSaveLastError' and most usages will be candidates for converting.
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 commit for example conversions in qemu_hotplug.c: https://github.com/libvirt/libvirt/commit/6f18150f7b61c3671622d29dba816247454616a1
  
See this example commit that moves logic out of that function and into the qemu driver: https://github.com/libvirt/libvirt/commit/ef08a545388f388b7c76b99a3f3d2584daf05145
+
=== Clean up variables in tools/libvirt-guests.sh.in ===
  
== Code Cleanups ==
+
Mentor: Martin Kletzander <mkletzan@redhat.com>
  
==== Finish conversion to virConfGetValue* functions ====
+
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.
  
We have internal APIs for type specific virConfGetValue* functions, such as virConfGetValueInt, virConfGetValueLLong, etc. A previous series added these and converted most code, but there's still a few other files that need conversion. As of Jan 2018 these include the following files:
+
=== Change testutils.[ch] referencing in tests/Makefile.am ===
  
* src/lxc/lxc_native.c
+
Mentor: Martin Kletzander <mkletzan@redhat.com>
* src/vmx/vmx.c
 
* src/xenconfig/xen_common.c
 
* src/xenconfig/xen_xl.c
 
  
For conversion details, see this [https://www.redhat.com/archives/libvir-list/2016-July/msg00312.html patch posting] . The 'make check' test suite can be used to validate the conversions are correct, it may not need manual testing.
+
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.
  
==== Add and use virGetLastErrorCode() ====
+
Ideas include:
Many places in the code call virGetLastError() just to check the raised error code. However virGetLastError() can return NULL, so the code has to check for that as well. Instead we should add a function called virGetLastErrorCode() that always returns a valid error code, to simplify callers. Follow the semantics of virGetLastErrorMessage() in src/util/virerror.c and convert callers.
 
  
* git grep "err->code" for possible candidates
+
* Create object file of testutils and add that to each tests' testname_LDADD
* git grep "virGetLastError(" for possible candidates
+
* Add it to LDADD in that file so that it's included in all the tests
* Extra credit: add an accompanying virGetLastErrorDomain() and virHasLastError() and remove usage of virGetLastError() almost entirely
+
* Create a variable TESTUTILS and use that instead of testutils.[ch]
 +
* etc.
  
An example patch converting some call sites to virGetLastErrorMessage(), the virGetLastErrorCode() conversion will be similar: http://www.redhat.com/archives/libvir-list/2016-March/msg00896.html
+
=== More conversions to VIR_AUTO(FREE|PTR|CLOSE) aka automatic freeing of resources ===
  
==== Remove virDomainDefNewFull() ====
+
Mentor: Erik Skultety <eskultet@redhat.com>
The function virDomainDefNewFull() in src/conf/domain_conf.c is a thin wrapper around virDomainDefNew() that is only used in a few places in the code. It should be removed, and the function callers just re-implement the minimal functionality it provides.
 
  
==== Split apart virDomainObjAssignDef() ====
+
Both gcc and clang implement the <code>__attribute__((cleanup))</code> which
The virDomainObjAssignDef() function takes 4 arguments, the latter 2 'live' and 'oldDef' are very rarely used. The function should be reworked to provide 3 functions instead:
+
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.
  
* Rename virDomainObjAssignDef() to virDomainObjAssignDefGetOldDef()
+
Some example patches:
* Add virDomainObjAssignDefLive(arg1, arg2) which does virDomainObjAssignDefGetOldDef(arg1, arg2, true, NULL);
+
https://github.com/libvirt/libvirt/commit/86d1f08669225dcd565d291d9a952dab95d681b6
* Add virDomainObjAssignDef(arg1, arg2) which does virDomainObjAssignDefGetOldDef(arg1, arg2, false, NULL);
+
https://www.redhat.com/archives/libvir-list/2018-August/msg00432.html
  
All the callers will need to be adjusted, but it will be much more clear what those callers actually do.
+
=== Switch headers to use #pragma once ===
  
==== Remove NULL checking around EventStateQueue ====
+
Header files currently have a conditional around them
  
We can streamline use of virObjectEventStateQueue and drop many NULL checks sprinkled about the code. The patches should roughly be:
+
  #ifndef LIBVIRT_QEMU_DRIVER_H
 +
  # define LIBVIRT_QEMU_DRIVER_H
 +
  ...snip...
 +
  #endif /* LIBVIRT_QEMU_DRIVER_H */
  
* Edit virObjectEventStateQueue in src/conf/object_event.c to just return if !event. Remove NULL checking from all callers that use it directly
+
Since libvirt mandates GCC or CLang, we can take advantage of this to replace the conditionals with "#pragma once".
* Remove the qemuDomainEventQueue in src/qemu, and replace all callers with virObjectEventStateQueue, and remove all checking for NULL events
 
* Same for src/test/test_driver.c testObjectEventQueue
 
* Same for src/uml/uml_driver.c  umlDomainEventQueue
 
  
==== Remove unused QEMU feature flags ====
+
This also means all other conditionals can have their indent reduced one level  ie "# define FOO" goes to "#define FOO"
In late 2015, libvirt updated its minimal required QEMU version. As a result there are many places in libvirt's QEMU/KVM driver that no longer need to accomodate a potentially missing QEMU feature. Not all of the code handling the conditional features has been removed yet. These conditional features are tracked internally as QEMU_CAPS_* enum values.
 
  
Here's an example patch that removed the QEMU_CAPS_NO_REBOOT check. You can see that all places in the code where we handled 'virQEMUCapsGet(QEMU_CAPS_NO_REBOOT) == false' can now be dropped, since that check is always true. So, lots of code removal :)
+
A couple example patches:
 +
* https://www.redhat.com/archives/libvir-list/2019-April/msg00373.html
 +
* https://www.redhat.com/archives/libvir-list/2019-April/msg00376.html
  
Removal of QEMU_CAPS_NO_REBOOT: https://www.redhat.com/archives/libvir-list/2015-November/msg00170.html
+
== Slightly-less-beginner tasks ==
  
More info:
+
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.
* v1 of the series that dropped old QEMU support: https://www.redhat.com/archives/libvir-list/2015-November/msg00165.html
 
* v2 of the series that dropped old QEMU support: https://www.redhat.com/archives/libvir-list/2015-November/msg00252.html
 
  
List of feature bits that can be removed:
+
=== Move virDomainVideoDefaultType logic to individual drivers ===
* QEMU_CAPS_DRIVE_SERIAL
 
* QEMU_CAPS_DRIVE_AIO
 
* QEMU_CAPS_VNC
 
* QEMU_CAPS_BOOT_MENU
 
* QEMU_CAPS_NAME_PROCESS
 
* QEMU_CAPS_VGA_NONE
 
* QEMU_CAPS_ENABLE_KVM (this is for the -enable-kvm flag. This qemu flag can be compiled out, but we already detect that case with the QEMU_CAPS_KVM check. So this check is redundant and can be removed)
 
  
==== 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.
+
 
 +
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:
  
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.
+
* 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
  
Just a few of the candidates for moving to virDomainDefPostParse:
+
The following drivers need the postparse plumbing added entirely. Your mentor could help with that:
* All the errors in virDomainDefParseXML containing the string "usb is disabled"
 
* All the errors in virDomainDefParseXML containing the string "period must be in range"
 
* All the errors in virDomainDefParseXML containing the string "quota must be in range"
 
* The error "tray is only valid for cdrom and floppy"
 
* The error "removable is only valid for usb disks"
 
* Many other checks in virDomainDiskDefParseXML
 
  
==== Add function that raises error if domain is not active ====
+
* src/test
In libvirt.git, try this command: git grep -i "domain is not running" src/
+
* src/vbox
  
Nearly every one of the over 100 hits there is just an occurence of the idiom:
+
=== Move default Input bus logic to PostParse handling ===
  
    if (!virDomainObjIsActive(vm)) {
+
Mentor: Cole Robinson <crobinso@redhat.com>
        virReportError(VIR_ERR_OPERATION_INVALID,
 
                      "%s", _("domain is not running"));
 
        goto out;
 
    }
 
  
Add a function in src/conf/domain_conf.c, maybe named virDomainObjCheckIsActive that raises that common and returns. Then update the callers to do
+
Similar to what was started with default video model logic in these commits:
  
    if (virDomainObjCheckIsActive(vm) < 0)
+
https://github.com/libvirt/libvirt/commit/a2ca7ca52eb624883ac5a7e1d5deebb43981a410
        goto out;
+
https://github.com/libvirt/libvirt/commit/29a90f071dd7c1764f82c7fcbfdded35d252b174
 +
https://github.com/libvirt/libvirt/commit/ef08a545388f388b7c76b99a3f3d2584daf05145
  
Which will remove 300 lines from the code base :) Don't do it all in one shot; send a patch with the new function and a few conversions first to ensure you aren't going down the wrong path!
+
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
  
==== Split apart qemu_driver.c ====
+
=== Add additional virsh command line completion functions ===
<b>INTERMEDIATE</b>: src/qemu/qemu_driver.c is over 20000 lines long, it would be useful for new and old contributors alike to split the file into several logical components. Some ideas:
 
  
* Move all the *Snapshot* functions to qemu_driver_snapshot.c
+
Mentor: Michal Privoznik <mprivozn@redhat.com>
* Move all the *Migration* functions to qemu_driver_migration.c
 
* Move all the driver init/shutdown routines to qemu_driver_init.c. Everything referenced by qemuStateDriver and qemuConnectDriver
 
* Moving some of the hotplug helper routines to the existing qemu_hotplug.c file
 
  
==== Split apart domain_conf.c ====
+
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:
<b>INTERMEDIATE</b>: src/conf/domain_conf.c is nearly 25000 lines long, it would be useful for new and old contributors alike to split the file into several logical components. Some ideas:
 
  
* Move parsing routines to domain_conf_parse.c
+
virsh # dom<TAB><TAB>
* Move formatting routines to domain_conf_format.c
+
domblkerror        domblkstat          domcontrol          domfsinfo ..
* Move enums to domain_conf_enum.c
 
* Move various utility functions to domain_conf_util.c
 
  
==== virsh: drop usage of #include "virsh-edit.c" ====
+
virsh # start --domain <TAB><TAB>
<b>INTERMEDIATE</b>: The 'virsh *edit' series of commands (virsh edit, virsh net-edit, virsh pool-edit, ...) use an abnormal coding pattern involving various C macros and #include on the virsh-edit.c file. This should be changed so that the same file is not being included over and over again, but its function is called instead, using code callbacks as necessary. To explore a bit, check out:
+
fedora      fedora.i686  freebsd      gentoo ...
  
* tools/virsh-edit.c code
+
Additional functions are needed to improve other commands.
* files that match this command: git grep virsh-edit.c tools/*.c
 
  
==== Abstract away most usage of WITH_GNUTLS ====
+
An example patch: https://www.redhat.com/archives/libvir-list/2018-June/msg01672.html
<b>INTERMEDIATE</b>: The code is filled with WITH_GNUTLS preprocessor macros sprinkled in the middle of functions, altering function signatures, etc. But in libvirt it is customary to use stub functions and abstract structures for this kind of differentiation.
+
A series of example patches: https://www.redhat.com/archives/libvir-list/2018-January/msg00436.html
  
Check out 'git grep WITH_GNUTLS daemon/libvirtd.c'; you will see many uses of WITH_GNUTLS embedded in functions. Instead, find a way to handle most WITH_GNUTLS usage in src/rpc/virnettlscontext.c or similar, and have this be transparent to the other pieces of libvirt that call functions from that file.
+
=== Share cgroup code that is duplicated between QEMU and LXC ===
  
For example, 'git grep WIN32 src/util/virfile.c'; you will see how certain functions are entirely stubbed out if we are running on WIN32... this way other places in libvirt code don't need to conditionalize their usage of virfile.c APIs on whether we are WIN32 or not. That should be the example to follow for WITH_GNUTLS as well.
+
Mentor: Cole Robinson <crobinso@redhat.com>
  
==== Share cgroup code that is duplicated between QEMU and LXC ====
+
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:
<b>INTERMEDIATE</b>: 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
 
* virLXCCgroupSetupBlkioTune() and qemuSetupBlkioCgroup() are similar
 
* virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are similar
 
* virLXCCgroupSetupCpuTune() and qemuSetupCpuCgroup() are similar
  
==== Share duplicated daemon code ====
+
=== Share duplicated daemon code ===
<b>INTERMEDIATE</b>: 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:
+
 
 +
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 *ForkIntoBackground functions
Line 185: Line 188:
 
* the *SetupSignals functions
 
* the *SetupSignals functions
 
* the *DaemonClientNew functions
 
* the *DaemonClientNew functions
* the *UnicSocketPaths functions
+
* the *UnixSocketPaths functions
 +
 
 +
== Bugs and feature requests from bugzilla (LibvirtFirstBug) ==
  
==== Clean up virQEMUDriverPtr parameters ====
+
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 [https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=POST&bug_status=MODIFIED&classification=Community&f1=status_whiteboard&f2=component&list_id=4807856&o1=substring&o2=anywordssubstr&product=Virtualization%20Tools&query_format=advanced&v1=LibvirtFirstBug&v2=libvirt here].
<b>EXTREMELY BEGINNER</b>: Many functions in the QEMU driver code (src/qemu/) take both a domain object and a driver as a parameter. However, since 2e6ecba1bcac, a pointer to the driver structure is saved in private data of the domain object. That can be used instead of a parameter being passed to various functions (a git grep for 'virQEMUDriverPtr driver,' shows some applicable candidates).
 
  
Example patch: https://www.redhat.com/archives/libvir-list/2017-July/msg01068.html
+
<rss templatename="Template:RSSBugzillaTemplate" max=500>https://bugzilla.redhat.com/buglist.cgi?bug_status=NEW&bug_status=ASSIGNED&bug_status=POST&bug_status=MODIFIED&classification=Community&f1=status_whiteboard&f2=component&list_id=4999130&o1=substring&o2=anywordssubstr&order=bug_id&product=Virtualization%20Tools&query_based_on=&query_format=advanced&v1=LibvirtFirstBug&v2=libvirt&ctype=atom</rss>
  
==== Clean up virBufferCheckError() callers ====
+
== Ongoing code transitions ==
<b>BEGINNER</b>: 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
+
=== GLib replacements ===
 +
 
 +
In 2019 libvirt began using GLib internally. We can use GLib APIs to drop some libvirt-specific code.
 +
 
 +
==== GKeyFile ====
 +
 
 +
Use the [https://developer.gnome.org/glib/stable/glib-Key-value-file-parser.html g_key_file_* APIs] to replace libvirt's virkeyfile.c code
 +
 
 +
==== Path functions ====
 +
 
 +
Functions in virfile.c  related to path handling can be replaced with [https://developer.gnome.org/glib/stable/glib-Miscellaneous-Utility-Functions.html g_path_* APIs]. The callers should be made to invoke the GLib APIS directly.
 +
 
 +
==== XAttr / ACLs ====
 +
 
 +
Replace virfile.c XAttr and ACL handling with [https://developer.gnome.org/gio/stable/gio-GFileAttribute.html GFileAttribute APIs]
 +
 
 +
==== Threads ====
  
==== More conversions to virErrorPreserveLast/virErrorRestore ====
+
The callers of the various virMutex, virThread, virCond, virOnce, etc APIs should be switched to use the GLib threading APIs.
  
virErrorPreserveLast + virErrorRestore are newer internal APIs for saving and restoring error reports. 'git grep virSaveLastError' and most usages will be candidates for converting.
+
=== g_autofree and g_autoptr usage ===
  
See this commit for example conversions in qemu_hotplug.c: https://github.com/libvirt/libvirt/commit/6f18150f7b61c3671622d29dba816247454616a1
+
g_autofree is a feature provided by GLib and the C compiler which will free a variable when it goes out of scope.
 +
g_autoptr works similarly, but it will call a custom registred 'free' function instead.
  
==== Clean up variables in tools/libvirt-guests.sh.in ====
+
We are slowly converting the code to use these new patterns. Any variable that is allocated and free'd within a single function can likely be converted to use g_autofree or g_autoptr. Similarly, any usage of VIR_FREE under a 'cleanup:' can likely be converted.
  
<b>BEGINNER</b>: 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.
+
* [https://www.redhat.com/archives/libvir-list/2019-October/msg01081.html Example series of g_autofree cleanups]
 +
* [https://github.com/libvirt/libvirt/commit/ac89b0549ea9f3c75f2df8571f77bd39d8094dea Example registering a g_autoptr function]
 +
* [https://www.redhat.com/archives/libvir-list/2019-October/msg01464.html Example g_autoptr code coversions]
  
==== Change testutils.[ch] referencing in tools/Makefile.am ====
+
=== gnulib removal ===
  
<b>BEGINNER</b>: 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.
+
We are working to remove usage of [https://www.gnu.org/software/gnulib/ gnulib] from libvirt. There are several wrapper APIs provided by gnulib that have rough equivalents in GLib. [https://github.com/libvirt/libvirt/blob/master/bootstrap.conf bootstrap.conf in libvirt.git] contains the list of gnulib modules we are still using & suggested replacement strategies
  
Ideas include:
+
'''Example commits'''
  
* Create object file of testutils and add that to each tests' testname_LDADD
+
* [https://www.redhat.com/archives/libvir-list/2019-November/msg00979.html Example conversions from c_* string functions to GLib g_ascii_* functions]
* Add it to LDADD in that file so that it's included in all the tests
+
* [https://www.redhat.com/archives/libvir-list/2019-November/msg00620.html Example conversions of setenv/unsetenv to GLib equivalents]
* Create a variable TESTUTILS and use that instead of testutils.[ch]
 
* etc.
 

Latest revision as of 15:09, 17 January 2020

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.

Ongoing code transitions

GLib replacements

In 2019 libvirt began using GLib internally. We can use GLib APIs to drop some libvirt-specific code.

GKeyFile

Use the g_key_file_* APIs to replace libvirt's virkeyfile.c code

Path functions

Functions in virfile.c related to path handling can be replaced with g_path_* APIs. The callers should be made to invoke the GLib APIS directly.

XAttr / ACLs

Replace virfile.c XAttr and ACL handling with GFileAttribute APIs

Threads

The callers of the various virMutex, virThread, virCond, virOnce, etc APIs should be switched to use the GLib threading APIs.

g_autofree and g_autoptr usage

g_autofree is a feature provided by GLib and the C compiler which will free a variable when it goes out of scope. g_autoptr works similarly, but it will call a custom registred 'free' function instead.

We are slowly converting the code to use these new patterns. Any variable that is allocated and free'd within a single function can likely be converted to use g_autofree or g_autoptr. Similarly, any usage of VIR_FREE under a 'cleanup:' can likely be converted.

gnulib removal

We are working to remove usage of gnulib from libvirt. There are several wrapper APIs provided by gnulib that have rough equivalents in GLib. bootstrap.conf in libvirt.git contains the list of gnulib modules we are still using & suggested replacement strategies

Example commits