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

[Joe Orton]

On Thu, Jun 13, 2024 at 08:35:37AM +0200, Ruediger Pluem wrote:
> 
> 
> On 6/11/24 6:05 PM, jor...@apache.org 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?

No, you got it correct, I obviously got confused counting syscalls at 
some point there, sorry! APR_FULL_BLOCK is the only case where using 
pipe2() saves syscalls, and for every other case it costs three syscalls 
regardless of whether pipe() or pipe2() is used. So r1918258 is a noop. 
I'll revert this, thanks for the review.

Regards, Joe

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

[Ruediger Pluem]

On 6/11/24 6:05 PM, jor...@apache.org 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;
>  }
>  
> 
> 
>