Re: [PATCH] t0005: skip signal death exit code test on Windows
On Mon, Jun 10, 2013 at 7:30 AM, Johannes Sixt wrote: > Am 6/9/2013 22:31, schrieb Junio C Hamano: >> Jeff King writes: >> >>> I'm a little negative on handling just SIGTERM. That would make the test >>> pass, but does it really address the overall issue? To me, the >>> usefulness is having exit values with consistent meanings. >> >> Yes. Unless the goal is to give Windows port pratically the same >> signal semantics as ports on other platforms, I do not think special >> casing SIGTERM (unless it is a very common signal on Windows and >> others are unlikely to be useful) buys us much. > > I'm thinking the same. And, no, SIGTERM is not very common on Windows. > I have no strong feelings on SIGTERM, but my knee-jerk reaction is the same. AFAIK, the only issue we've seen with it has been this one, which is synthetic. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Am 6/9/2013 22:31, schrieb Junio C Hamano: > Jeff King writes: > >> I'm a little negative on handling just SIGTERM. That would make the test >> pass, but does it really address the overall issue? To me, the >> usefulness is having exit values with consistent meanings. > > Yes. Unless the goal is to give Windows port pratically the same > signal semantics as ports on other platforms, I do not think special > casing SIGTERM (unless it is a very common signal on Windows and > others are unlikely to be useful) buys us much. I'm thinking the same. And, no, SIGTERM is not very common on Windows. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Jeff King writes: > I'm a little negative on handling just SIGTERM. That would make the test > pass, but does it really address the overall issue? To me, the > usefulness is having exit values with consistent meanings. Yes. Unless the goal is to give Windows port pratically the same signal semantics as ports on other platforms, I do not think special casing SIGTERM (unless it is a very common signal on Windows and others are unlikely to be useful) buys us much. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Fri, Jun 07, 2013 at 12:12:52PM +0200, Erik Faye-Lund wrote: > > Yeah, if it were mingw_raise responsible for this, I would suggest using > > the POSIX shell "128+sig" instead. We could potentially check for > > SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if > > that would create headaches or confusion for other msys programs, > > though. I'd leave that up to the msysgit people to decide whether it is > > worth the trouble. > > > > ...and here's the code to do just that: > [...] > @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) > sigint_fn(SIGINT); > return 0; > > + case SIGTERM: > + if (sigterm_fn == SIG_DFL) > + exit(128 + SIGTERM); > + else if (sigterm_fn != SIG_IGN) > + sigterm_fn(SIGTERM); > + return 0; I'm a little negative on handling just SIGTERM. That would make the test pass, but does it really address the overall issue? To me, the usefulness is having exit values with consistent meanings. Imagine I run a very large git hosting site, and I want to log exceptional conditions (e.g., a git sub-process crashes). What exit code do I get from a SIGSEGV or SIGBUS (or GPF, or whatever Windows calls these)? On Unix systems, this is pretty easy. To be honest, I do not care that much about Windows systems because I would not host a large site on it. :) But IMHO, the point of such a scheme is to be consistent across all signals. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Fri, Jun 7, 2013 at 3:07 PM, Johannes Sixt wrote: > Am 6/7/2013 14:46, schrieb Erik Faye-Lund: >> On Fri, Jun 7, 2013 at 2:19 PM, Johannes Sixt wrote: >>> Am 6/7/2013 14:00, schrieb Erik Faye-Lund: On Fri, Jun 7, 2013 at 12:24 PM, Johannes Sixt wrote: > Am 6/7/2013 12:12, schrieb Erik Faye-Lund: >> diff --git a/compat/mingw.c b/compat/mingw.c >> index b295e2f..8b3c1b4 100644 >> --- a/compat/mingw.c >> +++ b/compat/mingw.c >> @@ -1573,7 +1573,8 @@ static HANDLE timer_event; >> static HANDLE timer_thread; >> static int timer_interval; >> static int one_shot; >> -static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL; >> +static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL, >> +sigterm_fn = SIG_DFL; >> >> /* The timer works like this: >> * The thread, ticktack(), is a trivial routine that most of the time >> @@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig, >> sig_handler_t handler) >> sigint_fn = handler; >> break; >> >> + case SIGTERM: >> + sigterm_fn = handler; >> + break; >> + >> default: >> return signal(sig, handler); >> } >> @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) >> sigint_fn(SIGINT); >> return 0; >> >> + case SIGTERM: >> + if (sigterm_fn == SIG_DFL) >> + exit(128 + SIGTERM); >> + else if (sigterm_fn != SIG_IGN) >> + sigterm_fn(SIGTERM); >> + return 0; >> + >> default: >> return raise(sig); >> } > > That's pointless and does not work. The handler would only be called when > raise() is called, but not when a SIGTERM is received, e.g., via Ctrl-C > from the command line, because that route ends up in MSVCRT, which does > not know about this handler. That's not entirely true. On Windows, there's only *one* way to generate SIGTERM; "signal(SIGTERM)". Ctrl+C does not generate SIGTERM. We generate SIGINT on Ctrl+C in mingw_fgetc, but the default Control+C handler routine calls ExitProcess(): http://msdn.microsoft.com/en-us/library/windows/desktop/ms683242(v=vs.85).aspx >>> >>> But a call to signal(SIGTERM, my_handler) should divert Ctrl+C to >>> my_handler. The unpatched version does, because MSVCRT now knows about >>> my_handler and sets things up so that the event handler calls my_handler. >> >> No, it does not: >> Ctrl+C raises SIGINT, not SIGTERM. > > > > You are right. Your change would "fix" SIGTERM as it can be raised only > via raise() on Windows nor can it be caught when a process is killed via > mingw_kill(...,SIGTERM) by another process. > > But then the current handling of SIGINT in compat/mingw.c is broken. The > handler is not propagated to MSVCRT, and after a SIGINT handler is > installed, Ctrl+C still terminates the process. No? Yeah, probably. I wasn't aware that it handled SIGINT, but yeah it does. So this was indeed a regression. > BTW, isn't mingw_signal() bogus in that it returns the SIGALRM handler > even if a SIGINT handler is installed? Yep, that's a bug. Thanks for noticing. I've pushed out a branch here that tries to address these issues, but I haven't had time to test them. I'll post the series when I have. In the mean time: https://github.com/kusma/git/tree/win32-signal-raise -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Am 6/7/2013 14:46, schrieb Erik Faye-Lund: > On Fri, Jun 7, 2013 at 2:19 PM, Johannes Sixt wrote: >> Am 6/7/2013 14:00, schrieb Erik Faye-Lund: >>> On Fri, Jun 7, 2013 at 12:24 PM, Johannes Sixt wrote: Am 6/7/2013 12:12, schrieb Erik Faye-Lund: > diff --git a/compat/mingw.c b/compat/mingw.c > index b295e2f..8b3c1b4 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1573,7 +1573,8 @@ static HANDLE timer_event; > static HANDLE timer_thread; > static int timer_interval; > static int one_shot; > -static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL; > +static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL, > +sigterm_fn = SIG_DFL; > > /* The timer works like this: > * The thread, ticktack(), is a trivial routine that most of the time > @@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig, > sig_handler_t handler) > sigint_fn = handler; > break; > > + case SIGTERM: > + sigterm_fn = handler; > + break; > + > default: > return signal(sig, handler); > } > @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) > sigint_fn(SIGINT); > return 0; > > + case SIGTERM: > + if (sigterm_fn == SIG_DFL) > + exit(128 + SIGTERM); > + else if (sigterm_fn != SIG_IGN) > + sigterm_fn(SIGTERM); > + return 0; > + > default: > return raise(sig); > } That's pointless and does not work. The handler would only be called when raise() is called, but not when a SIGTERM is received, e.g., via Ctrl-C from the command line, because that route ends up in MSVCRT, which does not know about this handler. >>> >>> That's not entirely true. On Windows, there's only *one* way to >>> generate SIGTERM; "signal(SIGTERM)". Ctrl+C does not generate SIGTERM. >>> We generate SIGINT on Ctrl+C in mingw_fgetc, but the default Control+C >>> handler routine calls ExitProcess(): >>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683242(v=vs.85).aspx >> >> But a call to signal(SIGTERM, my_handler) should divert Ctrl+C to >> my_handler. The unpatched version does, because MSVCRT now knows about >> my_handler and sets things up so that the event handler calls my_handler. > > No, it does not: > Ctrl+C raises SIGINT, not SIGTERM. You are right. Your change would "fix" SIGTERM as it can be raised only via raise() on Windows nor can it be caught when a process is killed via mingw_kill(...,SIGTERM) by another process. But then the current handling of SIGINT in compat/mingw.c is broken. The handler is not propagated to MSVCRT, and after a SIGINT handler is installed, Ctrl+C still terminates the process. No? BTW, isn't mingw_signal() bogus in that it returns the SIGALRM handler even if a SIGINT handler is installed? -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Fri, Jun 7, 2013 at 2:19 PM, Johannes Sixt wrote:
> Am 6/7/2013 14:00, schrieb Erik Faye-Lund:
>> On Fri, Jun 7, 2013 at 12:24 PM, Johannes Sixt wrote:
>>> Am 6/7/2013 12:12, schrieb Erik Faye-Lund:
On Thu, Jun 6, 2013 at 7:40 PM, Jeff King wrote:
> On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote:
>
>>> The particular deficiency is that when a signal is raise()d whose
>>> SIG_DFL
>>> action will cause process death (SIGTERM in this case), the
>>> implementation of raise() just calls exit(3).
>>
>> After a bit of web searching, it seems to me that this behaviour of
>> raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls
>> that. In other words, "the implementation of raise()" is at an even
>> lower level than mingw/msys, and I would agree that it is a platform
>> issue.
>
> Yeah, if it were mingw_raise responsible for this, I would suggest using
> the POSIX shell "128+sig" instead. We could potentially check for
> SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if
> that would create headaches or confusion for other msys programs,
> though. I'd leave that up to the msysgit people to decide whether it is
> worth the trouble.
>
...and here's the code to do just that:
diff --git a/compat/mingw.c b/compat/mingw.c
index b295e2f..8b3c1b4 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1573,7 +1573,8 @@ static HANDLE timer_event;
static HANDLE timer_thread;
static int timer_interval;
static int one_shot;
-static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL;
+static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL,
+sigterm_fn = SIG_DFL;
/* The timer works like this:
* The thread, ticktack(), is a trivial routine that most of the time
@@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig,
sig_handler_t handler)
sigint_fn = handler;
break;
+ case SIGTERM:
+ sigterm_fn = handler;
+ break;
+
default:
return signal(sig, handler);
}
@@ -1715,6 +1720,13 @@ int mingw_raise(int sig)
sigint_fn(SIGINT);
return 0;
+ case SIGTERM:
+ if (sigterm_fn == SIG_DFL)
+ exit(128 + SIGTERM);
+ else if (sigterm_fn != SIG_IGN)
+ sigterm_fn(SIGTERM);
+ return 0;
+
default:
return raise(sig);
}
>>>
>>> That's pointless and does not work. The handler would only be called when
>>> raise() is called, but not when a SIGTERM is received, e.g., via Ctrl-C
>>> from the command line, because that route ends up in MSVCRT, which does
>>> not know about this handler.
>>
>> That's not entirely true. On Windows, there's only *one* way to
>> generate SIGTERM; "signal(SIGTERM)". Ctrl+C does not generate SIGTERM.
>> We generate SIGINT on Ctrl+C in mingw_fgetc, but the default Control+C
>> handler routine calls ExitProcess():
>> http://msdn.microsoft.com/en-us/library/windows/desktop/ms683242(v=vs.85).aspx
>
> But a call to signal(SIGTERM, my_handler) should divert Ctrl+C to
> my_handler. The unpatched version does, because MSVCRT now knows about
> my_handler and sets things up so that the event handler calls my_handler.
No, it does not:
--->8---
#include
#include
#include
void my_handler(int signum)
{
printf("signal: %d\n", signum);
exit(1);
}
int main()
{
signal(SIGTERM, my_handler);
while (1);
return 0;
}
--->8---
This quietly kills the process on Windows with MSVCRT's
signal-implementation. In fact SIGTERM isn't raised on Linux either.
Ctrl+C raises SIGINT, not SIGTERM.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Am 6/7/2013 14:00, schrieb Erik Faye-Lund: > On Fri, Jun 7, 2013 at 12:24 PM, Johannes Sixt wrote: >> Am 6/7/2013 12:12, schrieb Erik Faye-Lund: >>> On Thu, Jun 6, 2013 at 7:40 PM, Jeff King wrote: On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote: >> The particular deficiency is that when a signal is raise()d whose SIG_DFL >> action will cause process death (SIGTERM in this case), the >> implementation of raise() just calls exit(3). > > After a bit of web searching, it seems to me that this behaviour of > raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls > that. In other words, "the implementation of raise()" is at an even > lower level than mingw/msys, and I would agree that it is a platform > issue. Yeah, if it were mingw_raise responsible for this, I would suggest using the POSIX shell "128+sig" instead. We could potentially check for SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if that would create headaches or confusion for other msys programs, though. I'd leave that up to the msysgit people to decide whether it is worth the trouble. >>> >>> ...and here's the code to do just that: >>> >>> diff --git a/compat/mingw.c b/compat/mingw.c >>> index b295e2f..8b3c1b4 100644 >>> --- a/compat/mingw.c >>> +++ b/compat/mingw.c >>> @@ -1573,7 +1573,8 @@ static HANDLE timer_event; >>> static HANDLE timer_thread; >>> static int timer_interval; >>> static int one_shot; >>> -static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL; >>> +static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL, >>> +sigterm_fn = SIG_DFL; >>> >>> /* The timer works like this: >>> * The thread, ticktack(), is a trivial routine that most of the time >>> @@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig, >>> sig_handler_t handler) >>> sigint_fn = handler; >>> break; >>> >>> + case SIGTERM: >>> + sigterm_fn = handler; >>> + break; >>> + >>> default: >>> return signal(sig, handler); >>> } >>> @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) >>> sigint_fn(SIGINT); >>> return 0; >>> >>> + case SIGTERM: >>> + if (sigterm_fn == SIG_DFL) >>> + exit(128 + SIGTERM); >>> + else if (sigterm_fn != SIG_IGN) >>> + sigterm_fn(SIGTERM); >>> + return 0; >>> + >>> default: >>> return raise(sig); >>> } >> >> That's pointless and does not work. The handler would only be called when >> raise() is called, but not when a SIGTERM is received, e.g., via Ctrl-C >> from the command line, because that route ends up in MSVCRT, which does >> not know about this handler. > > That's not entirely true. On Windows, there's only *one* way to > generate SIGTERM; "signal(SIGTERM)". Ctrl+C does not generate SIGTERM. > We generate SIGINT on Ctrl+C in mingw_fgetc, but the default Control+C > handler routine calls ExitProcess(): > http://msdn.microsoft.com/en-us/library/windows/desktop/ms683242(v=vs.85).aspx But a call to signal(SIGTERM, my_handler) should divert Ctrl+C to my_handler. The unpatched version does, because MSVCRT now knows about my_handler and sets things up so that the event handler calls my_handler. But your patched version bypasses MSVCRT, and the default (whatever MSVCRT has set up) happens, and my_handler is not called. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Fri, Jun 7, 2013 at 12:24 PM, Johannes Sixt wrote: > Am 6/7/2013 12:12, schrieb Erik Faye-Lund: >> On Thu, Jun 6, 2013 at 7:40 PM, Jeff King wrote: >>> On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote: >>> > The particular deficiency is that when a signal is raise()d whose SIG_DFL > action will cause process death (SIGTERM in this case), the > implementation of raise() just calls exit(3). After a bit of web searching, it seems to me that this behaviour of raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls that. In other words, "the implementation of raise()" is at an even lower level than mingw/msys, and I would agree that it is a platform issue. >>> >>> Yeah, if it were mingw_raise responsible for this, I would suggest using >>> the POSIX shell "128+sig" instead. We could potentially check for >>> SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if >>> that would create headaches or confusion for other msys programs, >>> though. I'd leave that up to the msysgit people to decide whether it is >>> worth the trouble. >>> >> >> ...and here's the code to do just that: >> >> diff --git a/compat/mingw.c b/compat/mingw.c >> index b295e2f..8b3c1b4 100644 >> --- a/compat/mingw.c >> +++ b/compat/mingw.c >> @@ -1573,7 +1573,8 @@ static HANDLE timer_event; >> static HANDLE timer_thread; >> static int timer_interval; >> static int one_shot; >> -static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL; >> +static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL, >> +sigterm_fn = SIG_DFL; >> >> /* The timer works like this: >> * The thread, ticktack(), is a trivial routine that most of the time >> @@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig, >> sig_handler_t handler) >> sigint_fn = handler; >> break; >> >> + case SIGTERM: >> + sigterm_fn = handler; >> + break; >> + >> default: >> return signal(sig, handler); >> } >> @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) >> sigint_fn(SIGINT); >> return 0; >> >> + case SIGTERM: >> + if (sigterm_fn == SIG_DFL) >> + exit(128 + SIGTERM); >> + else if (sigterm_fn != SIG_IGN) >> + sigterm_fn(SIGTERM); >> + return 0; >> + >> default: >> return raise(sig); >> } > > That's pointless and does not work. The handler would only be called when > raise() is called, but not when a SIGTERM is received, e.g., via Ctrl-C > from the command line, because that route ends up in MSVCRT, which does > not know about this handler. That's not entirely true. On Windows, there's only *one* way to generate SIGTERM; "signal(SIGTERM)". Ctrl+C does not generate SIGTERM. We generate SIGINT on Ctrl+C in mingw_fgetc, but the default Control+C handler routine calls ExitProcess(): http://msdn.microsoft.com/en-us/library/windows/desktop/ms683242(v=vs.85).aspx So I believe this *does* entirely fix SIGTERM (as we currently know it) on Windows. SIGINT is still not entirely clean, but we can fix that on a case-by-case basis. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Am 6/7/2013 12:12, schrieb Erik Faye-Lund: > On Thu, Jun 6, 2013 at 7:40 PM, Jeff King wrote: >> On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote: >> The particular deficiency is that when a signal is raise()d whose SIG_DFL action will cause process death (SIGTERM in this case), the implementation of raise() just calls exit(3). >>> >>> After a bit of web searching, it seems to me that this behaviour of >>> raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls >>> that. In other words, "the implementation of raise()" is at an even >>> lower level than mingw/msys, and I would agree that it is a platform >>> issue. >> >> Yeah, if it were mingw_raise responsible for this, I would suggest using >> the POSIX shell "128+sig" instead. We could potentially check for >> SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if >> that would create headaches or confusion for other msys programs, >> though. I'd leave that up to the msysgit people to decide whether it is >> worth the trouble. >> > > ...and here's the code to do just that: > > diff --git a/compat/mingw.c b/compat/mingw.c > index b295e2f..8b3c1b4 100644 > --- a/compat/mingw.c > +++ b/compat/mingw.c > @@ -1573,7 +1573,8 @@ static HANDLE timer_event; > static HANDLE timer_thread; > static int timer_interval; > static int one_shot; > -static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL; > +static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL, > +sigterm_fn = SIG_DFL; > > /* The timer works like this: > * The thread, ticktack(), is a trivial routine that most of the time > @@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig, > sig_handler_t handler) > sigint_fn = handler; > break; > > + case SIGTERM: > + sigterm_fn = handler; > + break; > + > default: > return signal(sig, handler); > } > @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) > sigint_fn(SIGINT); > return 0; > > + case SIGTERM: > + if (sigterm_fn == SIG_DFL) > + exit(128 + SIGTERM); > + else if (sigterm_fn != SIG_IGN) > + sigterm_fn(SIGTERM); > + return 0; > + > default: > return raise(sig); > } That's pointless and does not work. The handler would only be called when raise() is called, but not when a SIGTERM is received, e.g., via Ctrl-C from the command line, because that route ends up in MSVCRT, which does not know about this handler. If you want to follow this route, you must emulate everything that MSVCRT already does for us, and that's quite a lot, in particular, when some form of thread safety is required. -- Hannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 6, 2013 at 7:40 PM, Jeff King wrote: > On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote: > >> > The particular deficiency is that when a signal is raise()d whose SIG_DFL >> > action will cause process death (SIGTERM in this case), the >> > implementation of raise() just calls exit(3). >> >> After a bit of web searching, it seems to me that this behaviour of >> raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls >> that. In other words, "the implementation of raise()" is at an even >> lower level than mingw/msys, and I would agree that it is a platform >> issue. > > Yeah, if it were mingw_raise responsible for this, I would suggest using > the POSIX shell "128+sig" instead. We could potentially check for > SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if > that would create headaches or confusion for other msys programs, > though. I'd leave that up to the msysgit people to decide whether it is > worth the trouble. > ...and here's the code to do just that: diff --git a/compat/mingw.c b/compat/mingw.c index b295e2f..8b3c1b4 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1573,7 +1573,8 @@ static HANDLE timer_event; static HANDLE timer_thread; static int timer_interval; static int one_shot; -static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL; +static sig_handler_t timer_fn = SIG_DFL, sigint_fn = SIG_DFL, +sigterm_fn = SIG_DFL; /* The timer works like this: * The thread, ticktack(), is a trivial routine that most of the time @@ -1688,6 +1689,10 @@ sig_handler_t mingw_signal(int sig, sig_handler_t handler) sigint_fn = handler; break; + case SIGTERM: + sigterm_fn = handler; + break; + default: return signal(sig, handler); } @@ -1715,6 +1720,13 @@ int mingw_raise(int sig) sigint_fn(SIGINT); return 0; + case SIGTERM: + if (sigterm_fn == SIG_DFL) + exit(128 + SIGTERM); + else if (sigterm_fn != SIG_IGN) + sigterm_fn(SIGTERM); + return 0; + default: return raise(sig); } -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Fri, Jun 7, 2013 at 12:01 PM, Erik Faye-Lund wrote: > On Thu, Jun 6, 2013 at 8:34 AM, Johannes Sixt wrote: >> From: Johannes Sixt >> >> The test case depends on that test-sigchain can commit suicide by a call >> to raise(SIGTERM) in a way that run-command.c::wait_or_whine() can detect >> as death through a signal. There are no POSIX signals on Windows, and a >> sufficiently close emulation is not available in the Microsoft C runtime >> (and probably not even possible). >> >> The particular deficiency is that when a signal is raise()d whose SIG_DFL >> action will cause process death (SIGTERM in this case), the >> implementation of raise() just calls exit(3). >> >> We could check for exit code 3 in addition to 143, but that would miss >> the point of the test entirely. Hence, just skip it on Windows. >> > > Huh? We do "exit(128 + sigint);" in mingw_raise these days, no? > > Or is the signal triggered from a non-git process? Argh, I'm blind. Yeah, SIGTERM, not SIGINT... -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 6, 2013 at 8:34 AM, Johannes Sixt wrote: > From: Johannes Sixt > > The test case depends on that test-sigchain can commit suicide by a call > to raise(SIGTERM) in a way that run-command.c::wait_or_whine() can detect > as death through a signal. There are no POSIX signals on Windows, and a > sufficiently close emulation is not available in the Microsoft C runtime > (and probably not even possible). > > The particular deficiency is that when a signal is raise()d whose SIG_DFL > action will cause process death (SIGTERM in this case), the > implementation of raise() just calls exit(3). > > We could check for exit code 3 in addition to 143, but that would miss > the point of the test entirely. Hence, just skip it on Windows. > Huh? We do "exit(128 + sigint);" in mingw_raise these days, no? Or is the signal triggered from a non-git process? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Am 6/6/2013 19:40, schrieb Jeff King:
> On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote:
>
>>> The particular deficiency is that when a signal is raise()d whose SIG_DFL
>>> action will cause process death (SIGTERM in this case), the
>>> implementation of raise() just calls exit(3).
>>
>> After a bit of web searching, it seems to me that this behaviour of
>> raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls
>> that. In other words, "the implementation of raise()" is at an even
>> lower level than mingw/msys, and I would agree that it is a platform
>> issue.
>
> Yeah, if it were mingw_raise responsible for this, I would suggest using
> the POSIX shell "128+sig" instead. We could potentially check for
> SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if
> that would create headaches or confusion for other msys programs,
> though. I'd leave that up to the msysgit people to decide whether it is
> worth the trouble.
Even if we move the signal emulation closer to POSIX in the way that you
suggested (if that were possible, I haven't checked), the emulation is
still not complete, because we would have addressed only raise().
Therefore, personally I would like to delay the issue until there is a
user (script) that depends on POSIXly exit codes.
As far as t0005.2 is concerned, its the best to just concede that Windows
does not have POSIX-like behavior as far as signals are concerned, and
skip the test.
We could also sweep the issue under the rug with the patch below, which
works because SIGALRM does not exist in MSVCRT and is handled entirely by
compat/mingw.c. But it goes into the wrong direction, IMO.
diff --git a/t/t0005-signals.sh b/t/t0005-signals.sh
index ad9e604..68b6c3b 100755
--- a/t/t0005-signals.sh
+++ b/t/t0005-signals.sh
@@ -12,9 +12,8 @@ EOF
test_expect_success 'sigchain works' '
test-sigchain >actual
case "$?" in
- 143) true ;; # POSIX w/ SIGTERM=15
- 271) true ;; # ksh w/ SIGTERM=15
- 3) true ;; # Windows
+ 142) true ;; # POSIX w/ SIGALRM=14
+ 270) true ;; # ksh w/ SIGTERM=14
*) false ;;
esac &&
test_cmp expect actual
@@ -23,8 +22,8 @@ test_expect_success 'sigchain works' '
test_expect_success 'signals are propagated using shell convention' '
# we use exec here to avoid any sub-shell interpretation
# of the exit code
- git config alias.sigterm "!exec test-sigchain" &&
- test_expect_code 143 git sigterm
+ git config alias.sigalrm "!exec test-sigchain" &&
+ test_expect_code 142 git sigalrm
'
test_done
diff --git a/test-sigchain.c b/test-sigchain.c
index 42db234..0233a39 100644
--- a/test-sigchain.c
+++ b/test-sigchain.c
@@ -14,9 +14,9 @@ X(three)
#undef X
int main(int argc, char **argv) {
- sigchain_push(SIGTERM, one);
- sigchain_push(SIGTERM, two);
- sigchain_push(SIGTERM, three);
- raise(SIGTERM);
+ sigchain_push(SIGALRM, one);
+ sigchain_push(SIGALRM, two);
+ sigchain_push(SIGALRM, three);
+ raise(SIGALRM);
return 0;
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 6, 2013 at 12:21 PM, Junio C Hamano wrote: > Jeff King writes: >> If somebody wants to write a note somewhere in the git >> documentation, that's fine with me, but I'm not clear on exactly >> what it would even say. > > I agree with both points. I can suggest to clarify the log message > a bit with "the implementation of raise() in msvcrt (Microsoft C > Runtime library) just calls exit(3)", but that does not address the > end-user documentation issue. > > I tried to summarize the issue for end-user documentation and came > up with this: > > The Git implementation on MinGW That's not accurate at all. MinGW is essentially a compiler for Windows. It doesn't matter if you use MinGW, Visual C++, or any other compiler for Windows, the result would be the same. > But when stated this way, it feels that it belongs to Msysgit > documentation, not ours, at least to me. Are we saying then that Git doesn't support Windows? That wouldn't be wise considering it's one of the most widely used reasons to argue that Git is not a good choice of a SCM; lack of Windows support. The truth of the matter is that exit codes are platform-dependent. Period. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 06, 2013 at 10:21:47AM -0700, Junio C Hamano wrote: > > The particular deficiency is that when a signal is raise()d whose SIG_DFL > > action will cause process death (SIGTERM in this case), the > > implementation of raise() just calls exit(3). > > After a bit of web searching, it seems to me that this behaviour of > raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls > that. In other words, "the implementation of raise()" is at an even > lower level than mingw/msys, and I would agree that it is a platform > issue. Yeah, if it were mingw_raise responsible for this, I would suggest using the POSIX shell "128+sig" instead. We could potentially check for SIG_DFL[1] mingw_raise and intercept and exit there. I don't know if that would create headaches or confusion for other msys programs, though. I'd leave that up to the msysgit people to decide whether it is worth the trouble. [1] I'd use sigaction to do that on POSIX, but I would not be surprised to find that there is no support for it in msys. :) > I tried to summarize the issue for end-user documentation and came > up with this: > > The Git implementation on MinGW exits with status code 3 upon > receiving an uncaught process-terminating signal, just like any > program that link with msvcrt (Microsoft C Runtime library) > whose raise() implementation just calls exit(3). This is > different from Git on POSIX, which reports a death by receiving > a signal with the exit status code (128 + signal number). > > But when stated this way, it feels that it belongs to Msysgit > documentation, not ours, at least to me. Yeah, I think I agree. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
Jeff King writes: > On Thu, Jun 06, 2013 at 01:41:05AM -0500, Felipe Contreras wrote: > >> > Thanks. I wasn't quite clear on how the signal handling worked on >> > Windows, but from your description, I agree there is not any point in >> > running the test at all. >> >> Shouldn't we clarify that Git exit codes only work on UNIX-like >> operating systems? > > Clarify where? My impression is that this issue is well-known in the > msys world, and it is a platform issue, not a git issue. I actually was scratching my head while reading "the implementation of raise() just calls exit(3)." part, in this: > The particular deficiency is that when a signal is raise()d whose SIG_DFL > action will cause process death (SIGTERM in this case), the > implementation of raise() just calls exit(3). After a bit of web searching, it seems to me that this behaviour of raise() is in msvcrt, and compat/mingw.c::mingw_raise() just calls that. In other words, "the implementation of raise()" is at an even lower level than mingw/msys, and I would agree that it is a platform issue. > If somebody wants to write a note somewhere in the git > documentation, that's fine with me, but I'm not clear on exactly > what it would even say. I agree with both points. I can suggest to clarify the log message a bit with "the implementation of raise() in msvcrt (Microsoft C Runtime library) just calls exit(3)", but that does not address the end-user documentation issue. I tried to summarize the issue for end-user documentation and came up with this: The Git implementation on MinGW exits with status code 3 upon receiving an uncaught process-terminating signal, just like any program that link with msvcrt (Microsoft C Runtime library) whose raise() implementation just calls exit(3). This is different from Git on POSIX, which reports a death by receiving a signal with the exit status code (128 + signal number). But when stated this way, it feels that it belongs to Msysgit documentation, not ours, at least to me. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 6, 2013 at 1:44 AM, Jeff King wrote: > On Thu, Jun 06, 2013 at 01:41:05AM -0500, Felipe Contreras wrote: > >> > Thanks. I wasn't quite clear on how the signal handling worked on >> > Windows, but from your description, I agree there is not any point in >> > running the test at all. >> >> Shouldn't we clarify that Git exit codes only work on UNIX-like >> operating systems? > > Clarify where? Documentation/technical/api-run-command.txt > My impression is that this issue is well-known in the > msys world, and it is a platform issue, not a git issue. If somebody > wants to write a note somewhere in the git documentation, that's fine > with me, but I'm not clear on exactly what it would even say. That the exit code is not the same in Windows (not msys). -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 06, 2013 at 01:41:05AM -0500, Felipe Contreras wrote: > > Thanks. I wasn't quite clear on how the signal handling worked on > > Windows, but from your description, I agree there is not any point in > > running the test at all. > > Shouldn't we clarify that Git exit codes only work on UNIX-like > operating systems? Clarify where? My impression is that this issue is well-known in the msys world, and it is a platform issue, not a git issue. If somebody wants to write a note somewhere in the git documentation, that's fine with me, but I'm not clear on exactly what it would even say. -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 6, 2013 at 1:37 AM, Jeff King wrote: > On Thu, Jun 06, 2013 at 08:34:41AM +0200, Johannes Sixt wrote: > >> From: Johannes Sixt >> >> The test case depends on that test-sigchain can commit suicide by a call >> to raise(SIGTERM) in a way that run-command.c::wait_or_whine() can detect >> as death through a signal. There are no POSIX signals on Windows, and a >> sufficiently close emulation is not available in the Microsoft C runtime >> (and probably not even possible). >> >> The particular deficiency is that when a signal is raise()d whose SIG_DFL >> action will cause process death (SIGTERM in this case), the >> implementation of raise() just calls exit(3). >> >> We could check for exit code 3 in addition to 143, but that would miss >> the point of the test entirely. Hence, just skip it on Windows. > > Thanks. I wasn't quite clear on how the signal handling worked on > Windows, but from your description, I agree there is not any point in > running the test at all. Shouldn't we clarify that Git exit codes only work on UNIX-like operating systems? -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] t0005: skip signal death exit code test on Windows
On Thu, Jun 06, 2013 at 08:34:41AM +0200, Johannes Sixt wrote: > From: Johannes Sixt > > The test case depends on that test-sigchain can commit suicide by a call > to raise(SIGTERM) in a way that run-command.c::wait_or_whine() can detect > as death through a signal. There are no POSIX signals on Windows, and a > sufficiently close emulation is not available in the Microsoft C runtime > (and probably not even possible). > > The particular deficiency is that when a signal is raise()d whose SIG_DFL > action will cause process death (SIGTERM in this case), the > implementation of raise() just calls exit(3). > > We could check for exit code 3 in addition to 143, but that would miss > the point of the test entirely. Hence, just skip it on Windows. Thanks. I wasn't quite clear on how the signal handling worked on Windows, but from your description, I agree there is not any point in running the test at all. Acked-by: Jeff King -Peff -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
