Handling errors when refactoring procedural code

Question

I was handed some C code that basically consists of a big main() function. I am now trying to unfold the method into smaller functions, to make clearer the code's intent. I am having some trouble, though:

void main(int argc, char *argv[])
{
    if(argc != 3)
    {
        printf("Usage: table-server <port> <n_lists>\n");
        return;
    }
    int port = atoi(argv[1]), n_lists = atoi(argv[2]);
    if(port < 1024 || port > 49151 || n_lists < 1)
    {
        printf("Invalid args.\n");
        return;
    }
    signal(SIGPIPE, SIG_IGN);
    int sockfd = socket(AF_INET, SOCK_STREAM, IPPROTO_TCP);
    struct sockaddr_in s_addr;
    s_addr.sin_family = AF_INET;
    s_addr.sin_port = htons(port);
    s_addr.sin_addr.s_addr = htonl(INADDR_ANY);
    if(bind(sockfd, (struct sockaddr *)&s_addr, sizeof(s_addr)) < 0)
    {
        printf("(bind).\n");
        return;
    }
    if(listen(sockfd, SOMAXCONN) < 0)
    {
        printf("(listen).\n");
        return;
    }

I can identify 4 main concerns in this code's function:

1. Verifying the number of args is correct.
2. Getting from the command line arguments the port.
3. Calling signal(SIGPIPE, SIG_IGN).
4. Actually try to make a connection with the socket.

The problem when trying to refactor this into small functions is mainly related with error handling. For instance,r trying to extract the logic of 1. would look like this:

int verify_number_of_args(int argc) {
    if (argc != 3) {
        printf("...");
        return -1;
    }
    return 0;
}

and calling it would be something like this

if (verify_number_of_args(argc) == -1) return;

which isn't actually that bad. Now, for the socket, that'd be way more troublesome as both sockfd and s_addr need to be returned, plus the status return value:

int sockfd;
struct sockaddr_in* s_addr;
if (create_socket(port, &sockfd, s_addr) == -1)
    return;

which kind of defeats the purpose of trying to keep my main method as simple and clear as possible. I could, of course, resort to global variables in the .c file but that doesn't seem that good of an idea.

How do you generally handle this kind of things in C?

Answer 1

Here's the simple approach.

Argument parsing and related error checking are main's concern, so I wouldn't split those out unless main is extremely long.

The actual work, i.e. the networking part of the program, can be split off to a function that is very similar to main, except that it takes properly parsed and validated arguments:

int main(int argc, char *argv[])
{
    // handle arguments

    return serve(port, n_lists);
}

int serve(int port, int n_lists)
{
    // do actual work
}

As for error handling: if this code is not meant to be a library, you can get away with just killing the calling process when something goes wrong in a function, no matter how deep down in the call chain it is; that is in fact recommended practice (Kernighan & Pike, The Practice of Programming). Just make sure you factor out the actual error printing routines in something like

void error(char const *details)
{
    extern char const *progname;  // preferably, put this in a header

    fprintf(stderr, "%s: error (%s): %s\n", progname, details, strerror(errno));
    exit(1);
}

to get consistent error messages. (You might want to check out err(3) on Linux and BSD and maybe emulate that interface on other platforms.)

You can also try to factor out those operations that simply can't go wrong or are just calling a few system calls with some fool-proof setup, since those make for easily reusable components.

Answer 2

Leave as is? A bit of setup at the start of main doesn't constitute a problem, IMO. Start refactoring after things are set up.

Answer 3

Isn't that a sign that you are refactoring for the sake of refactoring ?

Anyway, regarding the "let's initialise sockfd and s_addr in one go", you can always create a structure, and pass a pointer to it :

struct app_ctx {
    int init_stage;
    int sock_fd;
    struct sockaddr_in myaddr;
    ...
}

Then you pass a pointer to an instance of this structure to all your "do one thing at a time" functions, and return error code.

At cleanup time, you do the same thing and pass the same structure.