Re: svn commit: r1918258 - /apr/apr/trunk/file_io/unix/pipe.c

On 6/11/24 6:05 PM, [email protected] wrote:
> Author: jorton
> Date: Tue Jun 11 16:05:22 2024
> New Revision: 1918258
> 
> URL: http://svn.apache.org/viewvc?rev=1918258&view=rev
> Log:
> * file_io/unix/pipe.c (file_pipe_create): Use pipe2(,O_NONBLOCK) by
>   default unless APR_FULL_BLOCK was used; unconditionally set the
>   blocking state later. Saves two syscalls per invocation for both
>   APR_READ_BLOCK and APR_WRITE_BLOCK, no [intended] functional change.

I guess it is me being blind , but how do we save two syscalls for the
APR_READ_BLOCK and APR_WRITE_BLOCK case each? With the code before the patch
we started with a blocking pipe and needed to set one end of the pipe to non blocking.
Now we start with a non blocking pipe and need to set the other end to blocking.
Where did I get lost and miss the point?

Regards

Rüdiger

> 
> Github: closes #56
> 
> Modified:
>     apr/apr/trunk/file_io/unix/pipe.c
> 
> Modified: apr/apr/trunk/file_io/unix/pipe.c
> URL: http://svn.apache.org/viewvc/apr/apr/trunk/file_io/unix/pipe.c?rev=1918258&r1=1918257&r2=1918258&view=diff
> ==============================================================================
> --- apr/apr/trunk/file_io/unix/pipe.c (original)
> +++ apr/apr/trunk/file_io/unix/pipe.c Tue Jun 11 16:05:22 2024
> @@ -186,7 +186,10 @@ static apr_status_t file_pipe_create(apr
>      apr_interval_time_t fd_timeout;
>  
>  #ifdef HAVE_PIPE2
> -    if (blocking == APR_FULL_NONBLOCK) {
> +    /* If pipe2() is available, use O_NONBLOCK by default unless
> +     * APR_FULL_BLOCK is requested, then set the individual pipe state
> +     * as desired later - changing the state uses two syscalls. */
> +    if (blocking != APR_FULL_BLOCK) {
>          if (pipe2(filedes, O_NONBLOCK) == -1) {
>              return errno;
>          }
> @@ -239,21 +242,14 @@ static apr_status_t file_pipe_create(apr
>      apr_pool_cleanup_register((*out)->pool, (void *)(*out), apr_unix_file_cleanup,
>                           apr_pool_cleanup_null);
>  
> -    switch (blocking) {
> -    case APR_FULL_BLOCK:
> -        break;
> -    case APR_READ_BLOCK:
> -        apr_file_pipe_timeout_set(*out, 0);
> -        break;
> -    case APR_WRITE_BLOCK:
> -        apr_file_pipe_timeout_set(*in, 0);
> -        break;
> -    default:
> -        /* These are noops for the pipe2() case */
> -        apr_file_pipe_timeout_set(*out, 0);
> -        apr_file_pipe_timeout_set(*in, 0);
> -        break;
> -    }
> +    /* Set each pipe to the desired O_NONBLOCK state, which may be a
> +     * noop depending on whether the pipe2() or pipe() case was
> +     * used. (timeout of -1 == blocking) */
> +    apr_file_pipe_timeout_set(*in, (blocking == APR_FULL_BLOCK
> +                                    || blocking == APR_READ_BLOCK) ? -1 : 0);
> +    apr_file_pipe_timeout_set(*out, (blocking == APR_FULL_BLOCK
> +                                     || blocking == APR_WRITE_BLOCK) ? -1 : 0);
> +
>      return APR_SUCCESS;
>  }
>  
> 
> 
>