gh-127350: Add Py_fopen() function #127821

vstinner · 2024-12-11T12:27:01Z

Rename _Py_fopen_obj() to Py_fopen(). The function now also accepts bytes path on Windows.

Remove the private, undocumented, and untested function _Py_fopen_obj().

Issue: _Py_wfopen no longer exported #127350

📚 Documentation preview 📚: https://cpython-previews--127821.org.readthedocs.build/

Rename _Py_fopen_obj() to Py_fopen(). The function now also accepts bytes path on Windows. Remove the private, undocumented, and untested function _Py_fopen_obj().

zooba · 2024-12-11T17:12:42Z

Do we need Py_fclose as well?

Modules/_ssl.c

rruuaanng · 2024-12-12T14:57:05Z

Do we need Py_fclose as well?

fclose seems to be enough, we don't need to wrap it(Unless we use CloseHandle to close the file).

vstinner · 2024-12-12T15:00:01Z

@zooba:

Do we need Py_fclose as well?

Py_fopen() adds values to fopen(): accept a Python object, make the file descriptor non-inheritable, raise an exception on error.

Py_fclose() would just call fclose() without any additional value? I would prefer to not add it unless we have to, and call directly fclose(). I asked the reporter if Py_fclose() would be needed.

To be honest, I don't understand well the Windows DLL issue. From what I understood, to use a FILE* in Python DLL, it should be open in the Python DLL. Otherwise, things can go wrong if the C library version is not exactly the same in two DLLs. But I don't know if it's safe to call fopen() / Py_fopen() in DLL A and call fclose() in DLL B.

PlanetCNC · 2024-12-12T15:25:01Z

PyRun_AnyFileExFlags has "closeit" parameter which I always used.
But you are correct. Py_fopen() needs Py_fclose().

Thank you guys for doing all this.

vstinner · 2024-12-12T15:50:25Z

But you are correct. Py_fopen() needs Py_fclose().

Is Py_fclose() needed because of the DLL issue?

zooba · 2024-12-12T16:11:26Z

Is Py_fclose() needed because of the DLL issue?

Yes. When you call Py_fopen(), you will get a FILE * from whichever C runtime Python is linked to, which isn't necessarily the same as your own application. So you can only pass it back to the same C runtime - it must only be passed into Python APIs, essentially. So regular fclose might crash (or close a different file), and you need Py_fclose() to make sure it goes back to the right runtime.

vstinner · 2024-12-13T13:21:04Z

Ok, I wanted to make sure that I get the rationale correctly.

I added Py_fclose() function and documented that files opened by Py_fopen() must only be closed by Py_fclose().

zooba · 2024-12-13T14:26:43Z

Your updated documentation seems a bit snarky. The Py_fclose may be needed on any platform depending on your static linking situation.

I see no reason not to just document it as "should be used to close files opened by Py_fopen" and if people manage to use fclose and it works then good for them.

vstinner · 2024-12-13T14:45:39Z

Your updated documentation seems a bit snarky. The Py_fclose may be needed on any platform depending on your static linking situation.

It's not my intent to be snarky. I just tried to explain why calling fclose() directly can cause issues in practice.

I see no reason not to just document it as "should be used to close files opened by Py_fopen" and if people manage to use fclose and it works then good for them.

Ok, let's do that.

Doc/c-api/sys.rst

Co-authored-by: Steve Dower <[email protected]>

vstinner · 2024-12-16T12:58:06Z

I created capi-workgroup/decisions#51: "Add public Py_fopen() and Py_fclose() functions, and remove private _Py_fopen_obj() function".

serhiy-storchaka

LGTM in general. I would like to see more tests. The support of the path-like protocol is inconsistent.

serhiy-storchaka · 2024-12-31T15:22:05Z

Python/fileutils.c

-                                wmode, Py_ARRAY_LENGTH(wmode));
-    if (usize == 0) {
-        PyErr_SetFromWindowsErr(0);
+        wchar_t wmode[10];


TODO: Add tests for mode: NULL, non-decodable (non-ASCII) mode, too long mode (e.g. "rt+, ccs=UTF-8").

I added a test for "too long mode" and non-ASCII mode.

Passing NULL does crash. I don't know how to modify the Argument Clinic code to accept NULL.

You can use "z#" to pass arbitrary sequence of bytes or NULL. In Argument Clinic it is expressed as str(zeroes=True, accept={robuffer, str, NoneType}).

Use the NULLABLE macro for path.

If it crashes, just add a comment # CRASHES with specific call as in other tests as indication that we did not miss such case, but cannot test it.

If it crashes, just add a comment # CRASHES with specific call as in other tests as indication that we did not miss such case, but cannot test it.

Ok, I added a comment.

Lib/test/test_capi/test_file.py

Doc/c-api/sys.rst

Test also non-ASCII mode.

* Skip TESTFN_UNENCODABLE if it's None. * Remove TESTFN_UNDECODABLE test.

Lib/test/test_capi/test_file.py

serhiy-storchaka · 2025-01-06T11:43:41Z

Lib/test/test_capi/test_file.py

+
+        # non-ASCII mode failing with "Invalid argument"
+        with self.assertRaises(OSError):
+            _testcapi.py_fopen(__file__, "\xe9")


"\xe9" is encoded to b'\xc3\xa9'. Please test also with non-UTF-8 bytes. You may get different error on Windows. Actually, it may depend on the locale.

It's not possible to pass non-UTF-8 bytes, PySys_Audit() decodes the mode from UTF-8 in strict mode:

if (PySys_Audit("open", "Osi", path, mode, 0) < 0) { return NULL; }

I don't think that it's worth to "support" non-UTF-8 just for the test, whereas it's rejected anyway by fopen().

serhiy-storchaka · 2025-01-06T11:43:57Z

Python/fileutils.c

-                                wmode, Py_ARRAY_LENGTH(wmode));
-    if (usize == 0) {
-        PyErr_SetFromWindowsErr(0);
+        wchar_t wmode[10];


You can use "z#" to pass arbitrary sequence of bytes or NULL. In Argument Clinic it is expressed as str(zeroes=True, accept={robuffer, str, NoneType}).

Use the NULLABLE macro for path.

If it crashes, just add a comment # CRASHES with specific call as in other tests as indication that we did not miss such case, but cannot test it.

serhiy-storchaka

Yet more test suggestions.

vstinner · 2025-01-06T12:39:02Z

@serhiy-storchaka: I tried to implement all requested tests, but I skipped some of them for practical reasons.

On Windows, the file mode is limited to 10 characters. If it's a problem later, I can allocate memory on the heap and write more complicated code to handle longer modes. In practice, we should be fine with this arbitrary limit.

vstinner · 2025-01-06T13:12:00Z

PR merged, thanks for your reviews!

@serhiy-storchaka: You can write a follow-up PR if you want to extend the code coverage (add more tests).

)

pythongh-127350: Add Py_fopen() function

773c4d9

Rename _Py_fopen_obj() to Py_fopen(). The function now also accepts bytes path on Windows. Remove the private, undocumented, and untested function _Py_fopen_obj().

vstinner requested review from ericsnowcurrently and kumaraditya303 as code owners December 11, 2024 12:27

bedevere-app bot mentioned this pull request Dec 11, 2024

_Py_wfopen no longer exported #127350

Closed

bedevere-app bot added the awaiting core review label Dec 11, 2024

rruuaanng reviewed Dec 12, 2024

View reviewed changes

Modules/_ssl.c Show resolved Hide resolved

Add Py_fclose() function

b80f424

vstinner added 2 commits December 13, 2024 14:37

Merge branch 'main' into py_fopen

14fb249

Replace _Py_fopen_obj() with Py_fopen() in sysmodule.c

7390c5e

Simplify Py_fclose() documentation

26a5a56

Run make clinic

4375566

zooba reviewed Dec 13, 2024

View reviewed changes

Doc/c-api/sys.rst Outdated Show resolved Hide resolved

Update Doc/c-api/sys.rst

a779321

Co-authored-by: Steve Dower <[email protected]>

zooba approved these changes Dec 16, 2024

View reviewed changes

bedevere-app bot added awaiting merge and removed awaiting core review labels Dec 16, 2024

Document Py_fclose() return value

e613374

vstinner mentioned this pull request Dec 16, 2024

Add public Py_fopen() and Py_fclose() functions, and remove private _Py_fopen_obj() function capi-workgroup/decisions#51

Closed

12 tasks

serhiy-storchaka self-requested a review December 31, 2024 15:16

serhiy-storchaka approved these changes Dec 31, 2024

View reviewed changes

kumaraditya303 removed their request for review January 3, 2025 10:17

erlend-aasland reviewed Jan 4, 2025

View reviewed changes

Doc/c-api/sys.rst Outdated Show resolved Hide resolved

Doc/c-api/sys.rst Outdated Show resolved Hide resolved

erlend-aasland reviewed Jan 4, 2025

View reviewed changes

Doc/c-api/sys.rst Outdated Show resolved Hide resolved

vstinner added 9 commits January 6, 2025 11:30

Merge branch 'main' into py_fopen

a908b72

Address some of Serhiy's review

2af4d2b

Address Erlend's review

8e9170e

Add more tests

a8c26b2

Support __fspath__() protocol on Windows

5b52a4e

Test also non-ASCII mode.

Update doc for path-like object

e6a53fe

Keep _Py_fopen_obj() function

1d1e7f4

Fix typo

335c1bc

Fix test on macOS and WASI

8241422

* Skip TESTFN_UNENCODABLE if it's None. * Remove TESTFN_UNDECODABLE test.

vstinner enabled auto-merge (squash) January 6, 2025 11:37

vstinner disabled auto-merge January 6, 2025 11:37

vstinner enabled auto-merge (squash) January 6, 2025 11:38

erlend-aasland approved these changes Jan 6, 2025

View reviewed changes

serhiy-storchaka reviewed Jan 6, 2025

View reviewed changes

vstinner disabled auto-merge January 6, 2025 11:54

vstinner added 3 commits January 6, 2025 13:13

Rewrite the Windows implementation

c91aa82

Use FakePath

0fc6dc1

NULL mode does crash

dda4335

vstinner enabled auto-merge (squash) January 6, 2025 12:36

vstinner merged commit f89e5e2 into python:main Jan 6, 2025
40 of 41 checks passed

vstinner deleted the py_fopen branch January 6, 2025 12:43

bedevere-app bot removed the awaiting merge label Jan 6, 2025

srinivasreddy pushed a commit to srinivasreddy/cpython that referenced this pull request Jan 8, 2025

pythongh-127350: Add Py_fopen() and Py_fclose() functions (python#127821

a6bda81

)

Uh oh!

gh-127350: Add Py_fopen() function #127821

gh-127350: Add Py_fopen() function #127821

Uh oh!

Conversation

vstinner commented Dec 11, 2024 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zooba commented Dec 11, 2024

Uh oh!

Uh oh!

rruuaanng commented Dec 12, 2024

Uh oh!

vstinner commented Dec 12, 2024

Uh oh!

PlanetCNC commented Dec 12, 2024

Uh oh!

vstinner commented Dec 12, 2024

Uh oh!

zooba commented Dec 12, 2024

Uh oh!

vstinner commented Dec 13, 2024

Uh oh!

zooba commented Dec 13, 2024

Uh oh!

vstinner commented Dec 13, 2024

Uh oh!

Uh oh!

vstinner commented Dec 16, 2024

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Dec 31, 2024

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

serhiy-storchaka Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vstinner Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka Jan 6, 2025

Choose a reason for hiding this comment

Uh oh!

serhiy-storchaka left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 6, 2025

Uh oh!

Uh oh!

vstinner commented Jan 6, 2025

Uh oh!

Uh oh!

vstinner commented Dec 11, 2024 •

edited by github-actions bot

Loading

serhiy-storchaka Jan 6, 2025 •

edited

Loading