httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ryan Morgan <rmor...@covalent.net>
Subject Re: [PATCH] ap_rgetline rewrite
Date Tue, 22 Jan 2002 19:41:59 GMT

Looks great Justin.  The new prototype is much easier to work with.
Protocols other than HTTP can benefit from knowing the the apr_status_t
from the get_brigade and bucket_read from within ap_getline.

This passes my tests, so +1 from me.

-Ryan

On Mon, Jan 21, 2002 at 11:26:05PM -0800, Justin Erenkrantz wrote:
> As discussed before, here is an implementation rewrite of 
> ap_rgetline.  I'd appreciate it if people could review before I 
> consider committing it.  =)  It passes all of the httpd-tests (this
> is why I've been adding AP_MODE_SPECULATIVE).
> 
> The rationale behind this rewrite is to remove the req_cfg->bb
> usage so that no data is stored outside of the filter chain.
> This is just pure ugliness and needs to be eradicated.
> 
> A few notes:
> - ap_rgetline has a brand new prototype.  Since there are only
>   a few callers of ap_rgetline (and all of them are restricted to
>   protocol.c), I think it is worth changing it now.  ap_getline
>   has been modified to present the old API.  I imagine that we 
>   could try to tackle changing ap_getline to be more sane in 
>   another pass.  I'd prefer to do this in increments.
>   (Granted, I could rewrite ap_rgetline without changing the API,
>    but yuck...)
> - We return APR_ENOSPC when we are out of buffer space.  I couldn't
>   think up a better error code.  If you have a better idea, I'm all
>   ears.  I really dislike the len==n means we ran out of space 
>   semantic.  (APR_ENOMEM?)
> - ap_rgetline is now recursive in two cases:
>     - We didn't get a LF from ap_get_brigade
>     - We are doing MIME folding
>   I think the logical flow is now much better, so I think the 
>   potential speed costs of recursion is balanced by the fact 
>   that this code just might be maintainable.  However, this has 
>   two drawbacks:
>     - We have to do an extra malloc/copy on return when we do the
>       allocation (i.e. NULL is passed in).  We don't know what
>       ending size will be, so we have to pass NULL to our "child"
>       so that it can control the memory allocation.  So, we'll have
>       two disjoint buffers that we need to merge together.
>     - I think recursion makes a complete mess out of the xlate code.  
>       I'm not entirely sure that this code works on EBCDIC machines.
>       I don't have access to one, but we could introduce an
>       intermediate call that just does the xlate call on EBCDIC
>       machines (ap_rgetline_real and ap_rgetline).  You'd call
>       ap_rgetline which would do just the following:
>         ap_rgetline_real(s, foo, bar, baz...);
>         ap_xlate_from_ascii(s, what length we read);
> 
>       I wish EBCDIC was hidden by filters.  *sigh*
> 
> I have included the new ap_[r]getline in its entirety (since it's a
> big diff) so that it is easy to review and a complete patch follows.
> 
> Please let me know what you think.  Thanks.  -- justin

Mime
View raw message