Skip to content

Commit

Permalink
osdSockUnsentCount.c add MSVC version check
Browse files Browse the repository at this point in the history
  • Loading branch information
@mdavidsaver
mdavidsaver committed Oct 25, 2019
1 parent fc04d12 commit d5bc04a
Showing 1 changed file with 1 addition and 1 deletion.
2 changes: 1 addition & 1 deletion modules/libcom/src/osi/os/WIN32/osdSockUnsentCount.c
View file
Expand Up @@ -13,7 +13,7 @@
* See https://docs.microsoft.com/en-us/windows/win32/api/mstcpip/ns-mstcpip-tcp_info_v0
*/
int epicsSocketUnsentCount(SOCKET sock) {
#if defined (_WIN32) && WINVER >= _WIN32_WINNT_WIN10
#if defined (_WIN32) && WINVER >= _WIN32_WINNT_WIN10 && _MSC_VER>=1900
/* Windows 10 Version 1703 / Server 2016 */
DWORD infoVersion = 0, bytesReturned;
TCP_INFO_v0 tcpInfo;
Expand Down

6 comments on commit d5bc04a

@dirk-zimoch
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'#if defined (_WIN32) && WINVER >= _WIN32_WINNT_WIN10' is not sufficient?
Unfortunately I don't have all kinds of old Visual Studio installations available.

@dirk-zimoch
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I would prefer '&& defined(SIO_TCP_INFO)'

@FreddieAkeroyd
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

WINVER / _WIN32_WINNT define the target windows platform for your build, which can be different from what you are building on. I think this build failed on a Windows 2012 R2 server using the appveyor 2015 build image which includes several versions of visual studio from 2015 downwards but also includes the Windows 10 sdk. If you do not specify a target windows version in the header, it defaults to the most recent. So it looks like the build has picked up a windows 10 SDK and is targetting that, but the visual studio version is not supported by this windows sdk. I think Visual Studio 2015 is a minimum for W10, so some check along the lines of Michael's suggestion is appropriate. I don't know if this is an issue with how the things have been installed on the appveyor image, or lack of a check in the SDK headers as "this combination of sdk and visual studio is not supposed to occur"

@FreddieAkeroyd
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW the current _MSC_VER test would exclude mingw, if mingw supports this on W10 then we just need to adjust the check logic a bit.

@mdavidsaver
Copy link
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merged version is 5e6226e which only looks for the existence of SIO_TCP_INFO.

@dirk-zimoch As there is not yet any unit test coverage, and failures are silent, can you verify that I haven't broken anything? It would be excellent if you could create a short test program which connects to itself through localhost and shows that epicsSocketUnsentCount() can return 0?

@dirk-zimoch
Copy link

@dirk-zimoch dirk-zimoch commented on d5bc04a Oct 31, 2019 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Compiles and works on my Win10. I cannot test on older Windows Versions, not even on older Win10 where it may compile but not work because SIO_TCP_INFO is defined but the WSAIoctl call fails with an 'unsupported' error.
How to see it: var logClientDebug 2, then shut down the logServer and then send a message: errlog test
I'll write a test program.

Please sign in to comment.