Merge lp:~gz/bzr/ignore_sigquit_in_ssh_child_162502 into lp:bzr

[Martin Packman]

(mbp, for gz) mask out sigquit in ssh child process so that breakin doesn't kill it

[Martin Packman (gz)]

Trivial fix for bug 162502 that seemed worth jumping on while I was in the neighbourhood.

There might be some consequences that haven't been thought of here, shout if any come to mind.

[Martin Pool (mbp)]

Looks good, needs news.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Martin [gz] wrote:
> Martin [gz] has proposed merging lp:~gz/bzr/ignore_sigquit_in_ssh_child_162502 into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #162502 SIGQUIT kills the child SSH process
> https://bugs.launchpad.net/bugs/162502
>
>
> Trivial fix for bug 162502 that seemed worth jumping on while I was in the neighbourhood.
>
> There might be some consequences that haven't been thought of here, shout if any come to mind.
>

signal.SIGQUIT doesn't exist on Windows so you need a getattr()

John
=:->
 review: needsfixing

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkuC220ACgkQJdeBCYSNAAMDQACeKLt0tOsiz7v8xhg/dyuYCQQH
Eh4An1clVbAo8qy5XC4lfCOcY4jJWwVq
=VYAE
-----END PGP SIGNATURE-----

review: Needs Fixing

[Martin Packman (gz)]

> Looks good, needs news.

Done.

> signal.SIGQUIT doesn't exist on Windows so you need a getattr()

It can't be seen from the diff in this merge proposal, but the change is already on a nix-only path, after all no fork, no preexec_fn on Windows.

[Andrew Bennetts (spiv)]

Hmm, this is a bit unfortunate (ideally we wouldn't need to set non-default signal handlers in child processes), but probably the best we can do. I hadn't realised that Ctrl-\ causes SIGQUIT to be sent to the entire process group, but it does so there's no other choice if we want to keep using SIGQUIT like this.

review: Approve

[Martin Pool (mbp)]

sent to pqm

=== modified file 'NEWS'
--- NEWS	2010-02-21 07:09:22 +0000
+++ NEWS	2010-02-22 23:55:24 +0000
@@ -86,6 +86,10 @@
   ``add``.
   (Parth Malwankar, #335033, #300001)
 
+* SSH child processes will now ignore SIGQUIT on nix systems so breaking into
+  the debugger won't kill the session.
+  (Martin <[e-mail]>, #162502)
+
 API Changes
 ***********
 

=== modified file 'bzrlib/transport/ssh.py'
--- bzrlib/transport/ssh.py	2009-11-25 07:27:43 +0000
+++ bzrlib/transport/ssh.py	2010-02-22 23:55:24 +0000
@@ -173,12 +173,15 @@
 register_ssh_vendor = _ssh_vendor_manager.register_vendor
 
 
-def _ignore_sigint():
+def _ignore_signals():
     # TODO: This should possibly ignore SIGHUP as well, but bzr currently
     # doesn't handle it itself.
     # <https://launchpad.net/products/bzr/+bug/41433/+index>
     import signal
     signal.signal(signal.SIGINT, signal.SIG_IGN)
+    # GZ 2010-02-19: Perhaps make this check if breakin is installed instead
+    if signal.getsignal(signal.SIGQUIT) != signal.SIG_DFL:
+        signal.signal(signal.SIGQUIT, signal.SIG_IGN)
 
 
 class SocketAsChannelAdapter(object):
@@ -634,7 +637,7 @@
         # Running it in a separate process group is not good because then it
         # can't get non-echoed input of a password or passphrase.
         # <https://launchpad.net/products/bzr/+bug/40508>
-        return {'preexec_fn': _ignore_sigint,
+        return {'preexec_fn': _ignore_signals,
                 'close_fds': True,
                 }