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
