bpo-32394: Remove some TCP options on old version Windows. #5523

ghost · 2018-02-04T01:39:45Z

https://bugs.python.org/issue32394

skn78 · 2018-02-04T18:59:35Z

Microsoft site says "GetVersionEx may be altered or unavailable for releases after Windows 8.1. Instead, use the Version Helper functions"
VersionHelpers.h has "IsWindows10CreatorsOrGreater()" function for checking Windows 10 Build 15063+ and has "IsWindows10AnniversaryOrGreater()" function for checking Windows 10 Build 14393+.
It are official functions.

zware

Agreed with @skn78, this should use functions from VersionHelpers.h, not the deprecated GetVersionEx.

bedevere-bot · 2018-02-04T20:06:45Z

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.

ghost · 2018-02-05T04:09:37Z

Earlier versions of Windows 10 SDK don't have IsWindows10CreatorsOrGreater() function, people will get a build failure with those Win 10 SDK.
I'm thinking a workaround, any ideas?

ghost · 2018-02-06T01:01:43Z

Offical VersionHelpers.h doesn't have IsWindows10CreatorsOrGreater() and IsWindows10AnniversaryOrGreater() functions.

The two functions are available in a third part VersionHelpers.h, which using a DDK interface RtlGetVersion(...) to get version infomation.

Now I'm inclined to patch in socket.py again, although sys.getwindowsversion() is also using GetVersionEx(...) in underlying level. At least, it keeps socketmodule.c neat.
A code block like this solves the problem:

if hasattr(sys, 'getwindowsversion'):
    # (new_option, available_build)
    # make sure the list ordered by available_build descendent
    _new_flags = [
        ('TCP_KEEPIDLE',  16299),  # Windows 10 1709
        ('TCP_KEEPINTVL', 16299),
        ('TCP_KEEPCNT',   15063),  # Windows 10 1703
        ('TCP_FASTOPEN',  14393)   # Windows 10 1607
        ]
    _WIN_MAJOR, _WIN_MINOR, _WIN_BUILD, *_ = sys.getwindowsversion()
    
    if _WIN_MAJOR == 10 and _WIN_MINOR == 0:
        for _flag, _build in _new_flags:
            if _WIN_BUILD < _build:
                if hasattr(_socket, _flag):
                    delattr(_socket, _flag)
            else:
                break
    elif _WIN_MAJOR < 10:
        for _flag, _ in _new_flags:
            if hasattr(_socket, _flag):
                delattr(_socket, _flag)

ghost · 2018-02-11T00:23:55Z

This PR is prepared for review.

Official VersionHelpers.h uses VerifyVersionInfo(...) function in the inner side.
I'm also using VerifyVersionInfo(...), but made it aware of BuildNumber.

This PR is conflicts with 3.6 branch code.
PR 5585 is prepared for 3.6 branch, besides code conflict, the only difference is that PR 5585 can't recognize the two flags (TCP_KEEPIDLE,TCP_KEEPINTVL) added in 1709 SDK.

Comment addressed; not qualified for full review.

ghost · 2018-02-13T03:01:56Z

@zware
Forgive me for my verdant of GitHub.
This morning I pushed a minor commit, after that, I found there was a message:

zware dismissed their stale review 9 hours ago
Comment addressed; not qualified for full review.

But I can't found any comment, this make me feel very embarrassed.
So if you had addressed a comment, could you please post it again?

zware · 2018-02-13T22:27:53Z

Look just above the message from @bedevere-bot for my old review comment, which was just formalization of the comment left by @skn78 and which you have addressed already.

I'm afraid that's about the extent of my ability to review this one, though.

ghost · 2018-02-14T09:30:21Z

ok

zooba

There's nothing really wrong here, just a few suggestions and some code-quality items that we really can't let through (I don't think we have linting for them, and we probably can't add it easily at this stage, but newer projects would simply forbid mis-named methods).

zooba · 2018-02-19T23:25:48Z

Lib/test/test_socket.py

+                       'TCP_KEEPINTVL'
+                       }
+
+    def testNewTCPFlags(self):


Please use words_with_underscores style for new test names.

zooba · 2018-02-19T23:28:47Z

Lib/test/test_socket.py

+                   "see issue32394.\n"
+                   "Full avaliable TCP flags on this system: %s"
+                    ) % (unknown, provided)
+            raise Exception(msg)


This can be as simple as:

self.assertEqual([], unknown, "New TCP flags were discovered. See bpo-32394 for more information")

zooba · 2018-02-19T23:30:44Z

Modules/socketmodule.c

+        info.dwBuildNumber = flags[i].build_number;
+        /* greater than or equal to the specified version? 
+           Compatibility Mode will not cheat VerifyVersionInfo(...) */
+        if (VerifyVersionInfo(


The IsWindowsVersionOrGreater function should always be available, which will simplify this call.

IsWindowsVersionOrGreater is only aware of MajorVersion, MinorVersion, ServicePackMajor. While we need to compare BuildNumber.

IsWindowsVersionOrGreater uses VerifyVersionInfo in the inner side, I'm also using this function, but made it aware of MajorVersion, MinorVersion, BuildNumber, these three fields fit in with our task.

To make sure it works, I have tested here in several ways:

change build_number in the array.

change info.dwMajorVersion and info.dwMinorVersion.

operate setsockopt(), getsockopt() under Compatibility Mode.

upset the order of macro VER_SET_CONDITION().

zooba · 2018-02-19T23:32:23Z

Modules/socketmodule.c

@@ -305,6 +305,69 @@ if_indextoname(index) -- return the corresponding interface name\n\
 /* Provides the IsWindows7SP1OrGreater() function */
 #include <VersionHelpers.h>

+/* This block removes some flags on older version Windows during run-time.
+   https://msdn.microsoft.com/en-us/library/windows/desktop/ms738596.aspx */
+#if 1


No need for the #if 1 (I assume this is already within an MS_WINDOWS block)

I thought some IDEs can fold this code block, I removed it.

zooba · 2018-02-19T23:34:16Z

Modules/socketmodule.c

+} FlagRuntimeInfo;
+
+/* IMPORTANT: make sure the list ordered by descending build_number */
+static FlagRuntimeInfo flags[] = {


This is a big module - using a name more like tcp_flag_windows_version will avoid conflicts with other parts.

zooba · 2018-02-19T23:36:03Z

Modules/socketmodule.c

@@ -7883,5 +7946,10 @@ PyInit__socket(void)
 #if defined(USE_GETHOSTBYNAME_LOCK) || defined(USE_GETADDRINFO_LOCK)
    netdb_lock = PyThread_allocate_lock();
 #endif
+
+#ifdef MS_WINDOWS
+    return remove_unusable_flags(m);


Make this remove_unusable_flags(m) and then leave the return outside the if block (it's easier to reason about an optional action than a potential transformation, and the same object is always returned)

ghost · 2018-02-20T03:51:40Z

I have modified, sorry for my unskillful patch.

miss-islington · 2018-02-26T18:10:39Z

Thanks @animalize for the PR, and @zooba for merging it 🌮🎉.. I'm working now to backport this PR to: 3.6, 3.7.
🐍🍒⛏🤖

bedevere-bot · 2018-02-26T18:11:19Z

GH-5910 is a backport of this pull request to the 3.7 branch.

…5523) (cherry picked from commit 19e7d48) Co-authored-by: animalize <animalize@users.noreply.github.com>

miss-islington · 2018-02-26T18:11:44Z

Sorry, @animalize and @zooba, I could not cleanly backport this to 3.6 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 19e7d48ce89422091f9af93038b9fee075d46e9e 3.6

bedevere-bot · 2018-02-26T18:12:27Z

GH-5585 is a backport of this pull request to the 3.6 branch.

zooba · 2018-02-26T18:13:33Z

Thanks @animalize!

(cherry picked from commit 19e7d48) Co-authored-by: animalize <animalize@users.noreply.github.com>

wjssz added 2 commits February 4, 2018 09:24

bpo-32394: Remove some TCP options on old version Windows.

b042cb6

improve

e1d5b76

the-knights-who-say-ni added the CLA signed label Feb 4, 2018

bedevere-bot added the awaiting review label Feb 4, 2018

wjssz added 4 commits February 4, 2018 10:04

fix the 1 file(s) with whitespace issues

c320518

use sys.platform rather than os.name

dbce66a

trigger bot

c937594

print full options

5bc8392

zware previously requested changes Feb 4, 2018

View reviewed changes

bedevere-bot added awaiting changes and removed awaiting review labels Feb 4, 2018

wjssz added 10 commits February 10, 2018 13:20

C code

ca1b46b

doc, test

1133a46

0

e8d0b44

0

17cdba5

trailing whitespace

3e1b7f4

trailing whitespaces

b54a21d

trigger bot

1f3c4ca

0

800173f

use VerifyVersionInfo

47270a1

0

e956890

minor

88731af

zooba requested changes Feb 19, 2018

View reviewed changes

wjssz added 2 commits February 20, 2018 08:59

For Changes requested

f43db09

trigger BOT

bbd0979

remove_unusable_flags no return value

17d7403

zooba approved these changes Feb 26, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels Feb 26, 2018

zooba added needs backport to 3.6 labels Feb 26, 2018

zooba merged commit 19e7d48 into python:master Feb 26, 2018

bedevere-bot removed the awaiting merge label Feb 26, 2018

bedevere-bot removed the needs backport to 3.7 label Feb 26, 2018

bedevere-bot removed the needs backport to 3.6 label Feb 26, 2018

miss-islington added a commit that referenced this pull request Feb 26, 2018

bpo-32394: Remove some TCP options on old version Windows. (GH-5523)

53d3f8a

(cherry picked from commit 19e7d48) Co-authored-by: animalize <animalize@users.noreply.github.com>

ghost deleted the issue32394 branch February 27, 2018 09:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-32394: Remove some TCP options on old version Windows. #5523

bpo-32394: Remove some TCP options on old version Windows. #5523

ghost commented Feb 4, 2018 •

edited by bedevere-bot

Loading

skn78 commented Feb 4, 2018

zware left a comment

bedevere-bot commented Feb 4, 2018

ghost commented Feb 5, 2018 •

edited by ghost

Loading

ghost commented Feb 6, 2018 •

edited by ghost

Loading

ghost commented Feb 11, 2018

ghost commented Feb 13, 2018

zware commented Feb 13, 2018

ghost commented Feb 14, 2018

zooba left a comment

zooba Feb 19, 2018

zooba Feb 19, 2018

zooba Feb 19, 2018

ghost Feb 20, 2018

ghost Feb 20, 2018 •

edited by ghost

Loading

zooba Feb 19, 2018

ghost Feb 20, 2018

zooba Feb 19, 2018

zooba Feb 19, 2018

ghost commented Feb 20, 2018

miss-islington commented Feb 26, 2018

bedevere-bot commented Feb 26, 2018

miss-islington commented Feb 26, 2018

bedevere-bot commented Feb 26, 2018

zooba commented Feb 26, 2018

bpo-32394: Remove some TCP options on old version Windows. #5523

bpo-32394: Remove some TCP options on old version Windows. #5523

Conversation

ghost commented Feb 4, 2018 • edited by bedevere-bot Loading

skn78 commented Feb 4, 2018

zware left a comment

Choose a reason for hiding this comment

bedevere-bot commented Feb 4, 2018

ghost commented Feb 5, 2018 • edited by ghost Loading

ghost commented Feb 6, 2018 • edited by ghost Loading

ghost commented Feb 11, 2018

ghost commented Feb 13, 2018

zware commented Feb 13, 2018

ghost commented Feb 14, 2018

zooba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost Feb 20, 2018 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ghost commented Feb 20, 2018

miss-islington commented Feb 26, 2018

bedevere-bot commented Feb 26, 2018

miss-islington commented Feb 26, 2018

bedevere-bot commented Feb 26, 2018

zooba commented Feb 26, 2018

ghost commented Feb 4, 2018 •

edited by bedevere-bot

Loading

ghost commented Feb 5, 2018 •

edited by ghost

Loading

ghost commented Feb 6, 2018 •

edited by ghost

Loading

ghost Feb 20, 2018 •

edited by ghost

Loading