-
-
Notifications
You must be signed in to change notification settings - Fork 32.6k
gh-107545: Fix misleading setsockopt error message #107546
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
Conversation
Most changes to Python require a NEWS entry. Please add it using the blurb_it web app or the blurb command-line tool. |
I choose to solve the issue using a white-list approach rather then a black-list approach since there might be more errors that Error message for: import socket
with socket.socket() as s:
s.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 2 ** 31) is now |
Looks good, how about adding some tests for new error messages. |
Misc/NEWS.d/next/Core and Builtins/2023-08-01-22-30-35.gh-issue-107545.f9i3pQ.rst
Outdated
Show resolved
Hide resolved
I added the relevant tests. Any new review comments? |
Considering this is your first PR in cpython, please be patient! The next step is waiting for the review by core devs. Generally, core developers don't have enough time to review every PR, so you can see there are many PRs that can't be merged into the main. Before they come to review, we just need to keep our PR compliant and wait :) |
f712b65
to
6439014
Compare
6439014
to
167d212
Compare
Since this PR is old, I validated that the issue is still reproducible. |
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.
LGTM, but I think that we can get rid of _testcapi
.
The more user friendly approach is to parse arguments as "iiOO|O"
and then use == Py_None
, PyIndex_Check()
and PyBuffer_Check()
for the third argument. This will allow to generate better error messages like "... should be integer, bytes-like object or None".
Thanks for the feedback. |
Thank you for your update, @naweiss. Here is the next portion of comments. |
I noticed this too. We need to look at the history of the code. Anyway, this is a different issue. |
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.
LGTM.
@serhiy-storchaka: Would you mind to review the latest version of this change?
Misc/NEWS.d/next/Core_and_Builtins/2025-07-11-12-29-09.gh-issue-107545.ipfl7U.rst
Outdated
Show resolved
Hide resolved
…e-107545.ipfl7U.rst Co-authored-by: Victor Stinner <vstinner@python.org>
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.
LGTM. 👍
Thank you for your PR, @naweiss. After merging it, would you mind to create an issue and a PR for unification for |
Thanks @naweiss for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14. |
GH-137411 is a backport of this pull request to the 3.14 branch. |
No problem. Does it really make sense to have all overloads for AF_VSOCK? |
In general, And I think that it will make the code simpler. |
Congrats @naweiss for your change! |
According to Python's documentation,
setsockopt
can get eitherint
,buffer
orNone
as the third argument.The way it is currently implemented is by trying to parse all of the three arguments as ints, if this fails no matter what the reason is we just try the next overload.
Since
2**31
causesPyExc_OverflowError
we try the next overloads. Because thebuffer
overload is last we get type error for the third argument instead ofOverflowError
(e.g.TypeError: a bytes-like object is required, not 'int'
).setsockopt
error message #107545