On Fri, Apr 12, 2002 at 01:31:04PM -0700, Aaron Bannert wrote: > On Fri, Apr 12, 2002 at 01:18:19PM -0700, Justin Erenkrantz wrote: > > On Fri, Apr 12, 2002 at 12:29:52PM -0700, 'Aaron Bannert' wrote: > > > Ok, what's the word on this? Is my patch good or can we settle on another > > > way to do this and just be consistent? > > > > Please figure out another way to do it. Your change is wrong. > > I don't care if my change is wrong, this is a bug that needs to be > fixed and it seems to me that we can't agree what the correct solution > is. "Please figure out another way to do it" is not good enough.
You asked if your patch was good. Your change breaks any ap_get_brigade()-using modules. So, yes, we need to find another way (and the we *was* implicit in my earlier post). I have tried to suggest other ways to do it, but you and Ryan don't seem to care to hear them. Fine, but your patch isn't going in the repository. > > My take is that the non-ap_get_brigade() path (i.e. > > ap_get_client_block/mod_cgi) isn't handling the error case when > > ap_get_brigade says there is no data available due to the > > Limit case. > > If we are allowing the non-ap_get_brigade() path then are we > requiring *all* modules that might deal with HTTP to have to > understand Content-length: and chunked encoding? If that is > the case, then we have far more than one bug to deal with. No, they don't. That is all handled by ap_http_filter(). I don't see why any module even needs to know that information. Just call ap_get_brigade. If all is well, you'll see an EOS at the end of the request input. The only problem comes in with how to deal with 100-Continue responses. But, that is notoriously module-specific, and I'm not aware of any modules (save ones I've written) that use 100-Continue. Those modules can be smart enough to handle that on their own. (They can send the 100-Continue with a flush bucket on the output as it reads the request.) > That has nothing to do with my patch. Besides, I'm not even sure what > you are saying. When should and should not (in your opinion) -1 be > returned from ap_get_client_block? That API is broken as there is no way to indicate to the caller of ap_get_client_block that an error has occurred. This is very similar to the prior problems with ap_getline that resulted in the AP_MODE_GETLINE mode to ap_get_brigade. As I said, ap_get_client_block() marks the connection as aborted if an error occurred. I think that whole segment of code is broken. I'm still not clear why you guys are so leery of using buckets on the input side. -- justin
