Re: PATCH: Windows sockets

Mark Mitchell
2005-03-26T17:38:34Z

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