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

bpo-26826: Expose copy_file_range in the os module #7255

Merged
merged 6 commits into from May 31, 2019

Conversation

@pablogsal
Copy link
Member

@pablogsal pablogsal commented May 30, 2018

Based on an initial implementation by Marcos Dione (StyXman) .

https://bugs.python.org/issue26826

@pablogsal pablogsal force-pushed the bpo26826 branch 3 times, most recently from a742433 to 966459d May 30, 2018
@pablogsal pablogsal force-pushed the bpo26826 branch 2 times, most recently from e5a2949 to a9625b2 Jun 16, 2018
@pablogsal pablogsal force-pushed the bpo26826 branch 9 times, most recently from d304046 to 76544d4 Jun 23, 2018
Doc/library/os.rst Outdated Show resolved Hide resolved
Lib/test/test_os.py Outdated Show resolved Hide resolved
Modules/posixmodule.c Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
Doc/library/os.rst Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Outdated Show resolved Hide resolved
Modules/posixmodule.c Show resolved Hide resolved
@pablogsal pablogsal requested review from gpshead and giampaolo Sep 12, 2018
@pablogsal pablogsal force-pushed the bpo26826 branch 2 times, most recently from 745524f to 77e1d59 Sep 12, 2018
@pablogsal pablogsal requested a review from vstinner Sep 14, 2018
Lib/test/test_os.py Show resolved Hide resolved
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented May 29, 2019

I ran "autoreconf" to update configure and related files.

@ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented May 29, 2019

@pablogsal Why are you using _Py_BEGIN_SUPPRESS_IPH and _Py_END_SUPPRESS_IPH in os_copy_file_range_impl()? Those macros are completely useless there. They are only needed on MSVC.

Doc/library/os.rst Outdated Show resolved Hide resolved
@pablogsal pablogsal requested a review from giampaolo May 29, 2019
@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented May 29, 2019

@giampaolo Could you make another review of this PR?

configure.ac Outdated Show resolved Hide resolved
configure.ac Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented May 29, 2019

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented May 29, 2019

@gpshead Fixed in 9f4d18c.

Thanks for taking a look :)

The rebase was a bit complex and I missed that when regenerating the autotools files.

@gpshead
Copy link
Member

@gpshead gpshead commented May 29, 2019

Overall this looks good to me, especially for beta1.

From past experience, an issue that might come up in the future: A build could be done on a system where copy_file_range is defined as far as configure is concerned, yet isn't actually available or supported on the system the binary gets run on (old kernel, etc). This usually results in an EINVAL OSError. not a problem for us, but sometimes our buildbots are weird. So don't be surprised if we need to go adjust conditions under which the tests run vs get skipped in the future.

@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented May 29, 2019

From past experience, an issue that might come up in the future: A build could be done on a system where copy_file_range is defined as far as configure is concerned, yet isn't actually available or supported on the system the binary gets run on (old kernel, etc).

I am handling that here:

https://github.com/python/cpython/pull/7255/files#diff-af1a300489e3d8b6bdde858f37fc541eR298

Do you think is enough or more checks should be done around that aspect?

@giampaolo
Copy link
Contributor

@giampaolo giampaolo commented May 30, 2019

I have exposed this syscall in a project I'm working on and I took this route instead:

#undef HAVE_COPY_FILE_RANGE

#if defined(__linux__)
    #if defined(__GLIBC_PREREQ)
        #if __GLIBC_PREREQ(2, 27)
            #include <unistd.h>
            #define HAVE_COPY_FILE_RANGE
        #endif
    #endif
#endif


#ifdef HAVE_COPY_FILE_RANGE
// function definition
#endif

I am not sure but I suspect the above could avoid the ENOSYS scenario. Thoughts?

@pablogsal
Copy link
Member Author

@pablogsal pablogsal commented May 31, 2019

I am not sure but I suspect the above could avoid the ENOSYS scenario. Thoughts?

I am going to land this PR to make sure we do not miss feature freeze. I will leave the issue open so we can discuss how to handle different glibc versions.

Thank you, everyone, for the thorough review! 🎉

@pablogsal pablogsal merged commit aac4d03 into python:master May 31, 2019
7 checks passed
@pablogsal pablogsal deleted the bpo26826 branch May 31, 2019
@vstinner
Copy link
Member

@vstinner vstinner commented Jun 4, 2019

Why you mind to mention the new function at https://docs.python.org/dev/whatsnew/3.8.html#os ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants