bpo-36102: Prepend slash to all POSIX shared memory block names by applio · Pull Request #12036 · python/cpython (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation18 Commits2 Checks0 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
Prepend slash to all POSIX shared memory block names by default to avoid needing to add special handling for systems (i.e. FreeBSD) that require it.
https://bugs.python.org/issue36102
bpo36102: Prepend slash to all POSIX shared memory block names bpo-36102: Prepend slash to all POSIX shared memory block names
| @@ -68,6 +68,7 @@ class SharedMemory: |
|---|
| _buf = None |
| _flags = os.O_RDWR |
| _mode = 0o600 |
| _prepend_leading_slash = True if _USE_POSIX else False |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This could simply be _prepend_leading_slash = _USE_POSIX (or you can simply avoid defining a class attribute for this)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By defining a class attribute, we can permit this to be turned off easily in a simple subclass.
class NoSlashesSharedMemory(SharedMemory):
_prepend_leading_slash = False
Agreed that it could have simply been set equal to _USE_POSIX though I thought this would better emphasize that it is a True for POSIX and False otherwise. Perhaps this emphasis is unnecessary.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you (see my comment later).
| try: |
|---|
| if self._name[0] == "/": |
| reported_name = self._name[1:] |
| except Exception: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like catching exception is unnecessary. Also consider using self._name.startswith("/").
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My concern was the possibility that self._name could be set to something other than a str (especially if it were None). Granted, I have not tested using bytes for the name, but it did cross my mind that it might work, in which case self._name[0] == "/" would still work but self._name.startswith("/") would complain that it was not passed b"/".
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The C module only accepts strings so you'll fail in __init__ (where _name is always set). Don't worry about nasty things users may do with it (not your responsability)
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good. I have removed the try-except and changed to use str.startswith in commit 6f513bd.
I almost can't help but think about nasty things users may do.... the horror... =)
| @@ -198,7 +200,14 @@ def buf(self): |
|---|
| @property |
| def name(self): |
| "Unique name that identifies the shared memory block." |
| return self._name |
| reported_name = self._name |
| if _USE_POSIX and self._prepend_leading_slash: |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
_USE_POSIX seems unnecessary
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added it to parallel what was done in __init__() where the use of self._prepend_leading_slash only appears in the POSIX section and not at all in the Windows section of __init__(). If someone does subclass SharedMemory and chooses to set _prepend_leading_slash to True in that subclass as I suggested in my earlier comment, then when they try their code on Windows it would cause name() to behave oddly if we do not also have the check if _USE_POSIX ... here.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1
| @@ -95,6 +96,7 @@ def __init__(self, name=None, create=False, size=0): |
|---|
| self._name = name |
| break |
| else: |
| name = "/" + name if self._prepend_leading_slash else name |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One thing to consider. This will make it impossible to attach to a "foo" literal (e.g. a memory name which was not created by this class). The _prepend_leading_slash class attribute can be toggled to allow this, therefore I think it should be a public attribute (not worth being documented).
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I'm not sure if this use-case should be supported (and it looks pretty rare anyway). Having this as a settable class attribute is good already IMO. And if there's demand for it we can introduce it as a public property later. Dismiss my suggestion.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. I was worried about needing to clarify that it only impacted POSIX systems and had no effect on Windows. At the same time, I saw value exposing it as a hook for users to control in a subclass (given that the leading slash is really only needed on FreeBSD/NetBSD and otherwise shortens the remaining space for the name). I thought to be cautious and start by making it private but maybe it should be made public?
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IMO it should remain private as per your point about Windows. If there ever will be demand for it we can simply add it as a prepend_leading_slash public property (with setter) so that we won't break code for those few people who in the meantime relied on _prepend_leading_slash.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool -- sounds good.
When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@applio: Please replace # with GH- in the commit message next time. Thanks!