Re: plea for review

[Alexander Larsson <alla lysator liu se>]

On Wed, 24 Jan 2001, Tim Janik wrote:

> On Wed, 24 Jan 2001, Alexander Larsson wrote:
> 
> > Before starting to review the actual locking i'd like to state that the
> > following locking primitive is completely bogus:
> > 
> > static gint type_rw_lock = 0;
> > #define	G_READ_LOCK(rw_lock)	{ if (*(rw_lock)!=0) g_error (G_STRLOC": read lock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=1; }
> > #define	G_READ_UNLOCK(rw_lock)	{ if (*(rw_lock)!=1) g_error (G_STRLOC": read unlock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=0; }
> > #define	G_WRITE_LOCK(rw_lock)	{ if (*(rw_lock)!=0) g_error (G_STRLOC": write lock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=2; }
> > #define	G_WRITE_UNLOCK(rw_lock)	{ if (*(rw_lock)!=2) g_error (G_STRLOC": write unlock invalid (lock=%u)", *(rw_lock)); *(rw_lock)=0; }
> > 
> > There are so many problems with it I won't even get started on it. I
> > assume you're gonna replace this later with some real thing?
> 
> heh ;)
> 
> these are debugging primitives that should point out obvious
> things like G_READ_LOCK(); G_READ_LOCK(); use
> 
> static GStaticRWLock type_rw_lock = G_STATIC_RW_LOCK_INIT;
> #define	G_READ_LOCK(rw_lock)	g_static_rw_lock_reader_lock (rw_lock)
> #define	G_READ_UNLOCK(rw_lock)	g_static_rw_lock_reader_unlock (rw_lock)
> #define	G_WRITE_LOCK(rw_lock)	g_static_rw_lock_writer_lock (rw_lock)
> #define	G_WRITE_UNLOCK(rw_lock)	g_static_rw_lock_writer_unlock (rw_lock)
> 
> if you prefer the real thing ;)
> note however that pure testing doesn't do too much good currently,
> as a couple of other places in libgobject.so are still not MT safe,
> so the main purpose in me sending gtype.c out was in fact just
> theoretical proof reading ;)

I suspected so.

I've given it a read now, and i couldn't find much problems. I'm extremely
uncomfortable with dropping the lock and calling external code, but I
suppose if finalizers and initializer aren't completely insane it will
work.

check_derivation_U:
  pnode = lookup_type_node_L (parent_type);
  finfo = type_node_fundamental_info_L (pnode);
  if (!pnode)
    ...
might call type_node_fundamental_info_L (NULL), which looks bad.
    
Calls like g_type_fundamental_branch_last() are fundamentally
non-threadsafe. The value returned may have changed before the caller even
gets it.

Otherwise I found nothing, although I can't claim I understand all the
code.

/ Alex