Re: [PATCH] mi: Make sure fd is non-blocking before select+read.

[Tim Rühsen]

Hi Jose,

On 12.05.20 18:13, Jose E. Marchesi wrote:
> 
> Hi Tim!
> 
>     @@ -166,12 +194,21 @@ static int
>      pk_mi_loop (int fd)
>      {
>        fd_set active_fd_set, read_fd_set;
>     +  int old_flags;
>     +  int ret = 0;
>     
>        FD_ZERO (&active_fd_set);
>        FD_SET (fd, &active_fd_set);
>     
>     +  /* Make sure that fd is non-blocking.
>     +   * From 'man 2 select':
>     +   *  On Linux, select() may report a socket file descriptor as
>     +   *  "ready for reading", while nevertheless a subsequent read blocks." 
> */
>     +  old_flags = pk_mi_fd_set_nonblocking (fd);
> 
> 
> The set_nonblocking/restore_blocking is OK, thanks for noticing this.
> 
>     +
>        pk_mi_exit_p = 0;
>     -  while (1)
>     +
>     +  while (ret != -1 && ret != -2 && pk_mi_exit_p == 0)
> 
> This change in the loop, I don't like.
> 
> Before, the logic involving `ret' was only in one place.
> 
> With the proposed change the logic is in two places, so if we add a new
> `ret' value we may have to update _both_ the `while' condition and the
> if..elseif at the bottom of the function.
> 
> Doesn't sound like an improvement to me...

I agree. The other approach would be to call pk_mi_fd_restore_blocking
before each return. If you like this more, I'll change it.

Just want to mention that proper poke error codes, used by all
functions, would release us from making that suspicious return value
'translations' in the loop.

> 
>          {
>            read_fd_set = active_fd_set;
>            if (select (FD_SETSIZE, &read_fd_set, NULL, NULL,
>     @@ -182,20 +219,23 @@ pk_mi_loop (int fd)
>              }
>     
>            if (FD_ISSET (fd, &read_fd_set))
>     -        {
>     -          int ret = pk_mi_read_from_client (fd);
>     -
>     -          if (pk_mi_exit_p)
>     -            /* Client requested us to close the connection.  */
>     -            return 1;
>     -          else if (ret == -1)
>     -            /* Client closed the connection.  */
>     -            return 1;
>     -          else if (ret == -2)
>     -            /* Protocol error.  */
>     -            return 0;
>     -        }
>     +        ret = pk_mi_read_from_client (fd);
>          }
>     +
>     +  if (pk_mi_exit_p)
>     +    /* Client requested us to close the connection.  */
>     +    ret = 1;
>     +  else if (ret == -1)
>     +    /* Client closed the connection.  */
>     +    ret = 1;
>     +  else if (ret == -2)
>     +    /* Protocol error.  */
>     +    ret = 0;
>     +
>     +  /* Restore blocking mode if needed. */
>     +  pk_mi_fd_restore_blocking (fd, old_flags);
>     +
>     +  return ret;
>      }
>     
>      /* Message sender.  */
>     --
>     2.26.2
>

signature.asc
Description: OpenPGP digital signature