You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Jeff Trawick <tr...@ibm.net> on 2000/06/18 20:49:17 UTC
[PATCH] Re: extra syscalls related to ap_create_pipe()
Here is a new patch, fixing the timeout breakage Ryan described
yesterday. In addition, it fixes a couple of other broken scenarios
with blocking state:
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.
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.
Regarding Ryan's earlier concern with the solution employed:
> The second method has the unfortunate
>problem of not really working with ap_get_os_file well.
I have no clue what he is alluding to. It is true that the caller of
ap_get_os_file() has no way to find out whether or not the pipe is
blocking (other than knowing the operations their code must have
performed on the pipe). This is unchanged from the present code in
CVS.
An existing problem with ap_put_os_file(), described above, was fixed.
> It is also very
>messy IMO.
I feel differently, of course :) It took around 10 lines of code to
implement the state in Unix, and it fixes problems in the current code
as well as with my original patch.
Only Unix code is included in this patch. Once we can agree on this
I'll revisit Win32 and OS/2. I need to regression test the current
Win32 code first anyway to understand what is broke and what is
working and what I think I can fix.
Index: src/lib/apr/file_io/unix/filedup.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/filedup.c,v
retrieving revision 1.22
diff -u -r1.22 filedup.c
--- src/lib/apr/file_io/unix/filedup.c 2000/06/18 02:38:51 1.22
+++ src/lib/apr/file_io/unix/filedup.c 2000/06/18 18:25:56
@@ -84,6 +84,7 @@
#endif
(*new_file)->buffer = ap_palloc(p, APR_FILE_BUFSIZE);
}
+ (*new_file)->blocking = old_file->blocking; /* this is the way dup() works */
ap_register_cleanup((*new_file)->cntxt, (void *)(*new_file), ap_unix_file_cleanup,
ap_null_cleanup);
return APR_SUCCESS;
Index: src/lib/apr/file_io/unix/fileio.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/fileio.h,v
retrieving revision 1.21
diff -u -r1.21 fileio.h
--- src/lib/apr/file_io/unix/fileio.h 2000/06/13 16:30:39 1.21
+++ src/lib/apr/file_io/unix/fileio.h 2000/06/18 18:25:56
@@ -117,6 +117,7 @@
int pipe;
ap_interval_time_t timeout;
int buffered;
+ enum {BLK_UNKNOWN, BLK_OFF, BLK_ON } blocking;
int ungetchar; /* Last char provided by an unget op. (-1 = no char)*/
/* Stuff for buffered mode */
Index: src/lib/apr/file_io/unix/open.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/file_io/unix/open.c,v
retrieving revision 1.56
diff -u -r1.56 open.c
--- src/lib/apr/file_io/unix/open.c 2000/06/17 18:16:10 1.56
+++ src/lib/apr/file_io/unix/open.c 2000/06/18 18:25:56
@@ -109,6 +107,7 @@
(*new)->fname = ap_pstrdup(cont, fname);
+ (*new)->blocking = BLK_ON;
(*new)->buffered = (flag & APR_BUFFERED) > 0;
if ((*new)->buffered) {
@@ -213,6 +212,7 @@
}
(*file)->eof_hit = 0;
(*file)->buffered = 0;
+ (*file)->blocking = BLK_UNKNOWN; /* in case it is a pipe */
(*file)->timeout = -1;
(*file)->filedes = *dafile;
/* buffer already NULL;
@@ -246,6 +246,7 @@
(*thefile)->filedes = STDERR_FILENO;
(*thefile)->cntxt = cont;
(*thefile)->buffered = 0;
+ (*thefile)->blocking = BLK_UNKNOWN;
(*thefile)->fname = NULL;
(*thefile)->eof_hit = 0;
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;
}
@@ -89,12 +91,17 @@
{
if (thepipe->pipe == 1) {
thepipe->timeout = timeout;
+ if (thepipe->blocking != BLK_OFF) { /* blocking or unknown state */
+ return pipenonblock(thepipe);
+ }
return APR_SUCCESS;
}
return APR_EINVAL;
}
-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)
{
int filedes[2];
@@ -108,6 +115,7 @@
(*in)->pipe = 1;
(*in)->fname = ap_pstrdup(cont, "PIPE");
(*in)->buffered = 0;
+ (*in)->blocking = BLK_ON;
(*in)->timeout = -1;
(*in)->ungetchar = -1;
#if APR_HAS_THREADS
@@ -120,14 +128,20 @@
(*out)->pipe = 1;
(*out)->fname = ap_pstrdup(cont, "PIPE");
(*out)->buffered = 0;
+ (*out)->blocking = BLK_ON;
(*out)->timeout = -1;
#if APR_HAS_THREADS
(*in)->thlock = NULL;
#endif
- pipenonblock(*in);
- pipenonblock(*out);
+ if (!input_blocking) {
+ pipenonblock(*in);
+ }
+ if (!output_blocking) {
+ pipenonblock(*out);
+ }
+
return APR_SUCCESS;
}
@@ -171,6 +185,8 @@
#endif /* BEOS_BONE */
#endif /* !BEOS_R5 */
+
+ thepipe->blocking = BLK_ON;
return APR_SUCCESS;
}
Index: src/lib/apr/include/apr_file_io.h
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/include/apr_file_io.h,v
retrieving revision 1.55
diff -u -r1.55 apr_file_io.h
--- src/lib/apr/include/apr_file_io.h 2000/06/17 17:04:17 1.55
+++ src/lib/apr/include/apr_file_io.h 2000/06/18 18:25:57
@@ -541,17 +541,19 @@
/*
-=head1 ap_status_t ap_create_pipe(ap_file_t **in, ap_file_t **out, ap_pool_t *cont)
+=head1 ap_status_t ap_create_pipe(ap_file_t **in, int input_blocking, ap_file_t **out, int output_blocking, ap_pool_t *cont)
B<Create an anonymous pipe.>
arg 1) The file descriptor to use as input to the pipe.
- arg 2) The file descriptor to use as output from the pipe.
- arg 3) The pool to operate on.
+ arg 2) Flag: non-zero if you want the input end of the pipe returned in blocking mode
+ arg 3) The file descriptor to use as output from the pipe.
+ arg 4) Flag: non-zero if you want the output end of the pipe returned in blocking mode
+ arg 5) The pool to operate on.
=cut
*/
-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);
/*
Index: src/lib/apr/test/testpipe.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/test/testpipe.c,v
retrieving revision 1.7
diff -u -r1.7 testpipe.c
--- src/lib/apr/test/testpipe.c 2000/04/15 02:56:50 1.7
+++ src/lib/apr/test/testpipe.c 2000/06/18 18:26:01
@@ -86,7 +86,7 @@
fprintf(stdout, "Testing pipe functions.\n");
fprintf(stdout, "\tCreating pipes.......");
- if (ap_create_pipe(&readp, &writep, context) != APR_SUCCESS) {
+ if (ap_create_pipe(&readp, 1, &writep, 1, context) != APR_SUCCESS) {
perror("Didn't create the pipe");
exit(-1);
}
Index: src/lib/apr/threadproc/unix/proc.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/lib/apr/threadproc/unix/proc.c,v
retrieving revision 1.30
diff -u -r1.30 proc.c
--- src/lib/apr/threadproc/unix/proc.c 2000/06/06 21:45:11 1.30
+++ src/lib/apr/threadproc/unix/proc.c 2000/06/18 18:26:03
@@ -71,50 +71,36 @@
ap_int32_t out, ap_int32_t err)
{
ap_status_t status;
+ int child_blocking, parent_blocking;
+
if (in != 0) {
- if ((status = ap_create_pipe(&attr->child_in, &attr->parent_in,
- attr->cntxt)) != APR_SUCCESS) {
+ 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;
}
@@ -124,7 +110,7 @@
ap_file_t *parent_in)
{
if (attr->child_in == NULL && attr->parent_in == NULL)
- ap_create_pipe(&attr->child_in, &attr->parent_in, attr->cntxt);
+ ap_create_pipe(&attr->child_in, 0, &attr->parent_in, 0, attr->cntxt);
if (child_in != NULL)
ap_dupfile(&attr->child_in, child_in, attr->cntxt);
@@ -140,7 +126,7 @@
ap_file_t *parent_out)
{
if (attr->child_out == NULL && attr->parent_out == NULL)
- ap_create_pipe(&attr->child_out, &attr->parent_out, attr->cntxt);
+ ap_create_pipe(&attr->child_out, 0, &attr->parent_out, 0, attr->cntxt);
if (child_out != NULL)
ap_dupfile(&attr->child_out, child_out, attr->cntxt);
@@ -156,7 +142,7 @@
ap_file_t *parent_err)
{
if (attr->child_err == NULL && attr->parent_err == NULL)
- ap_create_pipe(&attr->child_err, &attr->parent_err, attr->cntxt);
+ ap_create_pipe(&attr->child_err, 0, &attr->parent_err, 0, attr->cntxt);
if (child_err != NULL)
ap_dupfile(&attr->child_err, child_err, attr->cntxt);
Index: src/main/http_log.c
===================================================================
RCS file: /home/cvs/apache-2.0/src/main/http_log.c,v
retrieving revision 1.56
diff -u -r1.56 http_log.c
--- src/main/http_log.c 2000/06/12 23:02:49 1.56
+++ src/main/http_log.c 2000/06/18 18:26:05
@@ -704,13 +704,10 @@
pl->p = p;
pl->program = ap_pstrdup(p, program);
pl->pid = NULL;
- if (ap_create_pipe(&ap_piped_log_read_fd(pl), &ap_piped_log_write_fd(pl), p) != APR_SUCCESS) {
- int save_errno = errno;
- errno = save_errno;
+ if (ap_create_pipe(&ap_piped_log_read_fd(pl), 1,
+ &ap_piped_log_write_fd(pl), 1, p) != APR_SUCCESS) {
return NULL;
}
- ap_block_pipe(ap_piped_log_read_fd(pl));
- ap_block_pipe(ap_piped_log_write_fd(pl));
ap_register_cleanup(p, pl, piped_log_cleanup, piped_log_cleanup_for_exec);
if (piped_log_spawn(pl) == -1) {
int save_errno = errno;
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
Another reason not to unconditionally make pipes non-blocking:
Win9x can't do non-blocking pipes...
Currently, with OS/2*, Unix, and I guess BeOS, ap_create_pipe()
returns handles which work non-blocking.
With WinNT you get blocking handles, but this is easily remedied.
With Win9x, there is no remedy AFAIK other than reimplementing pipes
from the ground up. I think Win9x has the old OS/2 1.x (non-server
limitation of not being able to *create* a named pipe; thus that
avenue is probably blocked.
*I guess there is magic that makes the write output handle (created
by DosOpen()) non-blocking by virtue of the fact that the pipe was
created non-blocking. Not real sure... The input handle is
definitely non-blocking.
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
> > -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.
>
I agree with Ryan here. I prefer the simple API.
Is it important that on some platforms, setting a timeout on a pipe may or may not set the
pipe to non-blocking? Windows NT and Unix will behave the same I believe, that is, setting
a non-zero timeout will set the pipe to non-blocking (unix will use a select to wait for
i/o and Windows will use WaitForSingleObject() or one of the selects available to it).
Other platforms (I have no examples right off hand) may be able to do something like a
setpipeopt to timeout a -blocking- call. This is how socket timeouts on Windows work (i.e,
use setsockopt(RCVTIMEO), et, al.). The Windows socket remains a blocking socket but with
a timeout built in.
Bill
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
> From: "Bill Stoddard" <st...@raleigh.ibm.com>
> Date: Tue, 20 Jun 2000 12:41:18 -0400
>
> > > O.k. I'll keep the ap_create_pipe() signature like it was, make both
> > > ends of the pipe blocking, and add ap_unblock_pipe() for when the
> > > caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
> > > the externalization of the internal function pipenonblock().
> >
> > Yes.
> >
>
> Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> sockets (see Dean's saferead code).
>
> ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
>
> Bill
For the record, either way is o.k. with me. The single function
ap_set_pipe_timeout() is sufficient for playing with timeouts and the
blocking state. (Yes, this is one and the same in the current Unix
implementation, but the app may not view it that way and it may not be
so intertwined in the implementation of timeouts on other platforms.)
If we keep ap_block_pipe(), I think ap_set_pipe_timeout(pipe, -1)
should call ap_block_pipe() under the covers.
If we keep the notion of ap_unblock_pipe(), I think
ap_set_pipe_timeout(pipe, 0) should call ap_unblock_pipe() under the
covers.
Note that some of the implementations of ap_set_pipe_timeout() don't
do the right thing with the blocking state when the timeout is
disabled. I may wait to work on this until *after* the big
all-platform commit of the change to ap_create_pipe().
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> > > Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> > > sockets (see Dean's saferead code).
> > >
> > > ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> > > ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> > > ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
> >
> > This is how the code is supposed to work right now. This requires the
> > fcntl calls that we are trying to get rid of.
>
> I don't understand "this requires the fcntl calls..." statement. I
> would like to get rid
> of ap_block_pipe() and ap_unblock_pipe() and use
> ap_set_pipe_timeout() as documented
> above. This doesn't need extra fcntl calls (provided you maintain the
> knowledge of the
> pipe blocking state in ap_file_t).
First of all, could you please set your line break at less than 76
characters?
Okay, I see how this would work now. I don't have any really strong
opinions one way or tother. :-)
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
> > Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> > sockets (see Dean's saferead code).
> >
> > ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> > ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> > ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
>
> This is how the code is supposed to work right now. This requires the
> fcntl calls that we are trying to get rid of.
>
I don't understand "this requires the fcntl calls..." statement. I would like to get rid
of ap_block_pipe() and ap_unblock_pipe() and use ap_set_pipe_timeout() as documented
above. This doesn't need extra fcntl calls (provided you maintain the knowledge of the
pipe blocking state in ap_file_t).
Bill
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
> sockets (see Dean's saferead code).
>
> ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
> ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
> ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
This is how the code is supposed to work right now. This requires the
fcntl calls that we are trying to get rid of.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Bill Stoddard <st...@raleigh.ibm.com>.
> > O.k. I'll keep the ap_create_pipe() signature like it was, make both
> > ends of the pipe blocking, and add ap_unblock_pipe() for when the
> > caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
> > the externalization of the internal function pipenonblock().
>
> Yes.
>
Do we need ap_unblock_pipe() and ap_block_pipe()? Why not use the same semantics as
sockets (see Dean's saferead code).
ap_set_pipe_timeout(pipe, 0) - sets the pipe to non-blocking w/o a timeout
ap_set_pipe_timeout(pipe, -1) - sets the pipe blocking
ap_set_pipe_timeout(pipe, n>0) - sets a timeout on the pipe.
Bill
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
> > 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.
>
> I see... I think what should happen is that if we're BEOS but not
> BEOS_BONE then pipenonblock() should return APR_ENOTIMPL.
>
> Here is what happens with the current CVS code...
>
> create a pipe:
>
> old BEOS:
> pipe returned in blocking state
> everywhere else
> pipe returned in non-blocking state
Or an error :-)
> set pipe timeout:
> everywhere:
> no error
>
> read from pipe when no data avail:
> old BEOS:
> hang
> everywhere else:
> return after timeout
>
> If pipenonblock() returns APR_ENOTIMPL when called from
> ap_set_pipe_timeout(), ap_set_pipe_timeout() will be able to tell the
> caller that we can't do that.
YES!!!!! Perfect.
> > 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.
>
> O.k. I'll keep the ap_create_pipe() signature like it was, make both
> ends of the pipe blocking, and add ap_unblock_pipe() for when the
> caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
> the externalization of the internal function pipenonblock().
Yes.
> > > + 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);
> > > - }
> > > }
> >
> > 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.
>
> The style will remain the same; we'll just call ap_unblock_pipe() as
> follows:
>
> switch(in) {
> case APR_FULL_BLOCK:
> break;
> case APR_PARENT_BLOCK:
> ap_unblock_pipe(attr->child_in);
> break;
> case APR_CHILD_BLOCK:
> ap_unblock_pipe(attr->parent_in);
> break;
> default:
> ap_unblock_pipe(attr->child_in);
> ap_unblock_pipe(attr->parent_in);
> }
Perfect. With these changes, you have my unequivicable ++1. Please
commit when ready.
Ryan
_______________________________________________________________________________
Ryan Bloom rbb@apache.org
406 29th St.
San Francisco, CA 94131
-------------------------------------------------------------------------------
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by Jeff Trawick <tr...@ibm.net>.
> From: rbb@covalent.net
> Date: Tue, 20 Jun 2000 07:41:21 -0700 (PDT)
>
> > 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.
I see... I think what should happen is that if we're BEOS but not
BEOS_BONE then pipenonblock() should return APR_ENOTIMPL.
Here is what happens with the current CVS code...
create a pipe:
old BEOS:
pipe returned in blocking state
everywhere else
pipe returned in non-blocking state
set pipe timeout:
everywhere:
no error
read from pipe when no data avail:
old BEOS:
hang
everywhere else:
return after timeout
If pipenonblock() returns APR_ENOTIMPL when called from
ap_set_pipe_timeout(), ap_set_pipe_timeout() will be able to tell the
caller that we can't do that.
>
> 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.
O.k. I'll keep the ap_create_pipe() signature like it was, make both
ends of the pipe blocking, and add ap_unblock_pipe() for when the
caller needs a handle to be non-blocking. On Unix, ap_unblock_pipe() is just
the externalization of the internal function pipenonblock().
> > + 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);
> > - }
> > }
>
> 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.
The style will remain the same; we'll just call ap_unblock_pipe() as
follows:
switch(in) {
case APR_FULL_BLOCK:
break;
case APR_PARENT_BLOCK:
ap_unblock_pipe(attr->child_in);
break;
case APR_CHILD_BLOCK:
ap_unblock_pipe(attr->parent_in);
break;
default:
ap_unblock_pipe(attr->child_in);
ap_unblock_pipe(attr->parent_in);
}
Thanks,
Jeff
--
Jeff Trawick | trawick@ibm.net | PGP public key at web site:
http://www.geocities.com/SiliconValley/Park/9289/
Born in Roswell... married an alien...
Re: [PATCH] Re: extra syscalls related to ap_create_pipe()
Posted by rb...@covalent.net.
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
-------------------------------------------------------------------------------