Обсуждение: 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@hagander.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@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.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 HaganderMe: 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@gmx.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Fri, Jan 1, 2010 at 20:55, Magnus Hagander <magnus@hagander.net> wrote: > On Fri, Jan 1, 2010 at 20:41, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> Magnus Hagander <magnus@hagander.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@hagander.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@hagander.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@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
Magnus Hagander <magnus@hagander.net> writes: > On Wed, Jan 6, 2010 at 23:17, Tom Lane <tgl@sss.pgh.pa.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@sss.pgh.pa.us> wrote: > Magnus Hagander <magnus@hagander.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@hagander.net> writes: > On Wed, Jan 6, 2010 at 22:42, Tom Lane <tgl@sss.pgh.pa.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@mansionfamily.plus.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/
On Sun, Jan 10, 2010 at 13:44, Magnus Hagander <magnus@hagander.net> wrote: > On Sun, Jan 10, 2010 at 13:33, James Mansion > <james@mansionfamily.plus.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 HaganderMe: http://www.hagander.net/Work: http://www.redpill-linpro.com/