Re: [PATCH 3/3] Add minimal thread-safety to BFD

[Nick Clifton <nickc@redhat.com>]

Hi Tom,

> This patch provides some minimal thread-safety to BFD.
> 
> The BFD client can request thread-safety by providing a lock and
> unlock function.  The globals used during BFD creation (e.g.,
> bfd_id_counter) are then locked, and the file descriptor cache is also
> locked.  A function to clean up any thread-local data is now provided
> for BFD clients.
> 
> 	* bfd-in2.h: Regenerate.
> 	* bfd.c (lock_fn, unlock_fn): New globals.
> 	(bfd_thread_init, bfd_thread_cleanup, bfd_lock, bfd_unlock): New
> 	functions.
> 	* cache.c (bfd_cache_lookup_worker): Use _bfd_open_file_unlocked.
> 	(cache_btell, cache_bseek, cache_bread, cache_bwrite): Lock
> 	and unlock.
> 	(cache_bclose): Add comment.
> 	(cache_bflush, cache_bstat, cache_bmmap): Lock and unlock.
> 	(_bfd_cache_init_unlocked): New function.
> 	(bfd_cache_init): Use it.  Lock and unlock.
> 	(_bfd_cache_close_unlocked): New function.
> 	(bfd_cache_close, bfd_cache_close_all): Use it.  Lock and unlock.
> 	(_bfd_open_file_unlocked): New function.
> 	(bfd_open_file): Use it.  Lock and unlock.
> 	* doc/bfd.texi (BFD front end): Add Threading menu item.
> 	* libbfd.h: Regenerate.
> 	* opncls.c (_bfd_new_bfd): Lock and unlock.
> 	* po/bfd.pot: Regenerate.

I have one concern about this patch:


> +typedef void (*bfd_lock_unlock_fn_type) (void);

Given that we are dealing with client provided locking and unlocking
functions, I feel that the client might want to be able to reference
a data structure of their own performing these actions.  (I am not
hugely familiar with locking and unlocking functions, so maybe I am
mistaken here).  I also wonder if the functions might be interested
in the BFD being locked.  Thus I think that the typedef might be
better specified as:

  typedef void (* bfd_lock_unlock_fn_type) (bfd *, void *);

I also wonder if the functions should be allowed to fail, in which
case the typedef would be:

  typedef bool (* bfd_lock_unlock_fn_type) (bfd *, void *);

Given all of that the init function would have to be extended to take
a data pointer:

> +void
> +bfd_thread_init (bfd_lock_unlock_fn_type lock, 
                     bfd_lock_unlock_fn_type unlock,
                     void * data)
> +{
> +  lock_fn = lock;
> +  unlock_fn = unlock;
      lock_data = data;
> +}

Also - should this function let the caller know if a previous set
of lock/unlock functions had been registered, or if there was a problem
registering them ?  (For example is it OK to have a lock function but
not an unlock function ?)  ie:

   bool
   bfd_thread_init (bfd_lock_unlock_fn_type lock,
                    bfd_lock_unlock_fn_type unlock,
                    void * data)
   {
      bool ret = lock_fn == NULL && unlock_fn == NULL;

      lock_fn = lock;
      unlock_fn = unlock;
      lock_data = data;

      ret &= (lock_fn != NULL && unlock_fn != NULL) || (lock_fn == NULL && unlock_fn == NULL);
      return ret;
   }


And of course the bfd_lock and bfd_unlock would need to be updated as well, eg:

> +bool
> +bfd_lock (bfd * abfd)
> +{
> +  if (lock_fn != NULL)
> +    return lock_fn (abfd, lock_data);
      return true;
> +}

What do you think ?

Cheers
   Nick