httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Date Tue, 20 Jun 2000 14:41:21 GMT

I dislike how the patch was executed, but the idea behind it is
sound.  Comments are inline.

> 1) If the caller sets a pipe blocking and later tries to set a
>    timeout, the timeout won't work because we don't realize that the
>    pipe is blocking, and ap_set_pipe_timeout() assumes that the pipe
>    is in non-blocking.

We have a flag in the file record now, so we do know that the pipe is
blocking, and a timeout can't work.  If somebody sets a pipe to block, and
then sets a timeout, we need to set the pipe non-blocking.  If the flag is
set to UNKNOWN, we can treat it like it is blocking, because setting it
twice only costs us an extra syscall.  I am willing to pay that price in
this case.

> 2) If the caller creates the ap_file_t via ap_put_os_file(), APR
>    doesn't know whether or not the pipe is blocking, so
>    ap_set_pipe_timeout() is broken again.

This is what I was talking about with it not working with
ap_get_os_file.  That should have been ap_put_os_file.

> @@ -213,6 +212,7 @@
>      }
>      (*file)->eof_hit = 0;
>      (*file)->buffered = 0;
> +    (*file)->blocking = BLK_UNKNOWN; /* in case it is a pipe */

++1!  This is GREAT!

> Index: src/lib/apr/file_io/unix/pipe.c
> ===================================================================
> RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/pipe.c,v
> retrieving revision 1.34
> diff -u -r1.34 pipe.c
> --- src/lib/apr/file_io/unix/pipe.c	2000/06/17 17:04:16	1.34
> +++ src/lib/apr/file_io/unix/pipe.c	2000/06/18 18:25:56
> @@ -82,6 +82,8 @@
>  #endif /* BEOS_BONE */
>  
>  #endif /* !BEOS */
> +
> +    thepipe->blocking = BLK_OFF;
>      return APR_SUCCESS;
>  }

No good.  This needs to be moved inside the #if...#else...#endif.  There
is a case where BeOS can not set the pipe to non-blocking mode.  This
makes it look like the pipe can have a timeout and it can't.

> -ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont)
> +ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking, 
> +                           ap_file_t **out, int output_blocking, 
> +                           ap_pool_t *cont)

I seriously dislike this new API.  I also do not think it is
necessary.  Create all pipes as blocking.  When somebody specifically sets
a pipe non-blocking or sets a timeout switch it.  What we have done here
is to add complexity to an API that used to be very clean.  The
(input|output)_blocking parameters are also integers, and this leads to
messy-ness later.  See below.

>      fprintf(stdout, "\tCreating pipes.......");
> -    if (ap_create_pipe(&readp, &writep, context) != APR_SUCCESS) {
> +    if (ap_create_pipe(&readp, 1, &writep, 1, context) != APR_SUCCESS) {

What are those two 1's doing there?  I know, and you know, but they are
really not obvious.  Anybody who looks at this code later is going to be
VERY confused.  APR has tried to get rid of most flags by using enums and
#defines instead of just 1, 2, or 3.  Also, as I said above, I do not
think those parameters are necessary.

> +        child_blocking = in == APR_FULL_BLOCK || in == APR_CHILD_BLOCK;
> +        parent_blocking = in == APR_FULL_BLOCK || in == APR_PARENT_BLOCK;
> +
> +        if ((status = ap_create_pipe(&attr->child_in, child_blocking, 
> +                                     &attr->parent_in, parent_blocking, 
> +                                     attr->cntxt)) != APR_SUCCESS) {
>              return status;
>          }
> -        switch (in) {
> -        case APR_FULL_BLOCK:
> -            ap_block_pipe(attr->child_in);
> -            ap_block_pipe(attr->parent_in);
> -        case APR_PARENT_BLOCK:
> -            ap_block_pipe(attr->parent_in);
> -        case APR_CHILD_BLOCK:
> -            ap_block_pipe(attr->child_in);
> -        }
>      } 
>      if (out) {
> -        if ((status = ap_create_pipe(&attr->parent_out, &attr->child_out,

> -                                   attr->cntxt)) != APR_SUCCESS) {
> +        child_blocking = out == APR_FULL_BLOCK || out == APR_CHILD_BLOCK;
> +        parent_blocking = out == APR_FULL_BLOCK || out == APR_PARENT_BLOCK;
> +
> +        if ((status = ap_create_pipe(&attr->parent_out, parent_blocking,
> +                                     &attr->child_out, child_blocking,
> +                                     attr->cntxt)) != APR_SUCCESS) {
>              return status;
>          }
> -        switch (out) {
> -        case APR_FULL_BLOCK:
> -            ap_block_pipe(attr->child_out);
> -            ap_block_pipe(attr->parent_out);
> -        case APR_PARENT_BLOCK:
> -            ap_block_pipe(attr->parent_out);
> -        case APR_CHILD_BLOCK:
> -            ap_block_pipe(attr->child_out);
> -        }
>      } 
>      if (err) {
> -        if ((status = ap_create_pipe(&attr->parent_err, &attr->child_err,

> -                                   attr->cntxt)) != APR_SUCCESS) {
> +        child_blocking = err == APR_FULL_BLOCK || err == APR_CHILD_BLOCK;
> +        parent_blocking = err == APR_FULL_BLOCK || err == APR_PARENT_BLOCK;
> +        if ((status = ap_create_pipe(&attr->parent_err, parent_blocking,
> +                                     &attr->child_err, child_blocking,
> +                                     attr->cntxt)) != APR_SUCCESS) {
>              return status;
>          }
> -        switch (err) {
> -        case APR_FULL_BLOCK:
> -            ap_block_pipe(attr->child_err);
> -            ap_block_pipe(attr->parent_err);
> -        case APR_PARENT_BLOCK:
> -            ap_block_pipe(attr->parent_err);
> -        case APR_CHILD_BLOCK:
> -            ap_block_pipe(attr->child_err);
> -        }
>      } 
>      return APR_SUCCESS;
>  }

This whole section can be pretty much left alone if we remove those two
parameters.  I think that makes the code much easier to read.

Ryan

_______________________________________________________________________________
Ryan Bloom                        	rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------




Mime
View raw message