Re: [PATCH 1/3] git-svn: use POSIX::sigprocmask to block signals
23 апреля 2012 г. 21:33 пользователь Junio C Hamano <junio@pobox.com> написал:
Show 8 quoted lines
> 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".
Sorry. I'll try to avoid misunderstanding next time ;)
Show 7 quoted lines
>> 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?
Unless the parent of this script decides to ignore those signals, nothing will change: the signals will be delivered at the end of the section and script will die.
Show 19 quoted lines
>> [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.
If read the code correctly (and I'm not a "native perl speaker"), all this is about maintaining consistency in the ad-hoc database for mapping svn revision numbers git commits.
However, blocking some signals is obviously not a good enough protection measure: the program may die on SIGKILL or the power may go off. Even worse, due to the unfortunate record size of 24 bytes there's essentially no way to ensure atomicity of appends.
So if the consistency of this database is critical for git-svn, then it needs a stronger protection mechanism; if it isn't then it's probably not worth the hassle at all.
However, as I said I'm not strong enough in perl to understand which of the above applies and how to do it right. (Yes, ~7kloc perl script using multi-level callback-based svn binding scares me off).
So I just made a fairly simple change that basically preserved the existing behavior but allowed me to set SIGPIPE to SIG_IGN for the duration of the script run in the second patch of the series.
Do you think I should merge this into the commit log, or just drop that appendix from my original log and be done with it?
Roman.
Roman Kagan