Wednesday, Debtember 02, 2009
I told you handing errors was error prone
- From
- Mark Grosberg <XXXXXXXXXXXXXXXXX>
- To
- sean@conman.org
- Subject
- Blog post.
- Date
- Wed, 2 Dec 2009 15:59:55 -0500 (EST)
I find it even more amusing that you didn't get the error handling right in the
create_socket()on your current blog post.Notice that you leak the socket and/or memory in the error cases. I guess it really is hard to handle errors. ;-)
Sorry, I just had to take this cheap shot!
-MYG
Heh.
Yup, I blew it again for demonstration purposes.
The code I posted yesterday was
actually pulled from a current project where the
create_socket() is only called during initialization and if it
fails, the program exits. Since I'm on a Unix system, the “lost”
resources like memory and sockets are automatically reclaimed. Not all
operating systems are nice like this.
There are a few ways to fix this. One, is to use a langauge that handles such details automatically with garbage collection, but I'm using C so that's not an option. The second one is to add cleanup code at each point we exit, but using that we end up with code that looks like:
/* ... */
if fcntl(listen->sock,F_GETFL,0) == -1)
{
perror("fcntl(GETFL)");
close(listen->socket);
free(listen);
return -1;
}
if (fcntl(listen->sock,F_SETFL,rc | O_NONBLOCK) == -1)
{
perror("fcntl(SETFL)");
close(listen->socket);
free(listen);
return -1;
}
if (bind(listen->sock,paddr,saddr) == -1)
{
perror("bind()");
close(listen->socket);
free(listen);
return -1;
}
/* ... */
Lots of duplicated code and the more complex the routine, the more complex the cleanup and potential to leak memory (or other resources like files and network connections). The other option looks like:
/* ... */
if fcntl(listen->sock,F_GETFL,0) == -1)
{
perror("fcntl(GETFL)");
goto create_socket_cleanup;
}
if (fcntl(listen->sock,F_SETFL,rc | O_NONBLOCK) == -1)
{
perror("fcntl(SETFL)");
goto create_socket_cleanup;
}
if (bind(listen->sock,paddr,saddr) == -1)
{
perror("bind()");
goto create_socket_cleanup;
}
/* rest of code */
return listen->sock; /* everything is okay */
create_socket_cleanup:
close(listen->sock);
create_socket_cleanup_mem:
free(listen);
return -1;
}
This uses the dreaded goto construct, but is one of the few
places that it's deemed “okay” to use goto, for cleaning up
errors. No code duplication, but you need to make sure you cleanup (or
unwind, or whatever) in reverse order.
So yeah, error handling … maddening.
I still wish there was a better way …
