Re: Locks and threads

[Linas Vepstas]

2009/3/4 Neil Jerram <address@hidden>:
> Here's another lock ordering fix.  (For 1.8.x at least, I haven't
> checked if it's relevant to master yet.)

I skimmed it quickly, looks reasonable, except for this:

       else
-       SCM_OUT_OF_RANGE (2, handler);
+       {
+         SCM_CRITICAL_SECTION_END;
+         SCM_OUT_OF_RANGE (2, handler);
+       }

The matching SCM_CRITICAL_SECTION_START;
is not in an if block, ... before your change, where
was the matching END?  Why is there now an END
when one did not used to be needed? Was this a
bug you fixed, unrelated to the rest of the patch?

In the past, without this END, there should have been
a deadlock, so I guess this demonstrates that the else
branch is never taken?  All this is rather confusing.

Now, if this was the linux kernel, people would ask you
to split this patch into two patches: one to fix this
unbalanced start/end problem, and another to pull the
alloc out of the critical section.  That makes it easier
to review the correctness of the changes ...

--linas