Skip to content
This repository was archived by the owner on Oct 27, 2022. It is now read-only.

Commit 3eb5c96

Browse files
authored
Functional test for restore_signals, include signal.h. (#51)
restore_signals=True lacked a functional test. It was possible, though unlikely in my experience, to build the extension module against a Python interpreter that was not already including <signal.h> via Python.h. I believe this might have happened on builds that did not enable --with-fpectl? The Debian CPython 2.7.13 package does not exhibit the problem. Regardless, testing for signal.h and explicitly including it is always the right thing to do.
1 parent 9647763 commit 3eb5c96

File tree

6 files changed

+43
-8
lines changed

6 files changed

+43
-8
lines changed

ChangeLog

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,11 @@
1+
-----------------
2+
2018-06-04 3.5.2 (not yet released)
3+
-----------------
4+
* Explicitly include <signal.h> in _posixsubprocess_helpers.c; it already
5+
gets configure checked and pulled in via Python's own <Python.h> but it
6+
is better to be explicit. #IWYU
7+
* Adds a functional test for restore_signals=True behavior.
8+
19
-----------------
210
2018-05-21 3.5.1
311
-----------------

_posixsubprocess_config.h.in

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,9 @@
2525
/* Define to 1 if you have the `setsid' function. */
2626
#undef HAVE_SETSID
2727

28+
/* Define to 1 if you have the <signal.h> header file. */
29+
#undef HAVE_SIGNAL_H
30+
2831
/* Define to 1 if you have the <stdint.h> header file. */
2932
#undef HAVE_STDINT_H
3033

_posixsubprocess_helpers.c

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,12 @@
22
This file is #included by _posixsubprocess.c and the functions
33
are declared static to avoid exposing them outside this module. */
44

5+
/* _posixsubprocess_config.h was already included by _posixsubprocess.c
6+
* which is #include'ing us despite the .c name. HAVE_SIGNAL_H comes
7+
* from there. Yes, confusing! */
8+
#ifdef HAVE_SIGNAL_H
9+
#include <signal.h>
10+
#endif
511
#include "unicodeobject.h"
612

713
#if (PY_VERSION_HEX < 0x02050000)

configure

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3477,7 +3477,7 @@ fi
34773477
done
34783478
34793479
3480-
for ac_header in unistd.h fcntl.h \
3480+
for ac_header in unistd.h fcntl.h signal.h \
34813481
sys/cdefs.h sys/types.h sys/stat.h sys/syscall.h
34823482
do :
34833483
as_ac_Header=`$as_echo "ac_cv_header_$ac_header" | $as_tr_sh`

configure.ac

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -137,7 +137,7 @@ fi
137137
######## The stuff we _actually_ want to care about. ########
138138

139139
AC_HEADER_STDC
140-
AC_CHECK_HEADERS(unistd.h fcntl.h \
140+
AC_CHECK_HEADERS(unistd.h fcntl.h signal.h \
141141
sys/cdefs.h sys/types.h sys/stat.h sys/syscall.h)
142142
AC_HEADER_DIRENT
143143

test_subprocess32.py

Lines changed: 24 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1245,12 +1245,30 @@ def test_exception_bad_args_0(self):
12451245
else:
12461246
self.fail("Expected OSError: %s" % desired_exception)
12471247

1248+
#@unittest.skipIf(not os.path.exists('/proc/self/status'))
12481249
def test_restore_signals(self):
1249-
# Code coverage for both values of restore_signals to make sure it
1250-
# at least does not blow up.
1251-
# A test for behavior would be complex. Contributions welcome.
1252-
subprocess.call([sys.executable, "-c", ""], restore_signals=True)
1253-
subprocess.call([sys.executable, "-c", ""], restore_signals=False)
1250+
if not os.path.exists('/proc/self/status'):
1251+
print("SKIP - Functional test requires /proc/self/status.")
1252+
return
1253+
# Blindly assume that cat exists on systems with /proc/self/status...
1254+
default_proc_status = subprocess.check_output(
1255+
['cat', '/proc/self/status'],
1256+
restore_signals=False)
1257+
for line in default_proc_status.splitlines():
1258+
if line.startswith(b'SigIgn'):
1259+
default_sig_ign_mask = line
1260+
break
1261+
else:
1262+
self.skipTest("SigIgn not found in /proc/self/status.")
1263+
restored_proc_status = subprocess.check_output(
1264+
['cat', '/proc/self/status'],
1265+
restore_signals=True)
1266+
for line in restored_proc_status.splitlines():
1267+
if line.startswith(b'SigIgn'):
1268+
restored_sig_ign_mask = line
1269+
break
1270+
# restore_signals=True should've unblocked SIGPIPE and friends.
1271+
self.assertNotEqual(default_sig_ign_mask, restored_sig_ign_mask)
12541272

12551273
def test_start_new_session(self):
12561274
# For code coverage of calling setsid(). We don't care if we get an
@@ -2258,7 +2276,7 @@ def test_cloexec_pass_fds(self):
22582276
finally:
22592277
fd_reader_proc.wait()
22602278
finally:
2261-
null_reader_proc.wait()
2279+
null_reader_proc.wait()
22622280

22632281

22642282
if not getattr(subprocess, '_posixsubprocess', False):

0 commit comments

Comments
 (0)