From 684a31a35d9c564199a6186437e7a11a82c86ce6 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 28 Oct 2024 19:58:20 +0000 Subject: [PATCH 1/5] Simplify conv_confname --- Lib/test/support/os_helper.py | 3 +- Lib/test/test_os.py | 2 +- Lib/test/test_posix.py | 34 +++++- ...-10-28-19-49-18.gh-issue-118201.v41XXh.rst | 2 + Modules/clinic/posixmodule.c.h | 8 +- Modules/posixmodule.c | 114 ++++-------------- 6 files changed, 65 insertions(+), 98 deletions(-) create mode 100644 Misc/NEWS.d/next/Library/2024-10-28-19-49-18.gh-issue-118201.v41XXh.rst diff --git a/Lib/test/support/os_helper.py b/Lib/test/support/os_helper.py index 891405943b78c5..8071c248b9b67e 100644 --- a/Lib/test/support/os_helper.py +++ b/Lib/test/support/os_helper.py @@ -632,8 +632,7 @@ def fd_count(): if hasattr(os, 'sysconf'): try: MAXFD = os.sysconf("SC_OPEN_MAX") - except (OSError, ValueError): - # gh-118201: ValueError is raised intermittently on iOS + except OSError: pass old_modes = None diff --git a/Lib/test/test_os.py b/Lib/test/test_os.py index 307f0f11ddc33f..a0da5b3fd3dc62 100644 --- a/Lib/test/test_os.py +++ b/Lib/test/test_os.py @@ -2447,8 +2447,8 @@ def test_fchown(self): support.is_emscripten or support.is_wasi, "musl libc issue on Emscripten/WASI, bpo-46390" ) - @unittest.skipIf(support.is_apple_mobile, "gh-118201: Test is flaky on iOS") def test_fpathconf(self): + self.assertIn("PC_NAME_MAX", os.pathconf_names) self.check(os.pathconf, "PC_NAME_MAX") self.check(os.fpathconf, "PC_NAME_MAX") self.check_bool(os.pathconf, "PC_NAME_MAX") diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index 35016b83a477fc..f0c26bdcb0922b 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -566,10 +566,38 @@ def test_dup(self): @unittest.skipUnless(hasattr(posix, 'confstr'), 'test needs posix.confstr()') - @unittest.skipIf(support.is_apple_mobile, "gh-118201: Test is flaky on iOS") def test_confstr(self): - self.assertRaises(ValueError, posix.confstr, "CS_garbage") - self.assertEqual(len(posix.confstr("CS_PATH")) > 0, True) + with self.assertRaisesRegex( + ValueError, "unrecognized configuration name" + ): + posix.confstr("CS_garbage") + + with self.assertRaisesRegex( + TypeError, "configuration names must be strings or integers" + ): + posix.confstr(1.23) + + path = posix.confstr("CS_PATH") + self.assertGreater(len(path), 0) + self.assertEqual(posix.confstr(posix.confstr_names["CS_PATH"]), path) + + @unittest.skipUnless(hasattr(posix, 'sysconf'), + 'test needs posix.sysconf()') + def test_sysconf(self): + with self.assertRaisesRegex( + ValueError, "unrecognized configuration name" + ): + posix.sysconf("SC_garbage") + + with self.assertRaisesRegex( + TypeError, "configuration names must be strings or integers" + ): + posix.sysconf(1.23) + + open_max = posix.sysconf("SC_OPEN_MAX") + self.assertGreater(open_max, 0) + self.assertEqual( + posix.sysconf(posix.sysconf_names["SC_OPEN_MAX"]), open_max) @unittest.skipUnless(hasattr(posix, 'dup2'), 'test needs posix.dup2()') diff --git a/Misc/NEWS.d/next/Library/2024-10-28-19-49-18.gh-issue-118201.v41XXh.rst b/Misc/NEWS.d/next/Library/2024-10-28-19-49-18.gh-issue-118201.v41XXh.rst new file mode 100644 index 00000000000000..bed4b3b5956f31 --- /dev/null +++ b/Misc/NEWS.d/next/Library/2024-10-28-19-49-18.gh-issue-118201.v41XXh.rst @@ -0,0 +1,2 @@ +Fixed intermittent failures of :any:`os.confstr`, :any:`os.pathconf` and +:any:`os.sysconf` on iOS and Android. diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index 1857fca736ef20..dffaf14f2da735 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -10047,7 +10047,7 @@ os_fpathconf(PyObject *module, PyObject *const *args, Py_ssize_t nargs) if (fd < 0) { goto exit; } - if (!conv_path_confname(args[1], &name)) { + if (!conv_confname(module, args[1], &name, "pathconf_names")) { goto exit; } _return_value = os_fpathconf_impl(module, fd, name); @@ -10121,7 +10121,7 @@ os_pathconf(PyObject *module, PyObject *const *args, Py_ssize_t nargs, PyObject if (!path_converter(args[0], &path)) { goto exit; } - if (!conv_path_confname(args[1], &name)) { + if (!conv_confname(module, args[1], &name, "pathconf_names")) { goto exit; } _return_value = os_pathconf_impl(module, &path, name); @@ -10159,7 +10159,7 @@ os_confstr(PyObject *module, PyObject *arg) PyObject *return_value = NULL; int name; - if (!conv_confstr_confname(arg, &name)) { + if (!conv_confname(module, arg, &name, "confstr_names")) { goto exit; } return_value = os_confstr_impl(module, name); @@ -10191,7 +10191,7 @@ os_sysconf(PyObject *module, PyObject *arg) int name; long _return_value; - if (!conv_sysconf_confname(arg, &name)) { + if (!conv_confname(module, arg, &name, "sysconf_names")) { goto exit; } _return_value = os_sysconf_impl(module, name); diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index bb5077cc7f0f09..8e9582697f8712 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -13535,46 +13535,33 @@ struct constdef { }; static int -conv_confname(PyObject *arg, int *valuep, struct constdef *table, - size_t tablesize) -{ - if (PyLong_Check(arg)) { - int value = PyLong_AsInt(arg); - if (value == -1 && PyErr_Occurred()) - return 0; - *valuep = value; - return 1; - } - else { - /* look up the value in the table using a binary search */ - size_t lo = 0; - size_t mid; - size_t hi = tablesize; - int cmp; - const char *confname; - if (!PyUnicode_Check(arg)) { - PyErr_SetString(PyExc_TypeError, - "configuration names must be strings or integers"); - return 0; - } - confname = PyUnicode_AsUTF8(arg); - if (confname == NULL) - return 0; - while (lo < hi) { - mid = (lo + hi) / 2; - cmp = strcmp(confname, table[mid].name); - if (cmp < 0) - hi = mid; - else if (cmp > 0) - lo = mid + 1; - else { - *valuep = table[mid].value; - return 1; +conv_confname(PyObject *module, PyObject *arg, int *valuep, const char *tablename) +{ + if (PyUnicode_Check(arg)) { + PyObject *table = PyObject_GetAttrString(module, tablename); + if (table != NULL) { + arg = PyObject_GetItem(table, arg); + if (arg == NULL) { + PyErr_SetString( + PyExc_ValueError, "unrecognized configuration name"); } + Py_DECREF(table); } - PyErr_SetString(PyExc_ValueError, "unrecognized configuration name"); - return 0; + if (PyErr_Occurred()) + return 0; + } else { + Py_INCREF(arg); + } + + if (PyLong_Check(arg)) { + *valuep = PyLong_AsInt(arg); + } else { + PyErr_SetString(PyExc_TypeError, + "configuration names must be strings or integers"); } + + Py_DECREF(arg); + return !PyErr_Occurred(); } @@ -13665,14 +13652,6 @@ static struct constdef posix_constants_pathconf[] = { {"PC_TIMESTAMP_RESOLUTION", _PC_TIMESTAMP_RESOLUTION}, #endif }; - -static int -conv_path_confname(PyObject *arg, int *valuep) -{ - return conv_confname(arg, valuep, posix_constants_pathconf, - sizeof(posix_constants_pathconf) - / sizeof(struct constdef)); -} #endif @@ -13897,14 +13876,6 @@ static struct constdef posix_constants_confstr[] = { #endif }; -static int -conv_confstr_confname(PyObject *arg, int *valuep) -{ - return conv_confname(arg, valuep, posix_constants_confstr, - sizeof(posix_constants_confstr) - / sizeof(struct constdef)); -} - /*[clinic input] os.confstr @@ -14454,14 +14425,6 @@ static struct constdef posix_constants_sysconf[] = { #endif }; -static int -conv_sysconf_confname(PyObject *arg, int *valuep) -{ - return conv_confname(arg, valuep, posix_constants_sysconf, - sizeof(posix_constants_sysconf) - / sizeof(struct constdef)); -} - /*[clinic input] os.sysconf -> long @@ -14486,40 +14449,15 @@ os_sysconf_impl(PyObject *module, int name) #endif /* HAVE_SYSCONF */ -/* This code is used to ensure that the tables of configuration value names - * are in sorted order as required by conv_confname(), and also to build - * the exported dictionaries that are used to publish information about the - * names available on the host platform. - * - * Sorting the table at runtime ensures that the table is properly ordered - * when used, even for platforms we're not able to test on. It also makes - * it easier to add additional entries to the tables. - */ - -static int -cmp_constdefs(const void *v1, const void *v2) -{ - const struct constdef *c1 = - (const struct constdef *) v1; - const struct constdef *c2 = - (const struct constdef *) v2; - - return strcmp(c1->name, c2->name); -} - static int setup_confname_table(struct constdef *table, size_t tablesize, const char *tablename, PyObject *module) { - PyObject *d = NULL; - size_t i; - - qsort(table, tablesize, sizeof(struct constdef), cmp_constdefs); - d = PyDict_New(); + PyObject *d = PyDict_New(); if (d == NULL) return -1; - for (i=0; i < tablesize; ++i) { + for (size_t i=0; i < tablesize; ++i) { PyObject *o = PyLong_FromLong(table[i].value); if (o == NULL || PyDict_SetItemString(d, table[i].name, o) == -1) { Py_XDECREF(o); From 1c2009c6796d8199336e5f0ceaaa77dcafbe4314 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 28 Oct 2024 21:25:50 +0000 Subject: [PATCH 2/5] Use Argument Clinic correctly --- Modules/clinic/posixmodule.c.h | 2 +- Modules/posixmodule.c | 34 +++++++++++++++++++--------------- 2 files changed, 20 insertions(+), 16 deletions(-) diff --git a/Modules/clinic/posixmodule.c.h b/Modules/clinic/posixmodule.c.h index dffaf14f2da735..fb93526445f37e 100644 --- a/Modules/clinic/posixmodule.c.h +++ b/Modules/clinic/posixmodule.c.h @@ -13013,4 +13013,4 @@ os__create_environ(PyObject *module, PyObject *Py_UNUSED(ignored)) #ifndef OS__SUPPORTS_VIRTUAL_TERMINAL_METHODDEF #define OS__SUPPORTS_VIRTUAL_TERMINAL_METHODDEF #endif /* !defined(OS__SUPPORTS_VIRTUAL_TERMINAL_METHODDEF) */ -/*[clinic end generated code: output=9756767bdbdabe94 input=a9049054013a1b77]*/ +/*[clinic end generated code: output=efba633c4191338e input=a9049054013a1b77]*/ diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 8e9582697f8712..f490fcf0ab0577 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -3111,18 +3111,22 @@ class Py_off_t_return_converter(long_return_converter): type = 'Py_off_t' conversion_fn = 'PyLong_FromPy_off_t' -class path_confname_converter(CConverter): +class confname_converter(CConverter): type="int" - converter="conv_path_confname" + converter="conv_confname" -class confstr_confname_converter(path_confname_converter): - converter='conv_confstr_confname' + def converter_init(self, *, table): + self.table = table -class sysconf_confname_converter(path_confname_converter): - converter="conv_sysconf_confname" + def parse_arg(self, argname, displayname, *, limited_capi): + return self.format_code(""" + if (!{converter}(module, {argname}, &{paramname}, "{table}")) {{{{ + goto exit; + }}}} + """, argname=argname, converter=self.converter, table=self.table) [python start generated code]*/ -/*[python end generated code: output=da39a3ee5e6b4b0d input=1860d32584c2a539]*/ +/*[python end generated code: output=da39a3ee5e6b4b0d input=8189d5ae78244626]*/ /*[clinic input] @@ -13660,7 +13664,7 @@ static struct constdef posix_constants_pathconf[] = { os.fpathconf -> long fd: fildes - name: path_confname + name: confname(table="pathconf_names") / Return the configuration limit name for the file descriptor fd. @@ -13670,7 +13674,7 @@ If there is no limit, return -1. static long os_fpathconf_impl(PyObject *module, int fd, int name) -/*[clinic end generated code: output=d5b7042425fc3e21 input=5b8d2471cfaae186]*/ +/*[clinic end generated code: output=d5b7042425fc3e21 input=023d44589c9ed6aa]*/ { long limit; @@ -13688,7 +13692,7 @@ os_fpathconf_impl(PyObject *module, int fd, int name) /*[clinic input] os.pathconf -> long path: path_t(allow_fd='PATH_HAVE_FPATHCONF') - name: path_confname + name: confname(table="pathconf_names") Return the configuration limit name for the file or directory path. @@ -13699,7 +13703,7 @@ On some platforms, path may also be specified as an open file descriptor. static long os_pathconf_impl(PyObject *module, path_t *path, int name) -/*[clinic end generated code: output=5bedee35b293a089 input=bc3e2a985af27e5e]*/ +/*[clinic end generated code: output=5bedee35b293a089 input=6f6072f57b10c787]*/ { long limit; @@ -13880,7 +13884,7 @@ static struct constdef posix_constants_confstr[] = { /*[clinic input] os.confstr - name: confstr_confname + name: confname(table="confstr_names") / Return a string-valued system configuration variable. @@ -13888,7 +13892,7 @@ Return a string-valued system configuration variable. static PyObject * os_confstr_impl(PyObject *module, int name) -/*[clinic end generated code: output=bfb0b1b1e49b9383 input=18fb4d0567242e65]*/ +/*[clinic end generated code: output=bfb0b1b1e49b9383 input=4c6ffca2837ec959]*/ { PyObject *result = NULL; char buffer[255]; @@ -14428,7 +14432,7 @@ static struct constdef posix_constants_sysconf[] = { /*[clinic input] os.sysconf -> long - name: sysconf_confname + name: confname(table="sysconf_names") / Return an integer-valued system configuration variable. @@ -14436,7 +14440,7 @@ Return an integer-valued system configuration variable. static long os_sysconf_impl(PyObject *module, int name) -/*[clinic end generated code: output=3662f945fc0cc756 input=279e3430a33f29e4]*/ +/*[clinic end generated code: output=3662f945fc0cc756 input=930b8f23b5d15086]*/ { long value; From 2a3e50a5587db13af2431bc8811c49acde20c73a Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 28 Oct 2024 22:11:50 +0000 Subject: [PATCH 3/5] Test os.sysconf with a key that's available on WASI --- Lib/test/test_posix.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/Lib/test/test_posix.py b/Lib/test/test_posix.py index f0c26bdcb0922b..237e77e37ea673 100644 --- a/Lib/test/test_posix.py +++ b/Lib/test/test_posix.py @@ -594,10 +594,10 @@ def test_sysconf(self): ): posix.sysconf(1.23) - open_max = posix.sysconf("SC_OPEN_MAX") - self.assertGreater(open_max, 0) + arg_max = posix.sysconf("SC_ARG_MAX") + self.assertGreater(arg_max, 0) self.assertEqual( - posix.sysconf(posix.sysconf_names["SC_OPEN_MAX"]), open_max) + posix.sysconf(posix.sysconf_names["SC_ARG_MAX"]), arg_max) @unittest.skipUnless(hasattr(posix, 'dup2'), 'test needs posix.dup2()') From 97bee9a15424da093318a1989962ad87f6679c0b Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Sat, 9 Nov 2024 23:48:03 +0000 Subject: [PATCH 4/5] Rearrange error handling --- Modules/posixmodule.c | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index f490fcf0ab0577..9ce9960d3151ef 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -13543,18 +13543,19 @@ conv_confname(PyObject *module, PyObject *arg, int *valuep, const char *tablenam { if (PyUnicode_Check(arg)) { PyObject *table = PyObject_GetAttrString(module, tablename); - if (table != NULL) { - arg = PyObject_GetItem(table, arg); - if (arg == NULL) { - PyErr_SetString( - PyExc_ValueError, "unrecognized configuration name"); - } - Py_DECREF(table); + if (table == NULL) { + return 0; } - if (PyErr_Occurred()) + + arg = PyObject_GetItem(table, arg); + Py_DECREF(table); + if (arg == NULL) { + PyErr_SetString( + PyExc_ValueError, "unrecognized configuration name"); return 0; + } } else { - Py_INCREF(arg); + Py_INCREF(arg); // Match the Py_DECREF below. } if (PyLong_Check(arg)) { From 8a026f13455b1f2c6bbc632ae5d3bf984aec0838 Mon Sep 17 00:00:00 2001 From: Malcolm Smith Date: Mon, 18 Nov 2024 15:33:13 +0000 Subject: [PATCH 5/5] More error handling improvements --- Modules/posixmodule.c | 14 +++++++++----- 1 file changed, 9 insertions(+), 5 deletions(-) diff --git a/Modules/posixmodule.c b/Modules/posixmodule.c index 51c9e0ac8fa338..c77bdfee0eacc5 100644 --- a/Modules/posixmodule.c +++ b/Modules/posixmodule.c @@ -13569,15 +13569,19 @@ conv_confname(PyObject *module, PyObject *arg, int *valuep, const char *tablenam Py_INCREF(arg); // Match the Py_DECREF below. } - if (PyLong_Check(arg)) { - *valuep = PyLong_AsInt(arg); - } else { + int success = 0; + if (!PyLong_Check(arg)) { PyErr_SetString(PyExc_TypeError, "configuration names must be strings or integers"); + } else { + int value = PyLong_AsInt(arg); + if (!(value == -1 && PyErr_Occurred())) { + *valuep = value; + success = 1; + } } - Py_DECREF(arg); - return !PyErr_Occurred(); + return success; }