Re: PATCH: Windows sockets

[Mark Mitchell <mark@codesourcery.com>]

Thanks for the quick review.

>    Here is a sumary of the changes in this patch:
> 
>    1. Link with -lws2_32 on MinGW so we can use sockets.
> 
> Can't you just use a standard autoconf check to check for that library
> instead of hardcoding it to be linked in on MinGW?

I could do that -- but I didn't because there are Windows configurations 
that do not want to use Winsock.  For example, we don't want to link in 
this library on Cygwin; we just want to let the Cygwin DLL take care of 
things.

>    2. In defs.h, define WINAPI so that source files know whether they are
>       supposed to use the Windows API.
> 
> This is bad.  Yet another #ifdef WINDHOOS when we really are trying to
> get rid of those.

This is the same problem as above; I could autoconf things for Windows, 
but those autoconf tests would fire under Cygwin as well.

>    3. Move ser_unix_readchar and friends (which were only barely
>       UNIX-specific) into ser-base.c, renaming appropriately.
> 
> Fair enough.
> 
>    4. Add a new member (read_prim) to struct serial_ops, and use it from
>       ser_base_readchar.  The reason for this is that sockets must be
>       read with "recv" on Windows; plain "read" does not work.  So, we
>       need a way to indicate which low-level primitive to use to read
>       from a file descriptor.
> 
> But write(2) does work and you don't need to use send(2).  Oh you do
> but things are not quite symmetric.  Could you fix that?  

I'm not sure what you mean.  Do you mean that you would like to create 
write_prim, and use that from the high-level serial "write" function? 
Sure, I can do that.

 > Isn't it a
> better idea to change the #ifdef WINAPI in a #ifdef HAVE_RECV, add an
> autoconf check for recv(2), and always prefer it over read(2) if
> available for reading from a socket?

I was confused about that.  I thought that using recv would not work as 
well as read on UNIX, but I've done some extra research, and I think 
send/recv should work fine everywhere.  Please consider the change you 
suggest made.

>    5. Make a handful of minor changes to ter-tcp.c to account for
>       differences in the BSD and Windows sockets APIs.
> 
> You mean that Windows doesn't have the proper BSD socket API even
> though Microsoft stole^Wused the BSD TCP stack?

Yes, there are a few minor differences.

> I really think these changes are too pervasive and that you really
> need to create a ser-win32.c that has all the Windows-specific cruft
> in it.

I can do that.

However, after making the changes you suggest above, there will only be 
the following Windows-specific changes:

1. Macro definitions at top-of-file to unify the Windows/UNIX spellings 
of ETIMEDOUT, closesocket, and ioctlsocket.

2. The change top net_open to use a "u_long *" argument as the last 
argument to ioctl, rather than an "int *".  (We could avoid the #ifdef 
with a typedef at the top of the file, if that's better than the way I 
have it now.)

3. The change to net_open to use WSAGetLastError() instead of errno on 
Windows.

4. The change to _initialize_ser_tcp to initialize the sockets library.

To me, it seems a shame to duplicate the entire file (in particular, 
net_open, which is the meat of the file) on account of these 
differences.  That function is mostly platform independent, including 
parsing the host/port specification, setting up the socket, connecting 
the socket, interacting with the UI during the connection delay, etc.

Would you please tell me definitively that you want this copied to 
another file?  I'm perfectly willing to do it, but I want to make sure 
that it's really the right thing before I do.  Or, perhaps you mean that 
you want hooks inserted at all the right places, and overridden in 
ser-win32.c?  I can also do that, but I think it will make the code 
harder to read.  Please let me know.

>    6. Tweak safe_strerror to deal with Windows sockets error codes.
> 
> I'm defenitely not thrilled by this tweak.  You're only changing
> "undocumented" into "winsock".  I presume it helps with debugging this
> stuff, but is it really worth the clutter it adds?

I think so, yes.  Windows strerror never returns NULL.  For unknown 
values, it returns "Unknown error" with no indication of *which* unknown 
error.  These errors are presented to users, so, if for example, a 
socket cannot be connected because the user entered the wrong port, or 
gdbserver is not running, the user would only see "Unknown error".  I 
agree that, from a user-experience point of view, "winsock error 12345" 
is not all that helpful -- but at least there is *some* method for 
figuring out what went wrong.

Thanks,

-- 
Mark Mitchell
CodeSourcery, LLC
mark@codesourcery.com
(916) 791-8304