Difference between revisions of "BiteSizedTasks"
(Update the comma escape patch list)
|Line 98:||Line 98:|
* The error "removable is only valid for usb disks"
* The error "removable is only valid for usb disks"
* Many other checks in virDomainDiskDefParseXML
* Many other checks in virDomainDiskDefParseXML
==== Split apart qemu_driver.c ====
==== Split apart qemu_driver.c ====
Revision as of 07:42, 23 April 2018
This page tracks introductory tasks for new libvirt contributors. Code cleanups, bugs, and small features are listed with difficulty ranging from trivial to intermediate.
- 1 Introductory bug reports (LibvirtFirstBug)
- 2 Enhancements
- 3 Code Cleanups
- 3.1 Finish conversion to virConfGetValue* functions
- 3.2 Add and use virGetLastErrorCode()
- 3.3 Remove virDomainDefNewFull()
- 3.4 Split apart virDomainObjAssignDef()
- 3.5 Remove NULL checking around EventStateQueue
- 3.6 Move validation checks out of domain XML parsing
- 3.7 Split apart qemu_driver.c
- 3.8 Split apart domain_conf.c
- 3.9 virsh: drop usage of #include "virsh-edit.c"
- 3.10 Abstract away most usage of WITH_GNUTLS
- 3.11 Share cgroup code that is duplicated between QEMU and LXC
- 3.12 Share duplicated daemon code
- 3.13 Clean up virQEMUDriverPtr parameters
- 3.14 Clean up virBufferCheckError() callers
- 3.15 More conversions to virErrorPreserveLast/virErrorRestore
- 3.16 Clean up variables in tools/libvirt-guests.sh.in
- 3.17 Change testutils.[ch] referencing in tests/Makefile.am
- 3.18 Remove redundant uses of *.la in Makefiles
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 here.
Failed to load RSS feed from 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: Error parsing XML for RSS
Many VIR_ERROR()/VIR_WARN() usages should show virGetLastErrorMessage()
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
- Nearly every VIR_WARN and VIR_ERROR in src/qemu/qemu_process.c
- Every VIR_WARN in src/lxc/lxc_driver.c
qemu: Use comma escaping for more command line values
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 More here: https://www.redhat.com/archives/libvir-list/2018-April/msg01534.html
Here's some examples, find their usages in src/qemu/qemu_command.c and add virQEMUBuildBufferEscapeComma calls used in the above patches.
- src->hosts->socket, src->path, src->configFile in qemuBuildNetworkDriveURI
- connect= qemuBuildHostNetStr
- fileval handling in qemuBuildChrChardevStr
- TYPE_DEV, TYPE_PIPE handling in qemuBuildChrChardevStr
- cfg->spiceTLSx509certdir in qemuBuildGraphicsSPICECommandLine
- data.nix.path in qemuBuildVhostuserCommandLine
- places that use strchr in qemuBuildSmartcardCommandLine, can be converted to use virBufferEscape
Add *DefNew and functions for all device objects
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
Move virDomainVideoDefaultType logic to individual drivers
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 qemu driver: https://github.com/libvirt/libvirt/commit/ef08a545388f388b7c76b99a3f3d2584daf05145
Finish conversion to virConfGetValue* functions
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:
For conversion details, see this patch posting . The 'make check' test suite can be used to validate the conversions are correct, it may not need manual testing.
Add and use virGetLastErrorCode()
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
- git grep "virGetLastError(" for possible candidates
- Extra credit: add an accompanying virGetLastErrorDomain() and virHasLastError() and remove usage of virGetLastError() almost entirely
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
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()
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:
- Rename virDomainObjAssignDef() to virDomainObjAssignDefGetOldDef()
- Add virDomainObjAssignDefLive(arg1, arg2) which does virDomainObjAssignDefGetOldDef(arg1, arg2, true, NULL);
- Add virDomainObjAssignDef(arg1, arg2) which does virDomainObjAssignDefGetOldDef(arg1, arg2, false, NULL);
All the callers will need to be adjusted, but it will be much more clear what those callers actually do.
Remove NULL checking around EventStateQueue
We can streamline use of virObjectEventStateQueue and drop many NULL checks sprinkled about the code. The patches should roughly be:
- Edit virObjectEventStateQueue in src/conf/object_event.c to just return if !event. Remove NULL checking from all callers that use it directly
- 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
Move validation checks out of domain XML parsing
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 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
Split apart qemu_driver.c
INTERMEDIATE: 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
- 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
INTERMEDIATE: 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
- Move formatting routines to domain_conf_format.c
- Move enums to domain_conf_enum.c
- Move various utility functions to domain_conf_util.c
virsh: drop usage of #include "virsh-edit.c"
INTERMEDIATE: 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:
- tools/virsh-edit.c code
- files that match this command: git grep virsh-edit.c tools/*.c
Abstract away most usage of WITH_GNUTLS
INTERMEDIATE: 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.
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.
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.
INTERMEDIATE: 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
INTERMEDIATE: 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 *UnicSocketPaths functions
Clean up virQEMUDriverPtr parameters
EXTREMELY BEGINNER: 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).
Clean up virBufferCheckError() callers
BEGINNER: 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.
More conversions to virErrorPreserveLast/virErrorRestore
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
BEGINNER: 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
BEGINNER: 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.
- 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]
Remove redundant uses of *.la in Makefiles
BEGINNER: There is variable LDADDS in most of the Makefiles that is used for most of the targets in the Makefile. So specifying it separately just pollutes the file for no benefit. Check for such cases, fix them and think of a way automatically checking whether that happens.