-
-
Notifications
You must be signed in to change notification settings - Fork 33.7k
gh-142419: Add mmap.set_name method for user custom annotation #142480
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
base: main
Are you sure you want to change the base?
Conversation
Modules/mmapmodule.c
Outdated
| } | ||
| #else | ||
| /* naming not supported on this platform */ | ||
| PyErr_SetString(PyExc_ValueError, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In ssl.c we usually raise a NotImplementedError in such cases. If you always want to raise the same error, I suggest that you document this ValueError.
| result = m.set_name(long_name) | ||
| self.assertIsNone(result) | ||
|
|
||
| # Test name too long |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a test with NUL bytes inside it?
Modules/mmapmodule.c
Outdated
| mmap_mmap_set_name_impl(mmap_object *self, const char *name) | ||
| /*[clinic end generated code: output=1edaf4fd51277760 input=6c7dd91cad205f07]*/ | ||
| { | ||
| const char* prefix = "cpython:mmap:"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| const char* prefix = "cpython:mmap:"; | |
| const char *prefix = "cpython:mmap:"; |
Alternatively you can use a macro and in the printf have MACRO "%s" as a format string instead. Same idea as usages of PRI* macros
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>
Misc/NEWS.d/next/Library/2025-12-10-02-31-43.gh-issue-142419.C8_LES.rst
Outdated
Show resolved
Hide resolved
Modules/mmapmodule.c
Outdated
| "Annotation of mmap is not supported on this platform"); | ||
| return NULL; | ||
| #endif | ||
| Py_RETURN_NONE; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can move this one in the ifdef block.
Co-authored-by: Victor Stinner <vstinner@python.org>
|
|
||
| .. method:: set_name(name, /) | ||
|
|
||
| Annotate the memory map with the given *name* for easier identification |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| Annotate the memory map with the given *name* for easier identification | |
| Annotate the memory mapping with the given *name* for easier identification |
| to Python or if Python is built in :ref:`debug mode <debug-build>` | ||
| The length of *name* must not exceed 67 bytes. | ||
|
|
||
| .. availability:: Linux >= 5.17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| .. availability:: Linux >= 5.17 | |
| .. availability:: Linux >= 5.17 (kernel built with `CONFIG_ANON_VMA_NAME` option) |
| (Contributed by Serhiy Storchaka in :gh:`78502`.) | ||
|
|
||
| * Added the :meth:`mmap.mmap.set_name` method | ||
| to annotate an anonymous memory map |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| to annotate an anonymous memory map | |
| to annotate an anonymous memory mapping |
| self.assertIsNone(result) | ||
|
|
||
| # Test name too long | ||
| too_long_name = 'x' * 80 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| too_long_name = 'x' * 80 | |
| too_long_name = 'x' * 68 |
| return NULL; | ||
| } | ||
| if (self->flags & MAP_ANONYMOUS) { | ||
| char buf[81] = {0, }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that it's useful to initialize the buffer since we are directly writing into it.
| char buf[81] = {0, }; | |
| char buf[81]; |
| /*[clinic end generated code: output=1edaf4fd51277760 input=6c7dd91cad205f07]*/ | ||
| { | ||
| #if defined(MAP_ANONYMOUS) && defined(__linux__) | ||
| const char *prefix = "cpython:mmap:"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it really needed to include mmap: in the name? Why not just:
| const char *prefix = "cpython:mmap:"; | |
| const char *prefix = "cpython:"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
An alternative is to even omit the prefix, and just the name to name.
|
|
||
| # Test name length limit (80 chars including prefix "cpython:mmap:") | ||
| # Prefix is 13 chars, so max name is 67 chars | ||
| long_name = 'x' * 30 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suggest to test the longest valid name here:
| long_name = 'x' * 30 | |
| long_name = 'x' * 67 |
And check for overflow below (68 characters).
📚 Documentation preview 📚: https://cpython-previews--142480.org.readthedocs.build/