Re: [PATCH 1/3] git-svn: use POSIX::sigprocmask to block signals

From: Junio C Hamano <junio@pobox.com>
Date: Apr 23, 2012, 17:33 UTC
Message-ID: <xmqqipgqjqtk.fsf@junio.mtv.corp.google.com>
In-Reply-To: <d21d7433574e8ea7628320dbe1a5fc0dc9d94e64.1335198921.git.rkagan@mail.ru>

Roman Kagan <rkagan@mail.ru> writes:

> rev_map_set() tries to avoid being interrupted by signals.

The wording "tries to avoid" was unclear and I had to read the code twice. The code defers the signal processing but still wants to get the signal after it is done what it is doing, which is different from simply "ignoring", which is another way to "try to avoid".

Show 6 quoted lines

> The conventional way to achieve this is through sigprocmask(), which is
> available in the standard POSIX module.
>
> This is implemented by this patch.  One important consequence of it is
> that the signal handlers won't be unconditionally set to SIG_DFL anymore
> upon the first invocation of rev_map_set() as they used to.

That may be the first degree consequence (another is what happens when you received signals of different kinds while in the blocked section), but how would that difference affect the overall program execution?

> [That said, I'm not convinced that messing with signals is necessary
> (and sufficient) here at all, but my perl-foo is too weak for a more
> intrusive change.]

Everything you discussed above in the log message before "That said" part made sense. Instead of catching and setting a single $sig and replaying that later, potentially losing accumulated signals that are of different kinds, blocking before entering the part you do not want to get interrupted and unblocking after you are done is better done using sigprocmask.

If the problem to solve is to implement deferral and delayed signal processing correctly, I think your patch did the right thing, but your "necessary/sufficient" comment implies that the problem you were trying to address is _different_ from that. But it is not clear what it is.

Could you elaborate on it a bit more here, or if it will become clear in the later patch, then please drop that parenthesized part out of the log message.