httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Jeff Trawick <traw...@ibm.net>
Subject [PATCH] Re: extra syscalls related to ap_create_pipe()
Date Sun, 18 Jun 2000 18:49:17 GMT
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...

Mime
View raw message