Re: Checking function calls

[Michael Elizabeth Chastain <mec@shout.net>]

Hi Fredrik,

> It is a GNU/Linux platform, and, yes, I am using gcc.

Well, that wraps up that line of inquiry.

> I know, I didn't plan ahead good enough when I started writing it, and
> now I'm stuck with either this, or a large rewrite.

When I run into this kind of problem, I like to step back -- way back --
get away from computers for a day or two and think about it.

I think there is no easy way out, that you actually are stuck with a
large rewrite.  There are just too many pthread_mutex_lock's flying
around.

For instance:

  client.c:findtransfer() does not have any locks.

  in client.c:freesharecache(), there is code:

    if (cache->parent != NULL)
    {
      pthread_mutex_lock(&cache->parent->mutex)l;
      ...
    }

  in general, it's unsafe to test a member and then acquire the lock,
  because someone else can delete cache->parent between the "if" statement
  and the acquisition of the lock.

  In client.c:clientmain():

    for(cur = transfers; cur != NULL; cur = next)
    {
      pthread_mutex_lock(&cur_mutex);
      next = cur->next;
      ...
    }

    between the execution of "cur = transfers" and "cur != NULL",
    the first item of the list can be deleted.

I recommend finding a textbook on multi-threaded programming that covers
"how to write thread-safe lists".  From your package, it looks like
you are in it to learn, so you could step way back from the code and
learn some theory at this point.

Another alternative is to use one big mutex for the whole list.
Then the primitive operations become:

  add item to list
    lock the whole list
    add the item
    unlock the whole list

  delete item from list
    lock the whole list
    delete the item
    unlock the whole list

  iterate over the list
    lock the whole list
    iterate over all the items
    unlock the whole list

The drawback is that walking the list locks the whole list against
addition and deletion.  If your list walker is just "print status
information" then that is fine.  If your list walker does some
long-lived network operation at each node then it is not fine.

Michael C