Re: win32 socket definition

The win64 port has showed that we have two sockets declared
incorrectly. They are supposed to be declared as SOCKET on win32, but
they are declared as int. See attached patch.

Given that SOCKET is actually defined as int on win32 (no warnings or
anything there, just on win64), I'm inclined to apply this patch just
to HEAD and not bother with backpatching.

Comments?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> The win64 port has showed that we have two sockets declared
> incorrectly. They are supposed to be declared as SOCKET on win32, but
> they are declared as int. See attached patch.

> Given that SOCKET is actually defined as int on win32 (no warnings or
> anything there, just on win64), I'm inclined to apply this patch just
> to HEAD and not bother with backpatching.

This looks pretty bletcherous --- plastering #ifdef WIN32 all over the
code is exactly not the way to be fixing this sort of thing. Maybe we
should go the other direction of "typedef int SOCKET" on Unix then use
SOCKET everywhere.

BTW, isn't this porting project showing the shortsightedness of using
WIN32 as the its-Windows platform symbol? The case that you're
worried about here is certainly not "WIN32".

regards, tom lane

On Fri, Jan 1, 2010 at 20:41, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> The win64 port has showed that we have two sockets declared
>> incorrectly. They are supposed to be declared as SOCKET on win32, but
>> they are declared as int. See attached patch.
>
>> Given that SOCKET is actually defined as int on win32 (no warnings or
>> anything there, just on win64), I'm inclined to apply this patch just
>> to HEAD and not bother with backpatching.
>
> This looks pretty bletcherous --- plastering #ifdef WIN32 all over the
> code is exactly not the way to be fixing this sort of thing.  Maybe we
> should go the other direction of "typedef int SOCKET" on Unix then use
> SOCKET everywhere.

Yeah, we can do that - I figured since it was only two places, this
was easier...

In keeping with how we usually name things though, shouldn't it be
pg_socket, and then have it typdef'ed to two different things
depending on which platform you're on?

> BTW, isn't this porting project showing the shortsightedness of using
> WIN32 as the its-Windows platform symbol?  The case that you're
> worried about here is certainly not "WIN32".

Well, the API is actually called "Win32 for 64-bit Windows" these
days. And the vast majority of the APIs are exactly the same. With the
patch that I'm working off now, there are exactly 4 "#ifdef _WIN64"
sections, two of which are in pg_config.h.win32 (which is intended to
deal with such things). The other one is spinlock and shared memory.
Everything else is exactly the same code.

The socket thing specifically, is already wrong on 32-bit win32. It's
just that it didn't throw a warning then.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On fre, 2010-01-01 at 20:25 +0100, Magnus Hagander wrote:
> The win64 port has showed that we have two sockets declared
> incorrectly. They are supposed to be declared as SOCKET on win32, but
> they are declared as int. See attached patch.
>
> Given that SOCKET is actually defined as int on win32 (no warnings or
> anything there, just on win64), I'm inclined to apply this patch just
> to HEAD and not bother with backpatching.

What is SOCKET defined as on win64?

On Sun, Jan 3, 2010 at 17:45, Peter Eisentraut <peter_e(at)gmx(dot)net> wrote:
> On fre, 2010-01-01 at 20:25 +0100, Magnus Hagander wrote:
>> The win64 port has showed that we have two sockets declared
>> incorrectly. They are supposed to be declared as SOCKET on win32, but
>> they are declared as int. See attached patch.
>>
>> Given that SOCKET is actually defined as int on win32 (no warnings or
>> anything there, just on win64), I'm inclined to apply this patch just
>> to HEAD and not bother with backpatching.
>
> What is SOCKET defined as on win64?

The socket definition is the same:
typedef UINT_PTR SOCKET;

But since it's defined as a pointer type, that makes it 64-bit on win64.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Fri, Jan 1, 2010 at 20:55, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Fri, Jan 1, 2010 at 20:41, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>>> The win64 port has showed that we have two sockets declared
>>> incorrectly. They are supposed to be declared as SOCKET on win32, but
>>> they are declared as int. See attached patch.
>>
>>> Given that SOCKET is actually defined as int on win32 (no warnings or
>>> anything there, just on win64), I'm inclined to apply this patch just
>>> to HEAD and not bother with backpatching.
>>
>> This looks pretty bletcherous --- plastering #ifdef WIN32 all over the
>> code is exactly not the way to be fixing this sort of thing.  Maybe we
>> should go the other direction of "typedef int SOCKET" on Unix then use
>> SOCKET everywhere.
>
> Yeah, we can do that - I figured since it was only two places, this
> was easier...
>
> In keeping with how we usually name things though, shouldn't it be
> pg_socket, and then have it typdef'ed to two different things
> depending on which platform you're on?

Something along the line of this?

Is there a good trick to find out if you've touched every place you
need to, because I'm very unsure I have. I don't even get a warning in
more than those two places, but there ought to be some way to trick
the system to tell me?

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> Is there a good trick to find out if you've touched every place you
> need to, because I'm very unsure I have. I don't even get a warning in
> more than those two places, but there ought to be some way to trick
> the system to tell me?

Can't think of one, but you could try grepping for the socket-related
syscalls to see what variables are referenced there.

regards, tom lane

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> + /* socket has a different definition on WIN32 */
> + #ifndef WIN32
> + typedef char pgsocket;
> + #else
> + typedef SOCKET pgsocket;
> + #endif

BTW, I trust the non-windows case should be "int".

regards, tom lane

On Wed, Jan 6, 2010 at 23:17, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> + /* socket has a different definition on WIN32 */
>> + #ifndef WIN32
>> + typedef char pgsocket;
>> + #else
>> + typedef SOCKET pgsocket;
>> + #endif
>
> BTW, I trust the non-windows case should be "int".

Haha, yeah, that was my attempt at producing a warning. Which didn't work :-)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Jan 6, 2010 at 23:17, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> BTW, I trust the non-windows case should be "int".

> Haha, yeah, that was my attempt at producing a warning. Which didn't work :-)

Hmm ... "char *" would provoke warnings, but only in the places that
were using the new definition, which is not what you need to find.

regards, tom lane

On Wed, Jan 6, 2010 at 22:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
> Magnus Hagander <magnus(at)hagander(dot)net> writes:
>> Is there a good trick to find out if you've touched every place you
>> need to, because I'm very unsure I have. I don't even get a warning in
>> more than those two places, but there ought to be some way to trick
>> the system to tell me?
>
> Can't think of one, but you could try grepping for the socket-related
> syscalls to see what variables are referenced there.

Found two more by going over it again that way.

Unless there are objections, I will apply this version tomorrow.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Magnus Hagander <magnus(at)hagander(dot)net> writes:
> On Wed, Jan 6, 2010 at 22:42, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> Can't think of one, but you could try grepping for the socket-related
>> syscalls to see what variables are referenced there.

> Found two more by going over it again that way.

> Unless there are objections, I will apply this version tomorrow.

There's another copy of ListenSocket[] in the BackendParameters struct.
I also wonder about postmaster.c's habit of using -1 for empty slots
in ListenSocket ... how safe is that for Win64?

regards, tom lane

Tom Lane wrote:
> There's another copy of ListenSocket[] in the BackendParameters struct.
> I also wonder about postmaster.c's habit of using -1 for empty slots
> in ListenSocket ... how safe is that for Win64?
>
On Windows, it should be INVALID_SOCKET.

James

On Sun, Jan 10, 2010 at 13:33, James Mansion
<james(at)mansionfamily(dot)plus(dot)com> wrote:
> Tom Lane wrote:
>>
>> There's another copy of ListenSocket[] in the BackendParameters struct.
>> I also wonder about postmaster.c's habit of using -1 for empty slots
>> in ListenSocket ... how safe is that for Win64?
>>
>
> On Windows, it should be INVALID_SOCKET.

Indeed it should, but I think we're Ok anyway. Here's the extract from
the SDK headers.

/*
* This is used instead of -1, since the
* SOCKET type is unsigned.
*/
#define INVALID_SOCKET (SOCKET)(~0)
#define SOCKET_ERROR (-1)

But it might be worthwhile going across all those places and setting
them to PGINVALID_SOCKET, and typedef that as well.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

On Sun, Jan 10, 2010 at 13:44, Magnus Hagander <magnus(at)hagander(dot)net> wrote:
> On Sun, Jan 10, 2010 at 13:33, James Mansion
> <james(at)mansionfamily(dot)plus(dot)com> wrote:
>> Tom Lane wrote:
>>>
>>> There's another copy of ListenSocket[] in the BackendParameters struct.
>>> I also wonder about postmaster.c's habit of using -1 for empty slots
>>> in ListenSocket ... how safe is that for Win64?
>>>
>>
>> On Windows, it should be INVALID_SOCKET.
>
> Indeed it should, but I think we're Ok anyway. Here's the extract from
> the SDK headers.
>
> /*
>  * This is used instead of -1, since the
>  * SOCKET type is unsigned.
>  */
> #define INVALID_SOCKET  (SOCKET)(~0)
> #define SOCKET_ERROR            (-1)
>
>
> But it might be worthwhile going across all those places and setting
> them to PGINVALID_SOCKET, and typedef that as well.

That was pretty easy, provided I didn't miss too many places :-) So I
did that, and applied.

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/