Linas Vepstas <[email protected]> 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
