How not to code

This little gem came up in conversation last night, and it was suggested that it would make a rather amusing blog entry. A Solaris project had a command line utility with the following, unspeakably horrid, piece of code:

        /\*
         \* Use the dynamic linker to look up the function we should
         \* call.
         \*/
        (void) snprintf(func_name, sizeof (func_name), "do_%s", cmd);
        func_ptr = (int (\*)(int, char \*\*))
                dlsym(RTLD_DEFAULT, func_name);                                                                 

        if (func_ptr == NULL) {
                fprintf(stderr, "Unrecognized command %s", cmd);
                usage();
        }                                                                       

        return ((\*func_ptr)(argc, argv));

So when you type "a.out foo", the command would sprintf into a buffer to make "do_foo", and rely on the dynamic linker to find the appropriate function to call. Before I get a stream of comments decrying the idiocy of Solaris programmers: the code will never reach the Solaris codebase, and the responsible party no longer works at Sun. The participants at the dinner table were equally disgusted that this piece of code came out of our organization. Suffice to say that this is much better served by a table:

        for (i = 0; i < sizeof (func_table) / sizeof (func_table[0]); i++) {
                if (strcmp(func_table[i].name, cmd) == 0)
                          return (func_table[i].func(argc, argv));
        }

        fprintf(stderr, "Unrecognized command %s", cmd);
        usage();

I still can't imagine the original motivation for this code. It is more code, harder to understand, and likely slower (depending on the number of commands and how much you trust the dynamic linker's hash table). We continually preach software observability and transparency - but I never thought I'd see obfuscation of this magnitude within a 500 line command line utility. This prevents us from even searching for callers of do_foo() using cscope.

This serves as a good reminder that the most clever way of doing something is usually not the right answer. Unless you have a really good reason (such as performance), being overly clever will only make your code more difficult to maintain and more prone to error.

Update - Since some people seem a little confused, I thoght I'd elaborate two points. First off, there is no loadable library. This is a library linked directly to the application. There is no need to asynchronously update the commands. Second, the proposed function table does not have to live separately from the code. It would be quite simple to put the function table in the same file with the function definitions, which would improve maintainability and understability by an order of magnitude.

Comments:

I don't think this is all that clear-cut. There are certainly advantages to either approach. The major advantage of using the dynamic linker is that you don't have to maintain a separate table of functions and keep it in sync with the functions available. That's a major source of possible bugs eliminated.

Posted by guest on March 31, 2005 at 03:22 PM PST #

I would have to agree with the previous comment. This scheme could be used to provide an system of extensions to an application. Of course, if the functions are known at compile time, then the table is obviously better. If functions are only known at run time, the lookup seems reasonable.

Posted by Brian Utterback on March 31, 2005 at 09:46 PM PST #

I have heard the argument that this "makes it extensible". But if you are relying on <tt>dlsym()</tt> and <tt>sprintf()</tt> as your module API, your application is fundamentally broken. If you do need a pluggable interface (which, BTW, this does not), then you need to define a proper module API. At the very least, you need to provide versioning information so that you can extend it in future releases.

For example, in this particular case the usage message for each option was kept in the command, not in the library next to the code. This completely destroys the idea that you can update the library without modifying the command. A proper module interface would have a way of encoding this information such that it actually can be maintained outside the command.

I'm all for extensible, loadable interfaces. But <tt>dlsym()</tt> is fundamentally the wrong answer, and always will be. Especially when you have a simple command line utility that has no need for complicated loadable functions.

As to the argument that this prevents a "major source of possible bugs", I'd couldn't disagree more. If I'm in a library somewhere and I want to modify a function, how am I supposed to know that someone out there is actually using it? If I change the function signature, or remove it completely, it doesn't even generate a compile-time error.

You always have to ask yourself, what is this interface trying to solve?. If you're trying to create loadable, maintainable objects outside the command, then this is the wrong answer. The only thing this could possibly try to solve is laziness from having to edit two files.

Posted by Eric Schrock on April 01, 2005 at 12:54 AM PST #

Post a Comment:
Comments are closed for this entry.
About

Musings about Fishworks, Operating Systems, and the software that runs on them.

Search

Archives
« April 2014
SunMonTueWedThuFriSat
  
1
2
3
4
5
6
7
8
9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
   
       
Today