[ovs-dev] [PATCH 3/4] socket-util: errno for winsock related functions.

[Gurucharan Shetty]

On Thu, Feb 6, 2014 at 4:51 PM, Ben Pfaff <blp at nicira.com> wrote:
> On Thu, Feb 06, 2014 at 08:12:32AM -0800, Gurucharan Shetty wrote:
>> For Windows platform, there are different ways to get error numbers
>> for differnt situations. For C library functions, errno is set like the way
>> they are set for Linux. For Windows API functions, it has to be gotten from
>> GetLastError().  For winsock2, errno has to be obtained from WSAGetLastError().
>>
>> The new functions will have the first users in an upcoming commit.
>>
>> Signed-off-by: Gurucharan Shetty <gshetty at nicira.com>
>
> I'm a little uncomfortable with this two-way transformation.  It seems
> like it will be easy to get it wrong.  Will we need to transform other
> error codes too?
I don't yet see the need for the transformation for other error codes.

> If not, then it might be better to add a function
> like this:
>
>     bool
>     is_eagain(int error)
>     {
>         return (error == EAGAIN || error == EWOULDBLOCK
>     #ifdef WSAEWOULDBLOCK
>                 || error == WASEWOULDBLOCK
>     #endif
>                );
>     }
>
> and then we could try to enforce its use with a Makefile rule.

This looks like the right thing to do. But stream-ssl.c uses EAGAIN
for things other than just to check a return value. So, the makefile
rule got a little tricky.
Since I see stream-ssl.c to be the only file where windows sockets and
POSIX sockets co-exist, I decided to get rid of sock_errno() function
from socket-util.c
and moved it to stream-ssl.c. I also got rid of the two-way
transformation of the error code and instead used a #ifdef directly
after the accept() call. It seems to
be the only place where we need this translation.


>
> I'm inclined to think that, if we want to transform error codes like
> this, then we should define wrappers for all the common functions that
> are likely to need them, something like this:
>
>     int
>     do_connect(int socket, struct sockaddr *addr, socklen_t length)
>     {
>         return connect(socket, addr, length) == -1 ? sock_errno() : 0;
>     }
>
> and again then try to enforce their use via Makefile rule.
>
> Some comments below.
>
>> +/* In Windows platform, errno is not set for socket calls.
>> + * The last error has to be gotten from WSAGetLastError(). */
>> +int
>
> This should say (void) instead of ():
Okay.
>
>> +sock_errno()
>> +{
>> +#ifdef _WIN32
>> +    int error = WSAGetLastError();
>> +
>> +    /* In case of non-blocking sockets, EAGAIN is an error that is checked to
>> +     * retry. To prevent multiple #ifdef's across the code base, set the error
>> +     * as EAGAIN when it is WSAEWOULDBLOCK. */
>> +    if (error == WSAEWOULDBLOCK) {
>> +        error = EAGAIN;
>> +    }
>> +    return error;
>> +#else
>> +    return errno;
>> +#endif
>> +}
>
> Can we make sock_strerror() do something like ovs_strerror()
> internally does, to eliminate the need for the caller to free its
> return value?
Yes. It can be done and v2 will have that change.

>
> Thanks,
>
> Ben.