Skip to content

bpo-26836: Add os.memfd_create() #13567

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

Merged
merged 10 commits into from
May 29, 2019

Conversation

ZackerySpytz
Copy link
Contributor

@ZackerySpytz ZackerySpytz commented May 25, 2019

Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good work! 👍

Please modify the patch to use the glibc wrapper instead of the syscall interface and to use MFD_CLOEXEC as default value for the new FD. Feel free to use bits and pieces from my pending PR master...tiran:bpo-26836-memfd-seal (Co-authored-by appreciated).

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ZackerySpytz
Copy link
Contributor Author

Thank you, @tiran, for the comments. I will address them soon.

@ZackerySpytz ZackerySpytz marked this pull request as ready for review May 27, 2019 15:33
@ZackerySpytz
Copy link
Contributor Author

MFD_CLOEXEC is now the default flag.

There were some issues with configure. It seems that someone had modified configure.ac without regenerating configure in a recent commit.

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@ZackerySpytz
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@tiran: please review the changes made to this pull request.

@tiran tiran self-assigned this May 29, 2019
Copy link
Member

@tiran tiran left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@tiran tiran merged commit 43fdbd2 into python:master May 29, 2019
@bedevere-bot
Copy link

@tiran: Please replace # with GH- in the commit message next time. Thanks!

@tiran
Copy link
Member

tiran commented May 29, 2019

Argh, I wanted to edit the commit message, too.

Thanks for your hard work. Is there a PR for sealing, too?

@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86 Gentoo Non-Debug with X 3.x has failed when building commit 43fdbd2.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/99/builds/2738) and take a look at the build logs.
  4. Check if the failure is related to this commit (43fdbd2) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/99/builds/2738

Click to see traceback logs
From https://github.com/python/cpython
 * branch                  master     -> FETCH_HEAD
Reset branch 'master'

Objects/unicodeobject.c: In function ‘_PyUnicode_CheckConsistency’:
Objects/unicodeobject.c:459:15: warning: variable ‘data’ set but not used [-Wunused-but-set-variable]
         void *data;
               ^~~~
Python/pystrhex.c: In function ‘_Py_strhex_impl’:
Python/pystrhex.c:18:45: warning: passing argument 1 of ‘PyObject_Size’ discards ‘const’ qualifier from pointer target type [-Wdiscarded-qualifiers]
         Py_ssize_t seplen = PyObject_Length(sep);
                                             ^~~
In file included from ./Include/Python.h:147,
                 from Python/pystrhex.c:3:
./Include/abstract.h:271:48: note: expected ‘PyObject *’ {aka ‘struct _object *’} but argument is of type ‘const PyObject *’ {aka ‘const struct _object *’}
 PyAPI_FUNC(Py_ssize_t) PyObject_Size(PyObject *o);
                                      ~~~~~~~~~~^
Python/pystrhex.c:60:27: warning: comparison of integer expressions of different signedness: ‘unsigned int’ and ‘Py_ssize_t’ {aka ‘const int’} [-Wsign-compare]
     if (abs_bytes_per_sep >= arglen) {
                           ^~
Python/pystrhex.c: In function ‘_Py_strhex_with_sep’:
Python/pystrhex.c:90:29: warning: ‘sep_char’ may be used uninitialized in this function [-Wmaybe-uninitialized]
                 retbuf[j++] = sep_char;
                 ~~~~~~~~~~~~^~~~~~~~~~
Python/pystrhex.c:14:13: note: ‘sep_char’ was declared here
     Py_UCS1 sep_char;
             ^~~~~~~~
Python/pystrhex.c: In function ‘_Py_strhex_bytes_with_sep’:
Python/pystrhex.c:90:29: warning: ‘sep_char’ may be used uninitialized in this function [-Wmaybe-uninitialized]
                 retbuf[j++] = sep_char;
                 ~~~~~~~~~~~~^~~~~~~~~~
Python/pystrhex.c:14:13: note: ‘sep_char’ was declared here
     Py_UCS1 sep_char;
             ^~~~~~~~
In file included from ./Include/Python.h:137,
                 from ./Modules/posixmodule.c:27:
./Modules/posixmodule.c: In function ‘all_ins’:
./Modules/posixmodule.c:14058:33: error: ‘MFD_HUGE_32MB’ undeclared (first use in this function); did you mean ‘MFD_HUGE_2MB’?
     if (PyModule_AddIntMacro(m, MFD_HUGE_32MB)) return -1;
                                 ^~~~~~~~~~~~~
./Include/modsupport.h:139:67: note: in definition of macro ‘PyModule_AddIntMacro’
 #define PyModule_AddIntMacro(m, c) PyModule_AddIntConstant(m, #c, c)
                                                                   ^
./Modules/posixmodule.c:14058:33: note: each undeclared identifier is reported only once for each function it appears in
     if (PyModule_AddIntMacro(m, MFD_HUGE_32MB)) return -1;
                                 ^~~~~~~~~~~~~
./Include/modsupport.h:139:67: note: in definition of macro ‘PyModule_AddIntMacro’
 #define PyModule_AddIntMacro(m, c) PyModule_AddIntConstant(m, #c, c)
                                                                   ^
./Modules/posixmodule.c:14060:33: error: ‘MFD_HUGE_512MB’ undeclared (first use in this function); did you mean ‘MFD_HUGE_512KB’?
     if (PyModule_AddIntMacro(m, MFD_HUGE_512MB)) return -1;
                                 ^~~~~~~~~~~~~~
./Include/modsupport.h:139:67: note: in definition of macro ‘PyModule_AddIntMacro’
 #define PyModule_AddIntMacro(m, c) PyModule_AddIntConstant(m, #c, c)
                                                                   ^
make: *** [Makefile:1865: Modules/posixmodule.o] Error 1

find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
find: ‘build’: No such file or directory
make: [Makefile:1763: clean] Error 1 (ignored)

@tiran
Copy link
Member

tiran commented May 29, 2019

@ZackerySpytz Please see buildbot logs. It looks like some HUGE TABLE constants are not defined on all platforms. You may have to ifdef them all :(

@ZackerySpytz
Copy link
Contributor Author

@tiran Okay, I'll take a look.

DinoV pushed a commit to DinoV/cpython that referenced this pull request Jan 14, 2020
* bpo-26836: Add os.memfd_create()

* Use the glibc wrapper for memfd_create()

Co-Authored-By: Christian Heimes <[email protected]>

* Fix deletions caused by autoreconf.

* Use MFD_CLOEXEC as the default value for *flags*.

* Add memset_s to configure.ac.

* Revert memset_s changes.

* Apply the requested changes.

* Tweak the docs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants