Skip to content
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

Added getting IPv4 netmask in Windows 7 and above #874

Merged
merged 2 commits into from
Sep 23, 2016

Conversation

ewedlund
Copy link
Contributor

This adds support for getting the IPv4 netmask on Windows (7 and above). On older versions of Windows, it returns a netmask of value None as before. Tested on Windows 7 and 10 with Python 2.7. I have not managed to build it on Windows XP so not tested there.

@giampaolo
Copy link
Owner

This is great, thanks. I only have one complaint. With this:

#if (_WIN32_WINNT >= 0x0601)  // Windows 7

...if the exe/wheel is compiled with Windows 7 the resulting exe/wheel will not be compatible with win < 7 (e.g. Vista). On Vista, the wheel / exe will "believe" it's running on Windows 7 when it's not. As such you should use some other strategy to determine the Windows version at runtime (a Windows API call).

@ewedlund
Copy link
Contributor Author

Ok, I'll try to find a solution to that.

@ewedlund
Copy link
Contributor Author

Actually, reading your consideration a second time I am not sure if I understand what you mean. Even if you are compiling on let's say Windows 7 for Windows Vista, _WIN32_WINNT should have the value indicating Windows Vista, right?

The check I used #ifdef's for is needed for two reasons:

  • OnLinkPrefixLength is available in IP_ADAPTER_UNICAST_ADDRESS as from Windows Vista
  • ConvertLengthToIpv4Mask is also available as from Windows Vista

(as I side note I realised actually my check was excessive, I should check for >= Vista, not Windows 7)

In order to avoid the preprocessor command, I could call GetProcAddress in order to check whether the function is available at runtime, and I agree that it is a more correct solution, but checking for the presence of the struct member OnLinkPrefixLength at runtime would be more complicated.

So basically, before trying a complicated solution for checking for OnLinkPrefixLength at runtime, I wanted to be sure that it is really needed. I am not very familiar with how the build procedure works, but AFAIK you would not compile for Windows Vista using Windows 7 libriaries? Or am I missing something?

@giampaolo
Copy link
Owner

Actually, reading your consideration a second time I am not sure if I understand what you mean.
Even if you are compiling on let's say Windows 7 for Windows Vista, _WIN32_WINNT should
have the value indicating Windows Vista, right?

No, it won't.
The problem is that the #ifdef check is "pre-processor code": as such it is executed once at the time of compilation and never again. The resulting exe/wheel binary has no #ifdef in it, just the C code block which was defined in the #ifdef block at the moment of compilation. That's why if you produce the exe/wheel on Windows 7 the resulting binary will be different than on Win XP or whatever.
I only discovered this recently, see:
#811 (comment)

as I side note I realised actually my check was excessive, I should check for >= Vista,
not Windows 7

Are you sure? Does this functionality require Win >= Vista?
If so, then your PR should be good because we already assumed the exes/wheel do not work on Windows XP (but compiling from sources is fine).

@ewedlund
Copy link
Contributor Author

Are you sure? Does this functionality require Win >= Vista?

According to
https://msdn.microsoft.com/en-us/library/windows/desktop/aa366066(v=vs.85).aspx
and
https://msdn.microsoft.com/en-us/library/windows/desktop/bb408402(v=vs.85).aspx

it is from Windows Vista or Windows server 2008 of and later. (Windows server 2008 has the same value of _WIN32_WINNT as Vista).

If so, then your PR should be good because we already assumed the exes/wheel do not work on Windows XP (but compiling from sources is fine).

I will fix the version and commit.

@giampaolo
Copy link
Owner

Looking back at this, if the change is Windows Vista + only I think this is good for merging.

@ewedlund
Copy link
Contributor Author

That would be great!

@giampaolo giampaolo merged commit 126d3a5 into giampaolo:master Sep 23, 2016
giampaolo added a commit that referenced this pull request Sep 23, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants