Re: Locks and threads

[Neil Jerram]

Linas Vepstas <address@hidden> writes:

> I skimmed it quickly, looks reasonable,

Thanks for reviewing!

> 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?

Yes, it's an unrelated bug.  All of the places that raise errors (and
so exit non-locally) should exit the critical section first.

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

Yes.  But that's expected; the else branch is only used if `sigaction'
is called with incorrect parameters.

> 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 ...

You're absolutely right.  I'll leave this part out, and generate a
separate patch for it.

Thanks again,
        Neil