From Libvirt Wiki
Revision as of 19:42, 6 June 2016 by ColeRobinson (talk | contribs) (link to latest series for VIR_ERROR cleanup)
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.

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 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

Example patch:

Replace VIR_ERROR with standard vir*Error in state driver init

Various driver startup routines use VIR_ERROR logging macros for error reporting when they should be using the standard vir*Error functions instead, which provide a number of additional benefits. Some hints for finding places to convert:

  • Use this command to find all state driver files. Check for VIR_ERROR usage in the functions referenced there: git grep "static virStateDriver"
  • All VIR_ERROR usage in src/uml/uml_driver.c
  • All VIR_ERROR usage in src/xen/xen_driver.c

Incomplete patch series that removes all usages, needs a respin:

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 qemuBufferEscapeComma.

Example patches that adds comma escaping in one place:

Here's some examples, find their usages in src/qemu/qemu_command.c and add qemuBufferEscapeComma calls used in the above patches.

  • info->romfile in qemuBuildRomStr
  • 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
  • everything in qemuBuildSmbiosBiosStr
  • everything qemuBuildSmbiosSystemStr
  • everything qemuBuildSmbiosBaseBoardStr
  • 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

Code Cleanups

Drop configFile caching for storage pools

The virStoragePoolObjPtr from src/conf/storage_driver.c explicitly tracks configFile and autostartLink values. These are paths pointing to where the XML and autostart link are stored on disk. Tracking these per object is largely redundant because it's very easy to rebuild these paths on demand, since they are predictable named. This values should be dropped.

However obj->configFile is also used to track whether the object is marked as persistent or not, because only a persistent object stores a configFile on disk. So a new obj->persistent value should be introduced to track this particular property. This matches the pattern used by domain and network objects already, so search src/conf/network_conf.c and src/network/bridge_driver.c for configFile and ->persistent, and adapt that code pattern to src/conf/storage_conf.c and src/storage/storage_driver.c.

See also this commit which dropped obj->configFile for the nwfilter driver, though the code there doesn't need to care about autostart or persistence so it's a bit simpler:

More usage of qemuDeviceDriveHostAlias()

There's a helper function qemuDeviceDriveHostAlias() in src/qemu/qemu_command.c for assembling a qemu drive alias string from the libvirt disk's alias. However most of the rest of the code isn't actually using this convenience function and instead is opencoding the logic itself. Do a 'git grep QEMU_DRIVE_HOST_PREFIX' to find similar code that could be using qemuDeviceDriveHostAlias instead.

Bonus points for centralizing the other usages of QEMU_DRIVE_HOST_PREFIX for converting a qemu alias to a libvirt alias... the logic could probably be cosolidated into a new function

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:

Remove virDomainDefNewFull()

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.

Rename test suite routines from virtTest* to virTest*

Many of the helper routines in tests/* are named virtTest*, however that doesn't match the convention in the rest of libvirt code to use the vir prefix (not virt).

Rename all functions that start with virtXXX to virXXX and adjust the callers.

Remove unused QEMU feature flags

In late 2015, libvirt updated it's 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, lot's of code removal :)

More info:

List of feature bits that can be removed:

  • 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)
  • QEMU_CAPS_CHARDEV (big one, lots of stuff to remove. maybe needs more usage of the SupportsChardev helper)
  • QEMU_CAPS_DEVICE (also quite a big one. Some previous patches have been posted but it can be done piece by piece.

These flags are available in all qemu versions, but can be compiled out of the qemu binary. Not sure how to handle these:

  • QEMU_CAPS_RTC (limited to x86 emulators for qemu 0.12.1)
  • QEMU_CAPS_NO_HPET (limited to x86 emulators for qemu 0.12.1)
  • QEMU_CAPS_SMBIOS_TYPE (limited to x86 emulators for qemu 0.12.1)
  • QEMU_CAPS_NO_ACPI (limited to x86 emulators for qemu 0.12.1)

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

Add function that raises error if domain is not active

In libvirt.git, try this command: git grep -i "domain is not running" src/

Nearly every one of the over 100 hits there is just an occurence of the idiom:

   if (!virDomainObjIsActive(vm)) {
                      "%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

   if (virDomainObjCheckIsActive(vm) < 0)
       goto out;

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!

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.

Share cgroup code that is duplicated between QEMU and LXC

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

Share duplicated daemon code

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