Re: [RPCRT4] support for RPC TCP servers

[Robert Shearman]

Damjan Jovanovic wrote:
> Changelog:
> - Makes RPCRT4 use Winsock2 instead of native sockets (needed for
>
> event object support)
> - Adds support for TCP (ncacn_ip_tcp protocol) servers

I appreciate your attempt at solving this hard problem, but I don't 
believe this is the right approach. Using winsock2 instead of Unix 
sockets adds overhead in terms of wineserver calls and extra processing 
time to mangle arguments, which we shouldn't need.

The reasoning for this is that at some point I want to use Unix domain 
sockets for the ncalrpc provider, so it should be as fast as possible.

More detailed comments inline:
> --- a/include/ws2tcpip.h	2006-08-28 20:35:33.000000000 +0200
> +++ b/include/ws2tcpip.h	2006-09-16 08:55:45.000000000 +0200
> @@ -137,6 +137,8 @@
>  #define WS_IFF_MULTICAST      0x00000010 /* multicast is supported */
>  #endif /* USE_WS_PREFIX */
>
> +#define INET_ADDRSTRLEN	16
> -
>
> #ifndef USE_WS_PREFIX
>  #define IP_OPTIONS                      1
>  #define IP_HDRINCL                      2

This change looks good. Please submit it separately.
> +static const char *gai_strerror(int errcode)
> +{
> - switch (errcode)
> - {
> - case EAI_AGAIN: return "Temporary failure in name resolution";
> - case EAI_BADFLAGS: return "Bad value for ai_flags";
> - case EAI_FAIL: return "Non-recoverable failure in name resolution";
> - case EAI_FAMILY: return "ai_family not supported";
> - case EAI_MEMORY: return "Memory allocation failure";
> - case EAI_NONAME: return "Name or service not known";
> - case EAI_SERVICE: return "Servname not supported for ai_socktype";
> - case EAI_SOCKTYPE: return "ai_socktype not supported";
> - }
> - return "Unknown error";
>
> +}
> -
>
> +static const char *inet_ntop(int af, const void *src, char *dst, int cnt)
> +{
> - if (af == AF_INET)
> - {
> - unsigned int addr = * (unsigned int*) src;
> - if (cnt < INET_ADDRSTRLEN)
> -
>  return NULL;
> - snprintf(dst, cnt, "%u.%u.%u.%u", (addr >> 24) & 0xFF,
> -
>  (addr >> 16) & 0xFF, (addr >> 8) & 0xFF, addr & 0xFF);
> - return dst;
> - }
> - else
> - return NULL;
>
> +}
> -

These should obviously not be here. I believe gai_strerror is 
implemented as a macro in one of the winsock header files. I think 
inet_ntop should be implemented in ws2_32.dll.
> @@ -506,51 +516,115 @@
>        continue;
>      }
> - if (0>connect(sock, ai_cur->ai_addr, ai_cur->ai_addrlen))
> - if (Connection->server)
> {
> -
>  WARN("connect() failed\n");
> -
>  close(sock);
> -
>  continue;
> -
>  BOOL success = FALSE;
> -
>  if (bind(sock, ai_cur->ai_addr, ai_cur->ai_addrlen) == 0)
> -
>  {
> -
>    if (listen(sock, 5) == 0)
> -
>    {
> -
>      if ((tcpc->event = WSACreateEvent()) != NULL)
> -
>      {
> -
>        if (WSAEventSelect(sock, tcpc->event, FD_ACCEPT) == 0)
> -
>          success = TRUE;
> -
>        else
> -
>          WARN("WSAEventSelect() failed\n");

Using WSAEVENTs is an interesting approach, but I'd rather see Unix 
sockets being used with a thread that select's on the socket and sets a 
Win32 event instead. Also, I think this amount of nesting is confusing 
and could be avoided.
> static int rpcrt4_conn_tcp_read(RpcConnection *Connection,
>                                  void *buffer, unsigned int count)
>  {
>    RpcConnection_tcp *tcpc = (RpcConnection_tcp *) Connection;
> - int r = recv(tcpc->sock, buffer, count, MSG_WAITALL);
> - TRACE("%d %p %u -> %d\n", tcpc->sock, buffer, count, r);
> - return r;
> - int rtotal = 0;
> - int r = 0;
> - do
> - {
> - r = recv(tcpc->sock, buffer, count, 0);
> - if (r > 0)
> -
>  rtotal += r;
> - else
> -
>  break;
> - } while (rtotal < count);
> - if (r < 0)
> - rtotal = r;
> - TRACE("%d %p %u -> %d\n", tcpc->sock, buffer, count, rtotal);
> - return rtotal;
>
> }

Hmmm. Is the MSG_WAITALL flag not supported in winsock? I think that 
should be fixed there.
> static int rpcrt4_conn_tcp_write(RpcConnection *Connection,
>                                   const void *buffer, unsigned int count)
>  {
>    RpcConnection_tcp *tcpc = (RpcConnection_tcp *) Connection;
> - int r = write(tcpc->sock, buffer, count);
> - int r = send(tcpc->sock, buffer, count, 0);
> TRACE("%d %p %u -> %d\n", tcpc->sock, buffer, count, r);
> return r;
>
> }

This is a good change. Please submit this as a separate fix.

Thanks,

[RPCRT4] support for RPC TCP servers

[Jan Zerebecki]

On Fri, Sep 22, 2006 at 12:04:40PM +0100, Robert Shearman wrote:
> Damjan Jovanovic wrote:
> > Changelog:
> > - Makes RPCRT4 use Winsock2 instead of native sockets (needed for
> >
> > event object support)
> > - Adds support for TCP (ncacn_ip_tcp protocol) servers
>
> I appreciate your attempt at solving this hard problem, but I don't 
> believe this is the right approach. Using winsock2 instead of Unix 
> sockets adds overhead in terms of wineserver calls and extra processing 
> time to mangle arguments, which we shouldn't need.

But shouldn't we have the possibility to use winsock so a build
of our RPCRT4 is possible for native (and e.g. ros)?

Jan

[RPCRT4] support for RPC TCP servers

[Jakob Eriksson]

Jan Zerebecki wrote:
> On Fri, Sep 22, 2006 at 12:04:40PM +0100, Robert Shearman wrote:
> > I appreciate your attempt at solving this hard problem, but I don't 
> > believe this is the right approach. Using winsock2 instead of Unix 
> > sockets adds overhead in terms of wineserver calls and extra processing 
> > time to mangle arguments, which we shouldn't need.
>
> But shouldn't we have the possibility to use winsock so a build
> of our RPCRT4 is possible for native (and e.g. ros)?

Exactly my thoughts too, but I didn't dare voice them.

regards,
Jakob

[RPCRT4] support for RPC TCP servers

[Robert Shearman]

Jakob Eriksson wrote:
> Jan Zerebecki wrote:
> > On Fri, Sep 22, 2006 at 12:04:40PM +0100, Robert Shearman wrote:
> > > I appreciate your attempt at solving this hard problem, but I don't 
> > > believe this is the right approach. Using winsock2 instead of Unix 
> > > sockets adds overhead in terms of wineserver calls and extra 
> > > processing time to mangle arguments, which we shouldn't need.
> >
> > But shouldn't we have the possibility to use winsock so a build
> > of our RPCRT4 is possible for native (and e.g. ros)?

Of course. In that case, the fix to add inet_ntop can go in 
include/wine/port.h. Then if someone wants to patch rpcrt4 to 
conditionally include winsock2.h instead of the Unix net headers and 
call WSAStartup that would be fine.
> Exactly my thoughts too, but I didn't dare voice them.

Why is that?