My web servers host a number of domains for both personal sites and open source projects, which of course means managing a number of DNS zone files. I use Gandi as my registrar, and they throw in free DNS hosting when you purchase a domain. When you have more than 2-3 domains to manage and want to keep the DNS records consistent across all of them, dealing with the pointy-clicky web form interfaces is really incredibly tedious. Thus I have traditionally hosted my own DNS servers, creating the Bind DNS zone files in emacs. Anyone who has ever used Bind though, will know that its DNS zone file syntax is one of the most horrific formats you can imagine. It is really easy to make silly typos which will screw up your zone in all sorts of fun ways. Keeping the DNS records in sync across domains is also still somewhat tedious.
What I wanted is a simpler, safer configuration file format for defining DNS zones, which can minimise the duplication of data across different domains. There may be tools which do this already, but I fancied writing something myself tailored to my precise use case, so didn’t search for any existing solutions. The result of a couple of evenings hacking efforts is a tool I’m calling NoZone, which now has its first public release, version 1.0. The upstream source is available in a GIT repository
The /etc/nozone.cfg configuration file
The best way to illustrate what NoZone can do, is to simply show a sample configuration file. For reasons of space, I’m cutting out all the comments – the copy that is distributed contains copious comments. In this example, 3 (hypothetical) domain names are being configured, nozone.com, nozone.org which are the public facing domains, and an internal domain for testing purposes qa.nozone.org. All three domains are intended to be configured with the same DNS records, the only difference is that the internal zone (qa.nozone.org) needs to have different IP addresses for its records. For each domain, there will be three physical machines involved, gold, platinum and silver
The first step is to define a zone with all the common parameters specified. Note that this zone isn’t specifying any machine IP addresses, or domain names. It is just referring to the machine names to define an abstract base for the child zones
zones = {
common = {
hostmaster = dan-hostmaster
lifetimes = {
refresh = 1H
retry = 15M
expire = 1W
negative = 1H
ttl = 1H
}
default = platinum
mail = {
mx0 = {
priority = 10
machine = gold
}
mx1 = {
priority = 20
machine = silver
}
}
dns = {
ns0 = gold
ns1 = silver
}
names = {
www = platinum
}
aliases = {
db = gold
backup = silver
}
wildcard = platinum
}
With the common parameters defined, a second zone is defined called “production” which lists the domain names nozone.org and nozone.com and the IP details for the physical machines hosting the domains.
production = {
inherits = common
domains = (
nozone.org
nozone.com
)
machines = {
platinum = {
ipv4 = 12.32.56.1
ipv6 = 2001:1234:6789::1
}
gold = {
ipv4 = 12.32.56.2
ipv6 = 2001:1234:6789::2
}
silver = {
ipv4 = 12.32.56.3
ipv6 = 2001:1234:6789::3
}
}
}
The third zone is used to define the internal qa.nozone.org domain.
testing = {
inherits = common
domains = (
qa.nozone.org
)
machines = {
platinum = {
ipv4 = 192.168.1.1
ipv6 = fc00::1:1
}
gold = {
ipv4 = 192.168.1.2
ipv6 = fc00::1:2
}
silver = {
ipv4 = 192.168.1.3
ipv6 = fc00::1:3
}
}
}
}
Generating the Bind DNS zone files
With the /etc/nozone.org configuration file created, the Bind9 DNS zone files can now be generated by invoking the nozone command.
$ nozone
This generates a number of files
# ls /etc/named
nozone.com.conf nozone.conf nozone.org.conf qa.nozone.org.conf
$ ls /var/named/data/
named.run named.run-20130317 nozone.org.data
named.run-20130315 nozone.com.data qa.nozone.org.data
The final step is to add one line to /etc/named.conf and then restart bind.
$ echo 'include "/etc/named/nozone.conf";' >> /etc/named.conf
$ systemctl restart named.service
The generated files
The /etc/named/nozone.conf file is always generated and contains references to the conf files for each domain named
include "/etc/named/nozone.com.conf";
include "/etc/named/nozone.org.conf";
include "/etc/named/qa.nozone.org.conf";
Each of these files defines a domain name and links to the zone file definition. For example, nozone.com.conf contains
zone "nozone.com" in {
type master;
file "/var/named/data/nozone.com.data";
};
Finally, the interesting data is in the actual zone files, in this case /var/named/data/nozone.com.data
$ORIGIN nozone.com.
$TTL 1H ; queries are cached for this long
@ IN SOA ns1 hostmaster (
1363531990 ; Date 2013/03/17 14:53:10
1H ; slave queries for refresh this often
15M ; slave retries refresh this often after failure
1W ; slave expires after this long if not refreshed
1H ; errors are cached for this long
)
; Primary name records for unqualfied domain
@ IN A 12.32.56.1 ; Machine platinum
@ IN AAAA 2001:1234:6789::1 ; Machine platinum
; DNS server records
@ IN NS ns0
@ IN NS ns1
ns0 IN A 12.32.56.2 ; Machine gold
ns0 IN AAAA 2001:1234:6789::2 ; Machine gold
ns1 IN A 12.32.56.3 ; Machine silver
ns1 IN AAAA 2001:1234:6789::3 ; Machine silver
; E-Mail server records
@ IN MX 10 mx0
@ IN MX 20 mx1
mx0 IN A 12.32.56.2 ; Machine gold
mx0 IN AAAA 2001:1234:6789::2 ; Machine gold
mx1 IN A 12.32.56.3 ; Machine silver
mx1 IN AAAA 2001:1234:6789::3 ; Machine silver
; Primary names
gold IN A 12.32.56.2
gold IN AAAA 2001:1234:6789::2
platinum IN A 12.32.56.1
platinum IN AAAA 2001:1234:6789::1
silver IN A 12.32.56.3
silver IN AAAA 2001:1234:6789::3
; Extra names
www IN A 12.32.56.1 ; Machine platinum
www IN AAAA 2001:1234:6789::1 ; Machine platinum
; Aliased names
backup IN CNAME silver
db IN CNAME gold
; Wildcard
* IN A 12.32.56.1 ; Machine platinum
* IN AAAA 2001:1234:6789::1 ; Machine platinum
As of 2 days ago, I’m using nozone to manage the DNS zones for all the domains I own. If it is useful to anyone else, it can be downloaded from CPAN. I’ll likely be submitting it for a Fedora review at some point too.
The OpenStack Nova project has the ability to inject files into a guest filesystem immediately prior to booting the virtual machine instance. Historically the way it did this was to setup either a loop or NBD device on the host OS and then mount the guest filesystem directly on the host OS. One of the high priority tasks for Red Hat engineers when we became involved in OpenStack was to integrate libguestfs FUSE into Nova to replace the use of loop back + NBD devices, and then subsequently refactor Nova to introduce a VFS layer which enables use of the native libguestfs API to avoid any interaction with the host filesystem at all.
There has already been a vulnerability in the Nova code which allowed a malicious user to inject files to arbitrary locations in the host filesystem. This has of course been fixed, but even so mounting guest disk images on the host OS should still be considered very bad practice. The libguestfs manual describes the remaining risk quite well:
When you mount a filesystem, mistakes in the kernel filesystem (VFS) can be escalated into exploits by attackers creating a malicious filesystem. These exploits are very severe for two reasons. Firstly there are very many filesystem drivers in the kernel, and many of them are infrequently used and not much developer attention has been paid to the code. Linux userspace helps potential crackers by detecting the filesystem type and automatically choosing the right VFS driver, even if that filesystem type is unexpected. Secondly, a kernel-level exploit is like a local root exploit (worse in some ways), giving immediate and total access to the system right down to the hardware level
Libguestfs provides protection against this risk by creating a virtual machine inside which all guest filesystem manipulations are performed. Thus even if the guest kernel gets compromised by a VFS flaw, the attacker then still has to break out of the KVM virtual machine and its sVirt confinement to stand a chance of compromising the host OS. Some people have doubted the severity of this kernel VFS driver risk in the past, but an article posted on LWN today should serve reinforce the fact that libguestfs is right to be paranoid. The article highlights two kernel filesystem vulnerabilities (one in ext4 which is enabled in pretty much all Linux hosts) which left hosts vulnerable for as long as 3 years in some cases:
- CVE-2009-4307: a divide-by-zero crash in the ext4 filesystem code. Causing this oops requires convincing the user to mount a specially-crafted ext4 filesystem image
- CVE-2009-4020: a buffer overflow in the HFS+ filesystem exploitable, once again, by convincing a user to mount a specially-crafted filesystem image on the target system.
If the user has access to an OpenStack deployment which is not using libguestfs for file injection, then “convincing a user to mount a specially crafted filesystem” merely requires them to upload their evil filesystem image to glance and then request Nova to boot it.
Anyone deploying OpenStack with file injection enabled, is strongly advised to make sure libguestfs is installed to avoid any direct exposure of the host OS kernel to untrusted guest images.
While I picked on OpenStack as a convenient way to illustrate the problem here, it is not unique to OpenStack. Far too frequently I find documentation relating to virtualization that suggests people mount untrusted disk images directly on their OS. Based on their documented features I’m confident that a number of public virtual machine hosting companies will be mounting untrusted user disk images on their virtualization hosts, likely without using libguestfs for protection.
When a program has to kill a process, it may well want to perform some safety checks before invoking kill(), to ensure that the process really is the one that it is expecting. For example, it might check that the /proc/$PID/exe symlink matches the executable it originally used to start it. Or it might want to check the uid/gid the process is running as from /proc/$PID/status. Or any number of other attributes about a process that are exposed in /proc/$PID files.
This is really a similar pattern to the checks that a process might do when operating on a file. For example, it may stat() the file to check it has the required uid/gid ownership. It has long been known that doing stat() followed by open() is subject to a race condition which can have serious security implications in many circumstances. For this reason, well written apps will actually do open() and then fstat() on the file descriptor, so they’re guaranteed the checks are performed against the file they’re actually operating on.
If the kernel ever wraps around on PID numbers, it should be obvious that apps that do checks on /proc/$PID before killing a process are also subject to a race condition with potential security implications. This leads to the questions of whether it is possible to design a race free way of checking & killing a process, similar to the open() + fstat() pattern used for files. AFAICT there is not, but there are a couple of ways that one could be constructed without too much extra support from the kernel.
To attack this, we first need an identifier for the process which is unique and not reusable if a process dies & new one takes it place. As mentioned above, if the kernel allows PID numbers to wrap, the PID is unsuitable for this purpose. A file handle for the /proc/$PID directory though, it potentially suitable for this purpose. We still can’t simply open files like /proc/$PID/exe directly, since that’s relying on the potentially reusable PID number. Here the openat()/readlinkat() system calls come to rescuse. They allow you to pass a file descriptor for a directory and a relative filename. This guarantees that the file opened is in the directory expected, even if the original directory path changes / is replaced. All that is now missing is a way to kill a process, given a FD for /proc/$PID. There are two ideas for this, first a new system call fkill($fd, $signum), or a new proc file /proc/$PID/kill where you can write signal numbers.
With these pieces, a race free way to validate a process’ executable before killing it would look like
int pidfd = open("/proc/1234", O_RDONLY);
char buf[PATH_MAX];
char *exe = readlinkat(pidfd, "exe", buf, sizeof(buf));
if (strcmp(exe, "/usr/bin/mybinary") != 0)
return -1;
fkill(pidfd, SIGTERM);
Alternatively the last line could look like
char signum[10];
snprintf(signum, sizeof(signum), "%d", SIGTERM);
int sigfd = openat(pidfd, "kill", O_WRONLY);
write(sigfd, signum, strlen(signum))
close(sigfd)
Though I prefer the fkill() variant for reasons of brevity.
After reading this, you might wonder how something like systemd safely kills processes it manages without this kind of functionality. Well, it uses cgroups to confine processes associated with each service. Therefore it does not need to care about safety checking arbitrary PIDs, instead it can merely iterate over every PID in the service’s cgroup and kill them. The downside of cgroups is that it is Linux specific, but that doesn’t matter for systemd’s purposes. I believe that a fkill() syscall, however, is something that could be more easily used in a variety of existing apps without introducing too much portability code. Apps could simply use fkill() if available, but fallback to regular kill() elsewhere.
NB I’ve no intention of actually trying to implement this, since I like to stay out of kernel code. It is just an idea I wanted to air in case it interests anyone else.
The following document presented for discussion is based on my experience over the past few months getting involved with OpenStack Nova through learning the codebase, examining its history, writing code and participating in reviews. I want to stress, that I don’t intend this is a criticism of any individuals. My desire is that this be taken as constructive feedback to help the project as a whole over the long term, since I believe OpenStack can benefit from stricter practices commonly followed in other mainstream opensource projects using GIT.
Enjoy the rather long doc that follows….
This document is also present on the OpenStack developer list
GIT Commit Good Practice
The following document is based on experiance doing code development, bug troubleshooting and code review across a number of projects using GIT, including libvirt, QEMU and OpenStack Nova. Examination of other open source projects such as the Kernel, CoreUtils, GNULIB and more suggested they all follow a fairly common practice. It is motivated by a desire to improve the quality of the Nova GIT history. Quality is a hard term to define in computing; One man’s “Thing of Beauty” is another’s man’s “Evil Hack”. We can, however, come up with some general guidelines for what todo, or conversely what not todo, when publishing GIT commits for merge with a project, in this case, OpenStack.
This topic can be split into two areas of concern
- The structured set/split of the code changes
- The information provided in the commit message
Executive Summary
The points and examples that will be raised in this document ought clearly demonstrate the value in splitting up changes into a sequence of individual commits, and the importance in writing good commit messages to go along with them. If these guidelines were widely applied it would result in a significant improvement in the quality of the OpenStack GIT history. Both a carrot & stick will be required to effect changes. This document intends to be the carrot by alerting people to the benefits, while anyone doing Gerrit code review can act as the stick ;-P
In other words, when reviewing a change in Gerrit, do not simply look at the correctness of the code. Review the commit message itself and request improvements to its content. Look out for commits which are mixing multiple logical changes and require the submitter to split them into separate commits. Ensure whitespace changes are not mixed in with functional changes. Ensure no-op code refactoring is done separately from functional changes. And so on.
It might be mentioned that Gerrit’s handling of patch series is not entirely perfect. This is a not a valid reason to avoid creating patch series. The tools being used should be subservient to developers needs, and since they are open source they can be fixed / improved. Software source code is “read mostly, write occassionally” and thus the most important criteria is the improve the long term maintainability by the large pool of developers in the community, and not to sacrifice too much for the sake of the single author who may never touch the code again.
And now the long detailed guidelines & examples of good & bad practice
Structural split of changes
The cardinal rule for creating good commits is to ensure there is only one “logical change” per commit. There are many reasons why this is an important rule:
- The smaller the amount of code being changed, the quicker & easier it to review & identify potential flaws.
- If a change is found to be flawed later, it make be neccessary to revert the broken commit. This is much easier todo if there are not other unrelated code changes entangled with the original commit.
- When troubleshooting problems using GIT’s bisect capability, small well defined changes will aid in isolating exactly where the code problem was introduced.
- When browsing history using GIT annotate/blame, small well defined changes also aid in isolating exactly where & why a piece of code came from.
Things to avoid when creating commits
With that in mind, there are some commonly encountered examples of bad things to avoid
- Mixing whitespace changes with functional code changes. The whitespace changes will obscure the important functional changes, making it harder for a reviewer to correctly determine whether the change is correct.
Solution: Create 2 commits, one with the whitespace changes, one with the functional changes. Typically the whitespace change would be done first, but that need not be a hard rule.
- Mixing two unrelated functional changes. Again the reviewer will find it harder to identify flaws if two unrelated changes are mixed together. If it becomes necessary to later revert a broken commit the two unrelated changes will need to be untangled, with further risk of bug creation.
- Sending large new features in a single giant commit. It may well be the case that the code for a new feature is only useful when all of it is present. This does not, however, imply the the entire feature should be provided in a single commit. New features often entail refactoring existing code. It is highly desirable that any refactoring is done in commits which are separate from those implementing the new feature. This helps reviewers and test suites validate that the refactoring has no unintentional functional changes.Even the newly written code can often be split up into multiple pieces that can be independantly reviewed. For example, changes which adds new internal APIs/classes, can be in self-contained commits. Again this leads to easier code review. It also allows other developers to cherry-pick small parts of the work, if the entire new feature is not immediately ready for merge.Addition of new public APIs or RPC interfaces should be done in commits separate from the actual internal implementation. This will encourage the author & reviewers to think about the generic API/RPC design, and not simply pick a design that is easier for their currently chosen internal implementation.
The basic rule to follow is
If a code change can be split into a sequence of patches/commits, then it should be split. Less is not more. More is more.
Examples of bad practice
Now for some illustrations from Nova history. NB, although I’m quoting commit hashes for reference, I am removing all author names, since I don’t want to blame/pick on any one person. Almost everybody, including me, is guilty of violating these good practice rules at times. In addition the people who reviewed & approved these commits are just as guilty as the person who wrote/submitted them ;-P
Example 1:
commit ae878fc8b9761d099a4145617e4a48cbeb390623
Author: [removed]
Date: Fri Jun 1 01:44:02 2012 +0000
Refactor libvirt create calls
* minimizes duplicated code for create
* makes wait_for_destroy happen on shutdown instead of undefine
* allows for destruction of an instance while leaving the domain
* uses reset for hard reboot instead of create/destroy
* makes resume_host_state use new methods instead of hard_reboot
* makes rescue/unrescue not use hard reboot to recreate domain
Change-Id: I2072f93ad6c889d534b04009671147af653048e7
There are at least two independent changes made in this commit.
- The switch to use the new “reset” API for the “hard_reboot” method
- The adjustment to internal driver methods to not use “hard_reboot”
What is the problem with this ? First there is no compelling reason why these changes needed to be made at the same time. A first commit could have included the changes to stop calling “hard_reboot” in various places. A second commit could have re-written the “hard_reboot” impl.
Because the switch to using the libvirt ‘reset’ method was burried in the large code refactoring, reviewers missed the fact that this was introducing a dependency on a newer libvirt API version. This commit was identified as the culprit reasonably quickly, but a trivial revert is not possible, due to the wide variety of unrelated changes included.
Example 2:
commit e0540dfed1c1276106105aea8d5765356961ef3d
Author: [removed]
Date: Wed May 16 15:17:53 2012 +0400
blueprint lvm-disk-images
Add ability to use LVM volumes for VM disks.
Implements LVM disks support for libvirt driver.
VM disks will be stored on LVM volumes in volume group
specified by `libvirt_images_volume_group` option.
Another option `libvirt_local_images_type` specify which storage
type will be used. Supported values are `raw`, `lvm`, `qcow2`,
`default`. If `libvirt_local_images_type` = `default`, usual
logic with `use_cow_images` flag is used.
Boolean option `libvirt_sparse_logical_volumes` controls which type
of logical volumes will be created (sparsed with virtualsize or
usual logical volumes with full space allocation). Default value
for this option is `False`.
Commit introduce three classes: `Raw`, `Qcow2` and `Lvm`. They contain
image creation logic, that was stored in
`LibvirtConnection._cache_image` and `libvirt_info` methods,
that produce right `LibvirtGuestConfigDisk` configurations for
libvirt. `Backend` class choose which image type to use.
Change-Id: I0d01cb7d2fd67de2565b8d45d34f7846ad4112c2
This is introducing one major new feature, so on the surface it seems reasonable to use a single commit, but looking at the patch, it clearly has entangled a significant amount of code refactoring with the new LVM feature code. This makes it hard to identify likely regressions in support for QCow2/Raw images. This should have been split into at least four separate commits
- Replace the ‘use_cow_images’ config FLAG with the new FLAG ‘libvirt_local_images_type’, with back-compat code for support of legacy ‘use_cow_images’ FLAG
- Creation of internal “Image” class and subclasses for Raw & QCow2 image type impls.
- Refactor libvirt driver to replace raw/qcow2 image management code, with calls to the new “Image” class APIs
- Introduce the new “LVM” Image class implementation
Examples of good practice
Example 1:
commit 3114a97ba188895daff4a3d337b2c73855d4632d
Author: [removed]
Date: Mon Jun 11 17:16:10 2012 +0100
Update default policies for KVM guest PIT & RTC timers
commit 573ada525b8a7384398a8d7d5f094f343555df56
Author: [removed]
Date: Tue May 1 17:09:32 2012 +0100
Add support for configuring libvirt VM clock and timers
Together these two changes provide support for configuring the KVM guest timers. The introduction of the new APIs for creating libvirt XML configuration have been clearly separated from the change to the KVM guest creation policy, which uses the new APIs.
Example 2:
commit 62bea64940cf629829e2945255cc34903f310115
Author: [removed]
Date: Fri Jun 1 14:49:42 2012 -0400
Add a comment to rpc.queue_get_for().
Change-Id: Ifa7d648e9b33ad2416236dc6966527c257baaf88
commit cf2b87347cd801112f89552a78efabb92a63bac6
Author: [removed]
Date: Wed May 30 14:57:03 2012 -0400
Add shared_storage_test methods to compute rpcapi.
...snip...
Add get_instance_disk_info to the compute rpcapi.
...snip...
Add remove_volume_connection to the compute rpcapi.
...snip...
Add compare_cpu to the compute rpcapi.
...snip...
Add get_console_topic() to the compute rpcapi.
...snip...
Add refresh_provider_fw_rules() to compute rpcapi.
...many more commits...
This sequence of commits refactored the entire RPC API layer inside nova to allow pluggable messaging implementations. With such a major change in a core piece of functionality, splitting up the work into a large sequence of commits was key to be able to do meaningful code review, and track / identify possible regressions at each step of the process.
Information in commit messages
As important as the content of the change, is the content of the commit message describing it. When writing a commit message there are some important things to remember
- Do not assume the reviewer understands what the original problem was. When reading bug reports, after a number of back & forth comments, it is often as clear as mud, what the root cause problem is. The commit message should have a clear statement as to what the original problem is. The bug is merely interesting historical background on /how/ the problem was identified. It should be possible to review a proposed patch for correctness without needing to read the bug ticket.
- Do not assume the reviewer has access to external web services/sites In 6 months time when someone is on a train/plane/coach/beach/pub troubleshooting a problem & browsing GIT history, there is no guarantee they will have access to the online bug tracker, or online blueprint documents. The great step forward with distributed SCM is that you no longer need to be “online” to have access to all information about the code repository. The commit message should be totally self-contained, to maintain that benefit.
- Do not assume the code is self-evident/self-documenting. What is self-evident to one person, might be clear as mud to another person. Always document what the original problem was and how it is being fixed, for any change except the most obvious typos, or whitespace only commits.
- Describe /why/ a change is being made. A common mistake is to just document how the code has been written, without describing /why/ the developer chose todo it that way. By all means describe the overall code structure, particularly for large changes, but more importantly describe the intent/motivation behind the changes.
- Read the commit message to see if it hints at improved code structure. Often when describing a large commit message, it becomes obvious that a commit should have in fact been split into 2 or more parts. Don’t be afraid to go back and rebase the change to split it up into separate commits.
- Ensure sufficient information to decide whether to review. When gerrit sends out email alerts for new patch submissions there is minimal information included, principally the commit message and the list of files changes. Given the high volume of patches, it is not reasonable to expect all reviewers to examine the patches in detail. The commit message must thus contain sufficient information to alert the potential reviewers to the fact that this is a patch they need to look at.
- The first commit line is the most important. In GIT commits the first line of the commit message has special significance. It is used as email subject line, git annotate messages, gitk viewer annotations, merge commit messages and many more places where space is at a premium. As well as summarising the change itself, it should take care to detail what part of the code is affected. eg if it affects the libvirt driver, mention ‘libvirt’ somewhere in the first line.
- Describe any limitations of the current code. If the code being changed still has future scope for improvements, or any known limitations then mention these in the commit message. This demonstrates to the reviewer that the broader picture has been considered and what tradeoffs have been done in terms of short term goals vs long term wishes.
The basic rule to follow is
The commit message must contain all the information required to fully understand & review the patch for correctness. Less is not more. More is more.
Including external references
The commit message is primarily targetted towards human interpretation, but there is always some metadata provided for machine use. In the case of OpenStack this includes at least the ‘Change-id’, but also optional “bug” ID references and “blueprint” name references. Although GIT records the author & committer of a patch, it is common practice across many open source projects to include a “Signed-off-by” tag. Though OpenStack does not mandate its use, the latter is still useful to include if a patch is a combination of work by many different developers, since GIT only records a single author. All machine targetted metadata, however, is of secondary consequence to humans and thus it should all be grouped together at the end of the commit message. For example:
Switch libvirt get_cpu_info method over to use config APIs
The get_cpu_info method in the libvirt driver currently uses
XPath queries to extract information from the capabilities
XML document. Switch this over to use the new config class
LibvirtConfigCaps. Also provide a test case to validate
the data being returned
Fixes: bug #1003373
Implements: blueprint libvirt-xml-cpu-model
Change-Id: I4946a16d27f712ae2adf8441ce78e6c0bb0bb657
Signed-off-by: Daniel P. Berrange <berrange@xxxxxxxxxx>
As well as the ‘Signed-off-by’ tag, there are various other ad-hoc tags that can be used to credit other people involved in a patch who aren’t the author.
- ‘Reviewed-by: …some name.. <…email…>’
Although Gerrit tracks formal review by project members, some patches have been reviewed by people outside the community prior to submission
- ‘Suggested-by: …some name.. <…email…>’
If a person other than the patch author suggested the code enhancement / influnced the design
- ‘Reported-by: …some name.. <…email…>’
If a person reported the bug / problem being fixed but did not otherwise file a launchpad bug report.
…invent other tags as relevant to credit other contributions
Some examples of bad practice
Now for some illustrations from Nova history, again with authors names removed since no one person is to blame for these.
Example 1:
commit 468e64d019f51d364afb30b0eed2ad09483e0b98
Author: [removed]
Date: Mon Jun 18 16:07:37 2012 -0400
Fix missing import in compute/utils.py
Fixes bug 1014829
Problem: this does not mention what imports where missing and why they were needed. This info was actually in the bug tracker, and should have been copied into the commit message, so it provides a self-contained description. eg:
Add missing import of 'exception' in compute/utils.py
nova/compute/utils.py makes a reference to exception.NotFound, however exception has not been imported.
Example 2:
commit 2020fba6731634319a0d541168fbf45138825357
Author: [removed]
Date: Fri Jun 15 11:12:45 2012 -0600
Present correct ec2id format for volumes and snaps
Fixes bug 1013765
* Add template argument to ec2utils.id_to_ec2_id() calls
Change-Id: I5e574f8e60d091ef8862ad814e2c8ab993daa366
Problem: this does not mention what the current (broken) format is, nor what the new fixed format is. Again this info was available in the bug tracker and should have been included in the commit message. Furthermore, this bug was fixing a regression caused by an earlier change, but there is no mention of what the earlier change was. eg
Present correct ec2id format for volumes and snaps
During the volume uuid migration, done by changeset XXXXXXX,
ec2 id formats for volumes and snapshots was dropped and is
now using the default instance format (i-xxxxx). These need
to be changed back to vol-xxx and snap-xxxx.
Adds a template argument to ec2utils.id_to_ec2_id() calls
Fixes bug 1013765
Example 3:
commit f28731c1941e57b776b519783b0337e52e1484ab
Author: [removed]
Date: Wed Jun 13 10:11:04 2012 -0400
Add libvirt min version check.
Fixes LP Bug #1012689.
Change-Id: I91c0b7c41804b2b25026cbe672b9210c305dc29b
Problem: This commit message is merely documenting what was done, and not why it was done. It should have mentioned what earlier changeset introduced the new min libvirt version. It should also have mentioned what behaviour is when the check fails. eg
Add libvirt version check, min 0.9.7
The commit XXXXXXXX introduced use of the 'reset' API
which is only available in libvirt 0.9.7 or newer. Add a check
performed at startup of the compute server against the libvirt
connection version. If the version check fails the compute
service will shutdown.
Fixes LP Bug #1012689.
Change-Id: I91c0b7c41804b2b25026cbe672b9210c305dc29b
Examples of good practice
Example 1:
commit 3114a97ba188895daff4a3d337b2c73855d4632d
Author: [removed]
Date: Mon Jun 11 17:16:10 2012 +0100
Update default policies for KVM guest PIT & RTC timers
The default policies for the KVM guest PIT and RTC timers
are not very good at maintaining reliable time in guest
operating systems. In particular Windows 7 guests will
often crash with the default KVM timer policies, and old
Linux guests will have very bad time drift
Set the PIT such that missed ticks are injected at the
normal rate, ie they are delayed
Set the RTC such that missed ticks are injected at a
higher rate to "catch up"
This corresponds to the following libvirt XML
<clock offset='utc'>
<timer name='pit' tickpolicy='delay'/>
<timer name='rtc' tickpolicy='catchup'/>
</clock>
And the following KVM options
-no-kvm-pit-reinjection
-rtc base=utc,driftfix=slew
This should provide a default configuration that works
acceptably for most OS types. In the future this will
likely need to be made configurable per-guest OS type.
Fixes LP bug #1011848
Change-Id: Iafb0e2192b5f3c05b6395ffdfa14f86a98ce3d1f
Some things to note about this example commit message
- It describes what the original problem is (bad KVM defaults)
- It describes the functional change being made (the new PIT/RTC policies)
- It describes what the result of the change is (new the XML/QEMU args)
- It describes scope for future improvement (the possible per-OS type config)
Example 2:
commit 31336b35b4604f70150d0073d77dbf63b9bf7598
Author: [removed]
Date: Wed Jun 6 22:45:25 2012 -0400
Add CPU arch filter scheduler support
In a mixed environment of running different CPU architecutres,
one would not want to run an ARM instance on a X86_64 host and
vice versa.
This scheduler filter option will prevent instances running
on a host that it is not intended for.
The libvirt driver queries the guest capabilities of the
host and stores the guest arches in the permitted_instances_types
list in the cpu_info dict of the host.
The Xen equivalent will be done later in another commit.
The arch filter will compare the instance arch against
the permitted_instances_types of a host
and filter out invalid hosts.
Also adds ARM as a valid arch to the filter.
The ArchFilter is not turned on by default.
Change-Id: I17bd103f00c25d6006a421252c9c8dcfd2d2c49b
Some things to note about this example commit message
- It describes what the problem scenario is (mixed arch deployments)
- It describes the intent of the fix (make the schedular filter on arch)
- It describes the rough architecture of the fix (how libvirt returns arch)
- It notes the limitations of the fix (work needed on Xen)
I have written before about what a great benefit GObject Introspection is, by removing the need to write dynamic language bindings for C libraries. When I ported GTK-VNC to optionally build with GTK3, I did not bother to update the previous manually created Python binding. Instead application developers are now instructed to use GObject Introspection if they ever want to use GTK-VNC from non-C languages. As a nice demo of the capabilities I have written the bare minimum “Hello World” like example for GTK-VNC in Perl, Python and JavaScript. The only significant difference between these examples is syntax for actually importing a particular library. The Perl binding is the most verbose for importing libraries, which is a surprise, since Perl is normally a very concise language. Hopefully they will invent a more concise syntax for importing soon.
Perl “hello world” VNC client
#!/usr/bin/perl
use Gtk3 -init;
Glib::Object::Introspection->setup(basename => 'GtkVnc', version => '2.0', package => 'GtkVnc');
Glib::Object::Introspection->setup(basename => 'GVnc', version => '1.0', package => 'GVnc');
GVnc::util_set_debug(1);
my $win = Gtk3::Window->new ('toplevel');
my $dpy = GtkVnc::Display->new();
$win->set_title("GTK-VNC with Perl");
$win->add($dpy);
$dpy->open_host("localhost", "5900");
$win->show_all;
Gtk3::main;
Python “hello world” VNC client
#!/usr/bin/python
from gi.repository import Gtk;
from gi.repository import GVnc;
from gi.repository import GtkVnc;
GVnc.util_set_debug(True)
win = Gtk.Window()
dpy = GtkVnc.Display()
win.set_title("GTK-VNC with Python")
win.add(dpy)
dpy.open_host("localhost", "5900")
win.show_all()
Gtk.main()
JavaScript “hello world” VNC client
#!/usr/bin/gjs
const Vnc = imports.gi.GtkVnc;
const GVnc = imports.gi.GVnc;
const Gtk = imports.gi.Gtk;
Gtk.init(0, null);
GVnc.util_set_debug(true);
var win = new Gtk.Window();
var dpy = new Vnc.Display();
win.set_title("GTK-VNC with JavaScript");
win.add(dpy);
dpy.open_host("localhost", "5900");
win.show_all();
Gtk.main();