Rusty's Bleeding Edge Page

So I added a few lines to talloc, to allow me to set a handler for out-of-memory. The Xen Store Daemon had a fair bit of code like so (do_rm):

	permnode = perm_node(node);
	if (!permnode) { /* ENOMEM */
		send_error(conn, errno);
		return;
	}

	/* Figure out where it is. */
	permpath = node_path(conn->transaction, permnode);
	if (!permpath) { /* ENOMEM */
		send_error(conn, errno);
		return;
	}

	path = node_path(conn->transaction, node);
	if (!path) {
		send_error(conn, errno);
		return;
	}

	if (!delete(path)) {
		send_error(conn, errno);
		return;
	}

	if (unlink(permpath) != 0)
		corrupt(conn, "Removing permfile %s after %s removed",
			permpath, path);

Since a reasonable action on allocation failure is to drop the connection to the client, I set the talloc_fail_handler to do a longjmp back to the core code, which talloc_free'd the connection, cleaning everything up. No more talloc failures!

The patch was 84 insertions, 255 deletions. Great, now I have code like so:

	permpath = node_path(conn->transaction, perm_node(node));
	if (!delete(node_path(conn->transaction, node))) {
		send_error(conn, errno);
		return;
	}
	if (unlink(permpath) != 0)
		corrupt(conn, "Removing permfile %s after %s removed",
			permpath, path);

Neat, but the obvious next step is:

	if (!delete(node_path(conn->transaction, node))) {
		send_error(conn, errno);
		return;
	}
	if (unlink(node_path(conn->transaction, perm_node(node))) != 0)
		corrupt(conn, "Removing permfile %s after %s removed",
			permpath, path);

Now this code has a subtle bug, which was obvious when writing code which had to handle node_path() failing to alloc and returning NULL: we can end up with the perm file not being unlinked, as the allocation in the unlink line fails. I'm pretty sure I wouldn't have noticed this if I had written using exceptions the first time.

By letting the programmer ignore error handling, exception mechanisms create subtle bugs: the worst kind. Its the same rationale against "return" statements within macros: it might save lines of code, but don't do it. I've left the change in there for the moment, but I'm a little frightened that I've traded off some typing for a new way to shoot myself in the foot...

[/tech] permanent link