Recently, I have been writing linux kernel modules at work. Like in the past, I struggled with finding a way to consistently match resource allocation to release. The standard way of doing this in the kernel as follows:
struct driver_data {
struct A *a;
struct B b;
opaque_t c;
};
int driver_init(struct parent_dev *parent) {
int rc = 0;
struct driver_data *data = kzalloc(sizeof struct driver_data,
GFP_KERNEL);
if (!data) {
rc = -ENOMEN;
goto out;
}
parent->private_data = data;
data->a = acquire_a();
if (!data->a) {
rc = -ENOSPC;
goto out_no_a;
}
if (!register_b(&data->b)) {
rc = -ENOSPC;
goto out_no_b;
}
data->c = make_c();
if (IS_ERR_C(data->c)) {
rc = -ENOMEM;
goto out_no_c;
}
return rc;
out_no_c:
unregister_b(&data->b);
out_no_b:
release_a(data->a);
out_no_a:
kfree(data);
out:
return rc;
}
I like the init function: If something does not work out mid-way, the acquired resources are released in the reverse order they were acquired. The code is relatively painless to write and easy to inspect.
And then you have to have a matching release function:
void driver_shutdown(struct parent_dev *parent) {
struct driver_data *data = parent->private_data;
unregister_b(&data->b);
destroy_c(data->c);
release_a(data-a);
kfree(data);
}
It's simple to see that the code in driver_shutdown looks like a straight line version of the code at the end of driver_init. Bonus points if you spotted that I mixed up the order of b and c in the driver_shutdown function.
I find it tedious to write this code twice and dangerous that the implementations can get out of sync.
As an alternative, the book "Linux Device Drivers"[1] recommends writing an extra cleanup function that releases all the acquired resources by checking "the status of each item before undoing its registration".
As the book points out, this may lead to you needing extra flags to keep track
of whether a resource has been initialized yet. Going through our example, it's
trivial for struct A *a. It is initialized to NULL by kzmalloc and
acquire_a() returns NULL in case of failure. It's trickier for
struct B b: We would have to look at its structure, register_b() and all
the code that handles struct Bs to tell whether we can find a member or a
combination of members that signal that struct B was correctly registered.
The situation is similar for opaque_t c. While there is an IS_ERR_C macro
to determine whether an opaque_t is invalid, I don't know whether the zero
value is valid. If so, I have to introduce an extra flag.
The cleanup function would look something like this, after having added
the relevant flags to struct driver_data.
void driver_cleanup(struct driver_data *data) {
// clearly, data may not be NULL
if (data->c_made) {
destroy_c(data->c);
}
if (data->b_registered) {
unregister_b();
}
if (data->a) {
release_a(data->a);
}
kfree(data);
}
I dislike this code for a number of reasons. First, for every acquired
resource, I have to find or introduce a way to detect whether it has been
acquired. The conditionals take up a lot of space in an otherwise pretty simple
function. Finally, and most importantly, if
data->c_made is true, the following conditions data->b_registered and
data->a should be true as well.
The error return paths reachable by the goto labels in driver_init did not
leave a possibility for this ambiguity. You jump to the approriate out_ label
and the rest of the code follows sequentially.
Ideally, I would like to only write the cleanup code once, in a simple fashion,
with a high degree of certainty of correctness and have both full release, in
driver_shutdown, and partial release, in driver_init do the right thing.
By including both the code of driver_init and driver_shutdown in one
function, I can achieve that goal. I introduced a new parameter init which
specifies whether the new function driver_init_or_shutdown performs the task
of driver_init or driver_shutdown.
struct driver_data {
struct A *a;
struct B b;
opaque_t c;
};
int driver_init_or_shutdown(struct parent_dev *parent, bool init) {
int rc = 0;
struct driver_data *data;
if (init) {
data = kzalloc(sizeof struct driver_data, GFP_KERNEL);
if (!data) {
rc = -ENOMEN;
goto out;
}
parent->private_data = data;
data->a = acquire_a();
if (!data->a) {
rc = -ENOSPC;
goto out_no_a;
}
if (!register_b(&data->b)) {
rc = -ENOSPC;
goto out_no_b;
}
data->c = make_c();
if (IS_ERR_C(data->c)) {
rc = -ENOMEM;
goto out_no_c;
}
return rc;
}
data = parent->private_data;
destroy_c(data->c);
out_no_c:
unregister_b(&data->b);
out_no_b:
release_a(data->a);
out_no_a:
kfree(data);
out:
return rc;
}
int driver_init(struct parent_dev *parent) {
return driver_init_or_shutdown(parent, true);
}
void driver_shutdown(struct parent_dev *parent) {
(void) driver_init_or_shutdown(parent, false);
}
I have never seen this structure before. I also haven't been able to find a mention of it on the web or in the Linux kernel. If you have seen it before, please drop me a line[2] and tell me where!
It feels somewhat unfamiliar in that
it is uncommon to have both driver_init and driver_shutdown in one function. The set of parameters of driver_init_or_shutdown needs to be the
union of the set of parameters of both _init and _shutdown and the return
types need to be made compatible somehow.
Here, _init and _shutdown share the exact same set of parameters. Further,
_shutdown has no return value, so it's a reasonable choice to pick the
return type of _init for _init_or_shutdown and just ignore the return
value in _shutdown.
To me, the benefits clearly outweigh the odd structure. By not duplicating code, I only have to get it right once. Not needing to check whether a resource
needs to be released means that I don't have to waste space for flags and think
of names for them, names with a rather large scope. By coding the resource
release paths as simple sequential statements, with goto labels as entry
points, I can be sure that the order resource release is always the same, a
benefit that is lost with the _cleanup function. Finally, the code is easy
to inspect for correctness: You read the _init_or_shutdown function from
both ends and make sure that for every resource allocation there is a
corresponding resource release.