gh-107545: Fix misleading setsockopt error message

[naweiss]

According to Python's documentation, setsockopt can get either int, buffer or None 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 causes PyExc_OverflowError we try the next overloads. Because the buffer overload is last we get type error for the third argument instead of OverflowError (e.g. TypeError: a bytes-like object is required, not 'int').

• Issue: misleading setsockopt error message #107545

[bedevere-bot]

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

[ghost]

All commit authors signed the Contributor License Agreement.

[naweiss]

I choose to solve the issue using a white-list approach rather then a black-list approach since there might be more errors that PyErr_Clear will silently suppress. For example, currently, if the type of the first argument is incorrect you still need to keep checking all of the overloads of setsockopt they will all fail and you get the error message from the buffer overload. when you could report the error immediately after the first overload failure.

Error message for:

import socket

with socket.socket() as s:
    s.setsockopt(socket.SOL_SOCKET, socket.SO_RCVBUF, 2 ** 31)

is now OverflowError: signed integer is greater than maximum

[CharlieZhao95]

Looks good, how about adding some tests for new error messages.

[naweiss]

I added the relevant tests. Any new review comments?

[CharlieZhao95]

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 :)

[python-cla-bot]

All commit authors signed the Contributor License Agreement.

[naweiss]

Since this PR is old, I validated that the issue is still reproducible.
Than rebased the branch onto main, resolved any conflicts, and confirmed that all tests pass.
I also did some minor changes to make the PR files a little cleaner and follow conventions better.

[serhiy-storchaka]

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".

[naweiss]

Thanks for the feedback.
I would be happy for guidance regarding the AF_VSOCK family.
The old code supported only an integer option, is this the expected behavior?

[serhiy-storchaka]

Thank you for your update, @naweiss. Here is the next portion of comments.

[serhiy-storchaka]

The old code supported only an integer option, is this the expected behavior?

I noticed this too. We need to look at the history of the code. Anyway, this is a different issue.

[vstinner]

LGTM.

@serhiy-storchaka: Would you mind to review the latest version of this change?

[serhiy-storchaka]

LGTM. 👍

[serhiy-storchaka]

Thank you for your PR, @naweiss. After merging it, would you mind to create an issue and a PR for unification for
AF_VSOCK?

[miss-islington-app]

Thanks @naweiss for the PR, and @serhiy-storchaka for merging it 🌮🎉.. I'm working now to backport this PR to: 3.14.
🐍🍒⛏🤖

[bedevere-app]

GH-137411 is a backport of this pull request to the 3.14 branch.

[naweiss]

Thank you for your PR, @naweiss. After merging it, would you mind to create an issue and a PR for unification for
AF_VSOCK?

No problem. Does it really make sense to have all overloads for AF_VSOCK?

[serhiy-storchaka]

In general, buffer and None, length would be enough for all cases. But most option values have type int, and it is more convenient to pass just optval instead of optval.to_bytes(4, sys.byteorder, signed=True). It seems that for AF_VSOCK options have 64-bit values, so a new special case was added. But you should always be able to pass a buffer of arbitrary length.

And I think that it will make the code simpler.

[vstinner]

Congrats @naweiss for your change!