Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

gh-127350: Add Py_fopen() function #127821

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Dec 11, 2024

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().


📚 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
Copy link
Member

zooba commented Dec 11, 2024

Do we need Py_fclose as well?

@rruuaanng
Copy link
Contributor

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
Copy link
Member Author

@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
Copy link

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
Copy link
Member Author

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

Is Py_fclose() needed because of the DLL issue?

@zooba
Copy link
Member

zooba commented Dec 12, 2024

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
Copy link
Member Author

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
Copy link
Member

zooba commented Dec 13, 2024

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
Copy link
Member Author

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 Outdated Show resolved Hide resolved
Co-authored-by: Steve Dower <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants