On Thu, Feb 6, 2014 at 4:51 PM, Ben Pfaff <b...@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 <gshe...@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.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
