httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Cliff Woolley <cliffwool...@yahoo.com>
Subject Implementing split() on pipe buckets?
Date Sun, 12 Nov 2000 02:01:21 GMT

Can you guys take a look at this and see what you think?  It's an attempt to get my
brain wrapped around the issues with implementing pipe_split().  Basically, I took
pipe_read() and made it into pipe_readn(), where pipe_readn() DOES obey the *len
input length, but behaves otherwise the same as pipe_read().  pipe_read() is now a
wrapper around pipe_readn() that calls it with *len=IOBUFSIZE.

Also included is a cleanup of handling failed malloc()'s, which were before just
noted as XXX's.  (Those fixes should go in even if pipe_split() doesn't... I can
separate them out into their own patch if you want.)  I have a few questions which I
noted inline with new XXX's, and removed any XXX's which I felt no longer applied.

A question I didn't note inline is that maybe pipe_split() should check the length
returned from pipe_readn() against the original length, and if they differ then it
should return APR_EINVAL.  (It currently doesn't do this.)

One thing I noted inline which might deserve more explanation is a question about
the test for (*len > 0).  My understanding is that this inserts a pipe bucket back
into the stream any time the amount of data read is greater than 0 (even if there's
nothing left to read).  If there's nothing left to read, we don't detect that until
after we've gone to all the trouble of inserting a new pipe bucket into the stream,
tried to read from it, and got 0 bytes back.  Is there any way to detect that the
pipe is exhausted the first time around and go ahead and close the pipe if it is? 
Also, isn't it possible for *len to be 0 when block==AP_NONBLOCK_READ even if the
pipe isn't really exhausted yet?  Should the test be for APR_EOF or something?  Or
is it correct as it stands and I'm missing something?

--Cliff


Index: ap_buckets_pipe.c
===================================================================
RCS file: /home/cvspublic/apache-2.0/src/ap/ap_buckets_pipe.c,v
retrieving revision 1.20
diff -u -r1.20 ap_buckets_pipe.c
--- ap_buckets_pipe.c	2000/11/11 04:41:56	1.20
+++ ap_buckets_pipe.c	2000/11/12 01:31:00
@@ -56,9 +56,8 @@
 #include "ap_buckets.h"
 #include <stdlib.h>
 
-/* XXX: We should obey the block flag */
-static apr_status_t pipe_read(ap_bucket *a, const char **str,
-			      apr_size_t *len, ap_read_type block)
+static apr_status_t pipe_readn(ap_bucket *a, const char **str,
+			       apr_size_t *len, ap_read_type block)
 {
     apr_file_t *p = a->data;
     ap_bucket *b;
@@ -71,9 +70,11 @@
         apr_set_pipe_timeout(p, 0);
     }
 
-    buf = malloc(IOBUFSIZE); /* XXX: check for failure? */
+    buf = malloc(*len);
+    if (buf == NULL) {
+        return APR_ENOMEM;
+    }
     *str = buf;
-    *len = IOBUFSIZE;
     rv = apr_read(p, buf, len);
 
     if (block == AP_NONBLOCK_READ) {
@@ -89,11 +90,17 @@
      * Change the current bucket to refer to what we read,
      * even if we read nothing because we hit EOF.
      */
-    ap_bucket_make_heap(a, buf, *len, 0, NULL);  /* XXX: check for failure? */
+    a = ap_bucket_make_heap(a, buf, *len, 0, NULL);
+    if (a == NULL) {
+        *str = NULL;
+        free(buf);
+        return APR_ENOMEM;
+    }
+
     /*
      * If there's more to read we have to keep the rest of the pipe
      * for later.  Otherwise, we'll close the pipe.
-     * XXX: Note that more complicated bucket types that 
+     * Note that more complicated bucket types that 
      * refer to data not in memory and must therefore have a read()
      * function similar to this one should be wary of copying this
      * code because if they have a destroy function they probably
@@ -102,8 +109,12 @@
      * rather than destroying the old one and creating a completely
      * new bucket.
      */
-    if (*len > 0) {
+    if (*len > 0) {  /* XXX: should this check for AP_NONBLOCK_READ? */
         b = ap_bucket_create_pipe(p);
+        if (b == NULL) {
+            apr_close(p);
+            return APR_ENOMEM;
+        }
 	AP_BUCKET_INSERT_AFTER(a, b);
     }
     else {
@@ -112,6 +123,34 @@
     return APR_SUCCESS;
 }
 
+static apr_status_t pipe_read(ap_bucket *a, const char **str,
+			      apr_size_t *len, ap_read_type block)
+{
+    *len = IOBUFSIZE;
+    return pipe_readn(a, str, len, block);
+}
+
+static apr_status_t pipe_split(ap_bucket *a, apr_off_t point)
+{
+    apr_size_t *len;
+    char **str;
+
+    if (point < 0) {
+        return APR_EINVAL;
+    }
+    *len = point;
+    /*
+     * pipe_readn() takes care of the split for us.
+     * its real purpose (to provide str) is just treated
+     * as a side-effect here and str is ignored.  this
+     * is fine since pipe_readn split the pipe into a heap
+     * bucket (containing str) and a pipe bucket (containing
+     * the rest of the pipe after *len bytes) anyway
+     */
+    /* XXX: should this use AP_NONBLOCK_READ or not? */
+    return pipe_readn(a, str, len, AP_NONBLOCK_READ);
+}
+
 AP_DECLARE(ap_bucket *) ap_bucket_make_pipe(ap_bucket *b, apr_file_t *p)
 {
     /*
@@ -144,5 +183,5 @@
     ap_bucket_destroy_notimpl,
     pipe_read,
     ap_bucket_setaside_notimpl,
-    ap_bucket_split_notimpl
+    pipe_split
 };


__________________________________________________
Do You Yahoo!?
Yahoo! Calendar - Get organized for the holidays!
http://calendar.yahoo.com/

Mime
View raw message