Re: [BUG FIX] Compare returned value by socket() against PGINVALID_SOCKET instead of < 0

On Tue, Apr 1, 2014 at 6:31 AM, Bruce Momjian <bruce(at)momjian(dot)us> wrote:
> I reviewed this patch and you are correct that we are not handling
> socket() and accept() returns properly on Windows. We were doing it
> properly in most place in the backend, but your patch fixes the
> remaining places:
>
> http://msdn.microsoft.com/en-us/library/windows/desktop/ms740516%28v=vs.85%29.aspx
>
> However, libpq doesn't seem to be doing much to handle Windows properly
> in this area. I have adjusted libpq to map socket to -1, but the proper
> fix is to distribute pgsocket and PGINVALID_SOCKET checks throughout the
> libpq code. I am not sure how to handle PQsocket() --- should it still
> return -1?

I think changing PQsocket() can impact all existing applications as it
is mentioned
in docs that "result of -1 indicates that no server connection is
currently open.".
Do you see any compelling need to change return value of PQSocket() after
your patch?

> Having the return value be conditional on the operating
> system is ugly. How much of this should be backpatched?

I think it's okay to back patch all the changes.
Is there any part in patch which you feel is risky to back patch?

> Why aren't we
> getting warnings on Windows about assigning the socket() return value to
> an integer?

I think by default Windows doesn't give warning for such code even at Warning
level 4. I have found one related link:
http://stackoverflow.com/questions/75385/make-vs-compiler-catch-signed-unsigned-assignments

> Updated patch attached.

It seems you have missed to change at below places.

1.
int
pg_foreach_ifaddr(PgIfAddrCallback callback, void *cb_data)
sock = WSASocket(AF_INET, SOCK_DGRAM, 0, 0, 0, 0);
if (sock == SOCKET_ERROR)

2.
pgwin32_waitforsinglesocket(SOCKET s, int what, int timeout)
{
static HANDLE waitevent = INVALID_HANDLE_VALUE;
static SOCKET current_socket = -1;

With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com