httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From r..@covalent.net
Subject Re: Implementing split() on pipe buckets?
Date Sun, 12 Nov 2000 02:30:21 GMT

I am against pipes and sockets having a split function.  They don't work
the same way that the other buckets work, and they really can't.  This
patch fixes the problem of a pipe split where there is enough data in the
pipe to split at the desired length, because it splits the pipe into a
heap and a pipe bucket at the correct place.  What if there isn't enough
data to do the split at the correct place?  This patch doesn't handle that
case.  All the other bucket types return an error if the split is past the
end of the bucket.

The only logical way to deal with pipe and socket splits is to read the
data and then split it.  The split functions can't do this, because that
changes the definition of split, so it is up to the program itself.  The
code isn't hard to read or understand, if anything it makes the code a bit
clearer about what is happening.

I am also against some of the malloc checks.  In general in Apache, we do
not check for NULL after calling malloc, because if malloc returns NULL
your machine is basically screwed and we can't really fail cleanly anyway.

Ryan

> 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/
> 
> 


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


Mime
View raw message