Archive for the ‘API’ Category

Goto be gone! Long live do {} while!

Part of my role at QNX is to be ornery about source code style, formatting, consistancy and the like. Obviously a very popular role to fill =;-)

One of the particularly religious debates that we occasionally get into revolves around the use of the goto statement. There are many people who use the goto statement considered harmful to argue that there is no rightfull place for a goto in our code.

This attachment to a particular dogma means that for complex structure initialization code developers often end up running a series of if clauses initializing each member of the structure in turn. If the code properly deals with errors and rollsback to undo the initialization, you end up with one of two scenarios:

  • If progress stops on initialization failure, then each if clause ends up having a growing list of clean up items on failure. A good compiler will optimize this, but it is too easy to forget an item and end up with a resource leak of one sort or another.
  • If progress continues, then you end up with a deeply nested if statement where the final return (on success) lies deep in the heart of the nesting. The cleanup for resources is generally straightforward, but in many cases the cleanup lies so far away (in code editor space) that it is cumbersome to read.
  • For this reason, I’ve always appreciated the goto statement as acceptable for these types of “go to a cleanup” scenarios where you have a large initialization dependancy (as might happen in a QNX resource manager) and want to group the cleanup so as to improve the readability and reduce the chance of an accidental resource leak. Unlike the use of goto resulting in spaghetti code flow control, this is a much more tempered use of goto statements in C code.

    However, the other day I was participating in a code review and came across an interesting use of the do {} while
    loop to achieve a similar early exit, but without all of the contempt of those who think that goto should be removed from the language all together:

      //Structure declaration and initial allocation here
      do {
        // ... Member initialization here
        if(failure) {
          break;
        }
        ...
       //Return successfully allocated and initialized member here
       } while(0);
      
       //Cleanup and failure reporting all goes here
    

    Here you get a nice linear, easy to read and maintain flow of code initialization without being burdened by the error condition flow control that a nested if requires or the deep unrolling of initialization that a chained if requires.

    In any case a nice escape away from goto’s; even though Dijkstra never said goto was considered harmfull =;-).

    Browsing trumps searching

    I was doing some maintenance work (bug fixing) on some code that was using the strncopy(src,dst,n)function to limit the number of bytes that were being copied.  Even though I’ve used this function hundreds of times, I never remember the exact details of what happens  with respect to the length and the terminating null character is it included as part of the length or isn’t it (it isn’t).   As a result I end up taking a trip into our QNX documentation and the C library reference to double check the behaviour.   Here is how the reading went this time for me:

     copy no more than n bytes over from dst to src. … yeah yeah …

    then whammo the next line from the docs jumped right out at me:

    If the string pointed to by src is shorter than n characters, null characters are appended to the copy in the array pointed to by dst, until n characters in all have been written.

    Wow! So strncpy() is always padding out the full size of the buffer with null characters. I don’t know why this hadn’t struck me before, but this time I was looking at some code that was working with a large maximum buffer (4K) but was most often dealing with strings that were only 10 or 20 characters long.  Since this was being done repeatedly, it seemed like a code change was going to be in order, something along the lines of (using the same src,dst and n):

    int len = strlen(src) + 1;  
    
    if(len > n) { 
       len = n - 1; 
       dst[len] = ''; 
    }   
    
    memcpy(dst, src, len);

    This was kind of a drag since it was more code to write, but it was going to be a bit more efficient since we weren’t going to iterate over all 4K of the buffer padding it in with bytes we didn’t need … although we were passing twice through the string. Wanting to double check the order of the src and dst arguments for memcpy I headed back to the docs. I use the HTML index instead of the search so I jumped to the ‘M’ functions. Looking through the list I came across a library function I had never heard of before … memccpy so I thought I’d take a quick read of it:

    Copy bytes between buffers until a given byte is found

    Cool! That is exactly what I was looking for. The call does an efficient one pass iteration copying only the data bytes until it hit the matching character or it runs out of space to copy to.  I still needed to deal with ensuring the destination buffer was going to be null terminated (as with strncpy() ) but at least now I wasn’t spending any time filling in extra bytes as padding:

    if(memcchr(dst, src, '', n) == NULL) { 
    dst[n-1] = ''; 
    }

    So, now I get the security of a bounded string buffer copy and efficiency to boot and I picked up a new standard C API for my toolbox … all because I like to browse the API instead of using the indexed search!  Of course, perhaps at some point this will get even more straightforward if the strlcpy() function becomes part of POSIX/ANSI C standard.

    A condvar is not a semaphore …

    I’ve been running into some situations lately where developers have been using condition variables (the pthread_cond_* family of functions) as semaphores (the sem_* family of functions) … but instead introducing bugs into their programs.The confusion is somewhat understandable. Given that condition variables have a wait() and a signal() function similar to the wait() and post()  function available for semaphores, the two might seem interchangeable.Here is an example of a condvar being used improperly as a semaphore replacement:

     struct _data { 
      pthread_mutex_t m; 
      pthread_condvar_t c; 
      ... other stuff ... 
     }; 
    void * writer_thread(void *arg) { 
      struct _data *d = (struct _data *)arg; 
      ... 
      pthread_mutex_lock(&d->m); 
      pthread_cond_signal(&d->c); 
      pthread_mutex_unlock(&d->m); 
      ... 
     } 
    void * reader_thread(void *arg) { 
      struct _data *d = (struct _data *)arg; 
      ... 
      pthread_mutex_lock(&d->m); 
      pthread_cond_wait(&d->c, &d->m); 
      pthread_mutex_unlock(&d->m); 
      ... 
     }

    The _intention_ here is that the writer_thread is signaling to the reader_thread that things are good and it can continue to operate. If this code were using sem_post()/sem_wait() the code would like operate as intended (assuming proper initialization values).However, imagine the following execution sequence (cause by context switches):

    writer_thread:pthread_mutex_lock() 
    reader_thread:pthread_mutex_lock() > blocks (mutex held) 
    writer_thread:pthread_cond_signal() 
    writer_thread:pthread_mutex_unlock() 
    reader_thread:pthread_cond_wait() > blocks waiting 
    ...

    In this case the writer thread had signaled the reader thread, but since the reader thread was not yet blocked, that signalling is lost. In the best case the writer_thread will have to wait a few extra cycles until the next reader_thread update or in the worst case the writer_thread may block forever if there are no further updates.

    Condition variables are like semaphores on steroids. You can trigger off of much more sophisticated combinations of data, but at the cost of managing those triggers and that data yourself.

    Assuming that we want our example to respond to every written request, and behave like a counting semaphore, then we should modify it as follows:

    struct _data { 
      pthread_mutex_t m; 
      pthread_condvar_t c; 
      int data; 
      ... other stuff ... 
    };  
    
    void * writer_thread(void *arg) { 
      struct _data *d = (struct _data *)arg; 
      ... 
      pthread_mutex_lock(&d->m); 
      d->data++; 
      pthread_cond_signal(&d->c); 
      pthread_mutex_unlock(&d->m); 
       ... 
    }  
    
    void * reader_thread(void *arg) { 
      struct _data *d = (struct _data *)arg; 
      ... 
      pthread_mutex_lock(&d->m); 
       while(d->data <= 0) { 
      pthread_cond_wait(&d->c, &d->m); 
      } 
      d->data--; 
      pthread_mutex_unlock(&d->m); 
      ... 
     }

    As you can see, the state data management is more manual, but is entirely within your control. In this case we are triggering on every data increment, but it can be easily adjusted to trigger on every fifth data increment instead.

    void * reader_thread(void *arg) { 
      ... 
      while(d->data <= 5) { 
      pthread_cond_wait(&d->c, &d->m); 
      } 
      d->data -= 5; 
      ...

    Code that uses a condvar in a lock/wait/unlock scenario without any intermediate condition testing being done around the wait should always be viewed with suspicion since chances are it was once a semaphore implementation that was switched to using condvars.

     If you wanted to give this code a bit of an additional performance boost, you could also avoid the (little) thundering herd problem that is introduced here by moving the condvar signalling operation of the writer thread to occur outside of the mutex locked area.  This will save the reader thread from waking up from the condition variable, only to go mutex blocked until the mutex is given up.

    Follow

    Get every new post delivered to your Inbox.