Code Snippet 1: A Random File From the Linux Source

Questions for the audience:

• What is this code?
• How does it work?
• Could it be cleaner?
• What parts of the code are easy to understand, and what parts are hard?

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
34
35
36
37
38
39
40
41
42
43

/*
 * Copyright (C) 2008 Freescale Semiconductor, Inc.
 *
 * This program is free software; you can redistribute it and/or
 * modify it under the terms of the GNU General Public License
 * as published by the Free Software Foundation; either version
 * 2 of the License, or (at your option) any later version.
 */

#include <linux/device.h>	/* devres_*(), devm_ioremap_release() */
#include <linux/gfp.h>
#include <linux/io.h>		/* ioremap_prot() */
#include <linux/export.h>	/* EXPORT_SYMBOL() */

/**
 * devm_ioremap_prot - Managed ioremap_prot()
 * @dev: Generic device to remap IO address for
 * @offset: BUS offset to map
 * @size: Size of map
 * @flags: Page flags
 *
 * Managed ioremap_prot().  Map is automatically unmapped on driver
 * detach.
 */
void __iomem *devm_ioremap_prot(struct device *dev, resource_size_t offset,
				 size_t size, unsigned long flags)
{
	void __iomem **ptr, *addr;

	ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
	if (!ptr)
		return NULL;

	addr = ioremap_prot(offset, size, flags);
	if (addr) {
		*ptr = addr;
		devres_add(dev, ptr);
	} else
		devres_free(ptr);

	return addr;
}
EXPORT_SYMBOL(devm_ioremap_prot);

There are a number of interesting things that I observe while reading this source code.

First, I certainly do read the purpose statement. In fact, I come back to it again and again; I look at the purpose statement, then at the code, then back at the purpose statement, etc.; there’s a refinement of understanding that occurs as I try to fit the pieces of the code together into an whole that “means” what the purpose statement says.

Second, variable names are totally vital. If the name of the returned variable were “x”, this code would be much harder to read. By the same token, though, I don’t see a lot of difference between “addr” and “ptr”, and I wish the author had taken the time to use names like “resourcePtr” or “remappedAddr”.

Third, and really important: mutation makes code hard to read. There’s nothing sophisticated going on here, but in order to figure out what the function returns, you have to scan backward through the source to find all of the places where the “addr” variable is mutated. The fewer of these there are, the easier the code is to read. If addr is simply declared once with a value, that’s a big win. Even better would be a simple “return” in the conditional branches.

Fourth, thinking about what’s really going on in this function brings me to the conclusion that all of the complexity stems from the error handling. Without it, the body of the function looks like this:

1
2
3
4
5
6

...
ptr = devres_alloc(devm_ioremap_release, sizeof(*ptr), GFP_KERNEL);
addr = ioremap_prot(offset, size, flags);
*ptr = addr;
devres_add(dev, ptr);
return addr;

If I were writing this in racket-prime, it would look like this (including the error-handling):

(with-handlers
    ([cant-alloc -> NULL]
     [cant-remap -> (devres-free the-box) NULL])
  (define the-box (devres-alloc devm-ioremap-release, GFP_KERNEL))
  (define remapped (ioremap-prot offset size flags))
  (set-box! the-box remapped)
  (devres-add dev the-box)
  addr)