Re: socket woes

--- In hercules-390@..., "kerravon86" <kerravon86@...> wrote:
I added more debug info (tracking down that porting
error) and got this:

maxfd is 764
fd 0 764 is socket
fd 1 -1 is not socket

so you can see that a -1 is in the array.

That maxfd is very large for here:

* Do the select and save results */
rc = select ( maxfd+1, &sockset, NULL, NULL, NULL );


I think Windows only allows this:

/usr/include/w32api/winsock.h:#define FD_SETSIZE        64


So passing 0 or FD_SETSIZE to select might be more
appropriate?


Regardless, I don't think the problem is directly
there.  Now I could add code like this:

static void SelectSet
(
...
    for (i=0; i < pSet->fd_count && i < FD_SETSIZE; i++)
    {
        /* protect against empty slots */
        if ( pSet->fd_array[i] == -1 ) continue;

to stop it from falling over on the -1, but I don't
think the -1 should be there in the first place.

There isn't a lot of places for it to be here:

void* socket_thread( void* arg )
...
    for (;;)
    {
        /* Set the file descriptors for select */
        FD_ZERO ( &sockset );
        maxfd  = add_socket_devices_to_fd_set (   0,   &sockset );
        SUPPORT_WAKEUP_SOCKDEV_SELECT_VIA_PIPE( maxfd, &sockset );

        /* Do the select and save results */
        rc = select ( maxfd+1, &sockset, NULL, NULL, NULL );

I believe it was added by this:

#define SUPPORT_WAKEUP_SOCKDEV_SELECT_VIA_PIPE( maxfd, prset )
SUPPORT_WAKEUP_SELECT_VIA_PIPE( sysblk.sockrpipe, (maxfd), (prset) )

If that sockrpipe is -1, it should flow through
this logic:

#define SUPPORT_WAKEUP_SELECT_VIA_PIPE( pipe_rfd, maxfd, prset ) \
  FD_SET((pipe_rfd),(prset)); \
  (maxfd)=(maxfd)>(pipe_rfd)?(maxfd):(pipe_rfd)

And get through here too:

DLL_EXPORT void w32_FD_SET( int fd, fd_set* pSet )
{
    SOCKET hSocket;

    if (0
        || socket_is_socket( fd )
        || (SOCKET) -1 == ( hSocket = (SOCKET) _get_osfhandle( fd ) )
    )
        hSocket = (SOCKET) fd;

    ORIGINAL_FD_SET( hSocket, pSet );   // (add HANDLE/SOCKET to specified set)
}

And the -1 would go in here:

#define  ORIGINAL_FD_SET( fd, pSet )                      \
    do                                                    \
    {                                                     \
        unsigned int i;                                   \
        for (i=0; i < ((fd_set*)(pSet))->fd_count; i++)   \
            if (((fd_set*)(pSet))->fd_array[i] == (fd))   \
                break;                                    \
        if (i == ((fd_set*)(pSet))->fd_count              \
            && ((fd_set*)(pSet))->fd_count < FD_SETSIZE)  \
        {                                                 \
            ((fd_set*)(pSet))->fd_array[i] = (fd);        \
            ((fd_set*)(pSet))->fd_count++;                \
        }                                                 \
    }                                                     \
    while (0)

And I'd say there's a race condition with this code
in impl.c:

#if defined( OPTION_WAKEUP_SELECT_VIA_PIPE )
    {
        int fds[2];
        initialize_lock(&sysblk.cnslpipe_lock);
        initialize_lock(&sysblk.sockpipe_lock);
        sysblk.cnslpipe_flag=0;
        sysblk.sockpipe_flag=0;
        VERIFY( create_pipe(fds) >= 0 );
        sysblk.cnslwpipe=fds[1];
        sysblk.cnslrpipe=fds[0];
        VERIFY( create_pipe(fds) >= 0 );
        sysblk.sockwpipe=fds[1];
        sysblk.sockrpipe=fds[0];
    }
#endif // defined( OPTION_WAKEUP_SELECT_VIA_PIPE )

And as a first step towards validation of this
theory, I've added code to inspect the sockrpipe
just above and exit if it's -1.

BFN.  Paul.