networkupstools / nut

The Network UPS Tools repository. UPS management protocol Informational RFC 9271 published by IETF at https://www.rfc-editor.org/info/rfc9271 Please star NUT on GitHub, this helps with sponsorships!
https://networkupstools.org/
Other
1.92k stars 345 forks source link

Revise use of struct pollfd, sock_fd and ups->fd (UPSCONN_t::fd) for WIN32 branch #1592

Open jimklimov opened 2 years ago

jimklimov commented 2 years ago

According to https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-ioctlsocket, https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-connect, https://docs.microsoft.com/en-us/windows/win32/api/winsock2/nf-winsock2-closesocket etc. type of sock_fd gotta be SOCKET in WIN32 builds, so TYPE_FD_SOCK (and related macros) for us., using closesocket() instead of close(), etc.

However clients/upsclient.c apparently gets away with int (and doesn't include winsock*.h either, at least not explicitly - common.h pulls that into most of our sources, though). Maybe our wincompat.h does wonders, or the code is broken.

NOTE: This is about UPSCONN_t defined in clients/upsclient.h, which is different from server/upstype.h defined upstype_t which does use TYPE_FD already.

jimklimov commented 2 years ago

The sktconnect(), sktclose(), sktread(), sktwrite() defined in wincompat.h seem to abstract this platform nuance to behave as expected with same code (and data types) as for POSIX code. At least, porting a variant of those methods to nutclient.cpp to address #1598 helped make the C++ client library functional (passes NIT).

jimklimov commented 2 years ago

Conversely, check apcupsd-ups.c (and struct pollfd->fd involved) whether they needed changes for TYPE_FD_SOCK or can do without? At least, that code also uses wincompat.h and so should have the tweaked socket() implementation, it seems.

UPDATE: Or maybe not - not without #define W32_NETWORK_CALL_OVERRIDE in play. This may be a point of interest for upsd.c however - it has the overrides and yet its build failures prompted introduction of TYPE_FD_SOCK in the first place. And "as is" at the moment, it works well enough to pass integration tests...