I hate to break it to all the C/Linux hackers out there, but Dijkstra is right. Even though Linus says that goto is ok in error handling (as a poor mans exception handler basically) he is wrong. Now I will show why with concrete examples, and show a better way of handling errors in C.
There are many arguments for the use of goto, such as:
- The generated code is the same as if you used if/then/else.
- Linus says it is ok.
- They make the code more readable.
- They eliminate the need for many nested if/then/else blocks.
Goto will generate the same code as if you used if/then/else
Good, there is no performance benefit to using goto, perfect.
Linus says it is ok
I hate the slag the guy, because I think linux is great and I use it every day, the same goes for git. However he is not the ultimate authority on software engineering. There are many examples where he engineered software incorrectly, and others had to come in and try to fix it, or in some cases we just live with his mistakes.
Git is an example, the user interface and the business logic are inseparable. Why didn’t he put the business logic in a library? This is very basic (modern day) software engineering, I know in the good old days of unix, you would build a small command line tool, and then build bigger tools around it with pipes. However times have changed, it is a new millennium, and today we use libraries with (hopefully) good APIs. Does anyone think it is sad that he has to use libgit2 in subsurface because he didn’t design git properly? If I had to deliver that type of news to my manager my desk would get moved to the basement.
Sorry Mr. Manager, we have all the code we need to do the job in house, but we can’t use it because we didn’t engineer it correctly, luckily some other people have decided to do it right so we can just use their code…
Another example is linux itself, I work in the linux kernel quite often, and I go completely insane almost every time I have to grep the kernel, or google something in the kernel. So many functions and variables are single English words that make finding anything a royal PITA. It was his decision, and I think it was a bad one that reduces the ease of maintenance of almost anything in the kernel. I love working with a driver where they had the simple foresight to just prefix everything with some unique identifier. It is so much easier to find mx123_whatever(), than whatever(), it is a small thing, but it bad software engineering.
I could go on, but I won’t, my point is that Linus is great at hacking at code, and managing projects (with the exception of outrageous public shaming of contributors), but as far as actual software engineering goes, I have serious doubts. So if he says goto is ok, I take it with a big grain of salt.
Goto makes the code more readable
I can’t argue that they don’t make code more readable, however code that is merely readable is not good enough. Software engineering is:
The application of a systematic, disciplined, quantifiable approach to the development, operation, and maintenance of software
Notice it doesn’t say anything about reading code, it does however talk about maintaining code. It is of course always the case that maintainable code is readable, however readable code is not always maintainable:
Readable/Maintainable code Venn diagram
If you are writing code that is just readable, then you are missing the target, and the use of goto as an error handling tool is not maintainable because it requires changes in two places to make one update, with no compiler help if you get it wrong. Here is what I mean:
void initstuff()
{
dev = kzalloc(size);
if (!dev)
{
goto nodev;
}
dev->rcow = get_resource_cow();
if (!dev->rcow)
{
goto nocow;
}
dev->rbird = get_resource_bird();
if (!dev->rbird)
{
goto nobird;
}
dev->rdog = get_resource_dog();
if (!dev->rdog)
{
goto nodog;
}
dev->rhog = get_resource_hog();
if (!dev->rhog)
{
goto nohog;
}
// init everything else...
return;
nohog:
release_dog(dev->rdog);
nodog:
release_cow(dev->rcow);
nocow:
release_bird(dev->rbird);
nobird:
kfree(dev);
nodev:
}
void remove()
{
release_hog(dev->rhog);
release_dog(dev->rdog);
release_cow(dev->rcow);
release_bird(dev->rbird);
kfree(dev);
}
As you can see, there is duplicate code, and if a change is made to the top of the initstuff() function, then a corresponding change must also be made to the bottom of initstuff(). People mess that up by either just forgetting to make the corresponding change, or by making it wrong, as shown above. The label order bug above will not get caught by the compiler, will likely get past a code review, and will probably not get caught by any testing done, unless there are specific tests for all error paths with checks for this specific type of resource leak which is unlikely.
The next thing that makes this code un-maintainable is the fact that at label nohog, you have to release the rdog. This is counter intuitive, and is fraught with peril.
Goto eliminates the need for many nested if/then/else blocks
In most code I am opposed to early returns, especially if there are many other flow control constructs in the code (loops, nested if/then/else etc), however during init time it is frequently the case that the code is straight line with little flow control. So I recommend an early return with an error code, combined with a common release method that is used everywhere that some or all resources need to be released.
int initstuff()
{
dev = kzalloc(size);
if (!dev)
{
return -ENOMEM;
}
dev->rcow = get_resource_cow();
if (!dev->rcow)
{
return -ENOMEM;
}
dev->rbird = get_resource_bird();
if (!dev->rbird)
{
return -ENOMEM;
}
dev->rdog = get_resource_dog();
if (!dev->rdog)
{
return -ENOMEM;
}
dev->rhog = get_resource_hog();
if (!dev->rhog)
{
return -ENOMEM;
}
// init everything else...
return 0;
}
void remove()
{
if (dev)
{
if (dev->rhog)
{
release_hog(dev->rhog);
}
if (dev->rdog)
{
release_dog(dev->rdog);
}
if (dev->rcow)
{
release_cow(dev->rcow);
}
if (dev->rbird)
{
release_bird(dev->rbird);
}
kfree(dev);
}
}
This eliminates the duplicate code, keeps the code readable, and more importantly it makes the code maintainable. As you can see this code eliminates the label order bug in the first code snippet, and with this pattern it doesn’t matter what order the resources are allocated in, if they are allocated then they will be freed. Another alternative to reduce indent levels is to write modular code, rather than having one big init function; possibly with crazy levels of indent, have a bunch of small init functions that each init one small group of related things.
Summary
Goto Dijkstra Goto Considered Harmful
goto bugs in the wild
- omap34xxcam.c from the linux-omap kernel, goto out_v4l2_fh_add, you can’t call v4l2_fh_del() if the v4l2_fh_add() failed.
- synaptics_dsx_core.c 3rd party linux driver for touchscreens, the lookup of pinctrl_state_release in synaptics_dsx_pinctrl_init(), just because it is at the end of the function, doesn’t mean you can skip the goto on failure. You still need to jump over the early return.
Leave a Reply