Re: cvs commit: httpd-2.0/modules/dav/fs repos.c
On Thu, Jan 24, 2002 at 03:23:58PM -, [EMAIL PROTECTED] wrote:
> wrowe 02/01/24 07:23:58
>
> Modified:modules/dav/fs repos.c
> Log:
> Cause dav_fs_get_parent_resource to fail if the file path is entirely
> invalid or incomplete, or if it is root [determined by the platform's
> apr implemention]. Identified by Greg Stein.
Well, it was also intended to mean "root URI path". There is no parent for
http://example.com/. Similarly, there is no parent for
http://example.com/foo/bar/ when that Location maps to C:/
(yes, the latter case has a parent, but mod_dav_fs and Apache aren't quite
sophisticated enough to figure it out)
>...
> --- repos.c 23 Jan 2002 20:55:10 - 1.58
> +++ repos.c 24 Jan 2002 15:23:58 - 1.59
> @@ -735,10 +735,18 @@
>dav_resource *parent_resource;
>apr_status_t rv;
>char *dirpath;
> +char *testroot;
> +char *testpath;
>
> -/* If given resource is root, then there is no parent */
> -if (strcmp(resource->uri, "/") == 0 ||
> -ap_os_is_path_absolute(ctx->pool, ctx->pathname)) {
That first strcmp() is needed for the URI part of the test.
I'll fix it.
Thanks for the patches, Bill!
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
On Thu, Jan 24, 2002 at 08:42:05AM -0600, William A. Rowe, Jr. wrote:
> From: "Greg Stein" <[EMAIL PROTECTED]>
> Sent: Thursday, January 24, 2002 4:09 AM
>...
> > Looking at the usage of 'finfo', this could be APR_FINFO_SIZE
>
> Yup, I had limited time ... so I simply made it work. That patch can definately
> be optimized based on the context of the apr_file_info_get and lstat changes!
Understood. I just happened to see it during the code review :-)
>...
> > > -#ifndef WIN32
> > >{
> > >dav_lock_discovery *ld;
> > >dav_lock_indirect *id;
> > > @@ -1071,7 +1078,6 @@
> > >return err;
> > >}
> > >}
> > > -#endif
> >
> > Hmm. Debatable change. It could end up removing a lock record, then putting
> > it right back. I don't think it is a real problem, tho.
>
> Goal 1 of APR, module code doesn't use #if PLAT fooness, or it will discover
> itself borked in future ports :) Is there a trivial patch to discover [or cache]
> the fact that the platform won't support the feature?
The WIN32 block was based on "win32 doesn't have inode/dev". When a platform
doesn't have that, then all locks are stored using pathnames. If a platform
*does* have inode/dev, then it uses them for most of the locks. However, you
can have a lock on a file that doesn't exist, so there is no inode/dev, so
those locks use the pathname. When an inode/dev arrives, then the above
#ifndef/#endif code is needed to convert a lock from the pathname style to
the inode/dev style.
So... the test is whether you have an inode/dev for a given file.
The above code removes the pathname-based lock, then computes a new key, and
saves the lock. If the platform does not support inode/dev, then the key
recomputation will come up with the same thing. No biggy. But if the
platform *does* support it, then it does the right thing.
[ as I said in the post: a bit of extra work, but it should still function
properly ]
>...
> Reviewing ... yup. This needs an apr_filepath_root() call, and test if we
> have any remaining path info [if so, it isn't the root.] apr_filepath_root
> slurps off the leading '/' in unix, the leading dir in win32/os2, and
> //mach/share on win32 (os2?) and mach:/share on netware.
>
> Correct? I'll fix.
Sounds good. One comment coming up on that patch, tho.
>...
> > If the device doesn't exist or doesn't match, then it just does a copy.
>
> My question was; can this be a tristate (can_rename, try_rename, must_copy)
> for platforms that can't tell us in advance about .dev? It's preferable
> to fail-over in the try_rename case, I would expect.
That would certainly be possible. However, once you introduce the failover,
then you wouldn't ever have can_rename. Thus, your logic would be:
* have device, if they match: try_rename
* have device, if they don't match: must_copy
* no device: try_rename
The complication arises in try_rename: how do you detect a failure due to
cross-device move/rename, as opposed to a real failure?
Maybe you just ignore most failures on a rename, and try the copy? Only give
a "real" failure if the copy fails, too?
>...
> > And one more to put back.
>
> So these work on OS2/Netware today? If so, I agree. If not, dav is broken,
> and adding a list of more platforms is the wrong solution, IMHO.
Oh, probably not :-) But we went from "working on Unix and Win32" to
"working on Unix". Thus, my complaint...
[ "working" in the sense tha the property was (correctly) present on unix,
and (correctly) absent on win32. ]
>...
> > *dirpath_p is not set in the 'else' branch.
>
> In the else case, the path is either APR_EBADPATH [meaning *dirpath_p
> isn't split into file + path, so *dirpath_p is returned to the caller
> as-given]
*dirpath_p was always set in the prior code, so "as-given" is meaningless.
> or APR_EINCOMPLETE [again *dirpath_p is returned as-was]
same.
> since the some had been too agressive in splitting off names (there
> is no meaning to //server without a successive /share/ element, together
> they form the root path).
Understood (the prior code was bad).
Just pointing out that the new code doesn't have the same invariant. That is
bad.
Ideally, the code should return an error. I can fix that...
>...
> So *dirpath_p can/should retain it's identity if ap_make_dirstr_parent really
> never had a 'child' object?
We should produce an error. That function *must* be supplied a filename. The
function says " file in directory." We then do some other work
in the directory.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
another one... Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
oops. missed this in my first response:
On Wed, Jan 23, 2002 at 06:28:06PM -, [EMAIL PROTECTED] wrote:
>...
> --- repos.c 14 Jan 2002 13:43:24 - 1.56
> +++ repos.c 23 Jan 2002 18:28:05 - 1.57
> @@ -244,17 +244,43 @@
>*fname_p = NULL;
>}
>else {
> +const char *testpath, *rootpath;
>char *dirpath = ap_make_dirstr_parent(ctx->pool, ctx->pathname);
>apr_size_t dirlen = strlen(dirpath);
> +apr_status_t rv = APR_SUCCESS;
>
> -if (fname_p != NULL)
> -*fname_p = ctx->pathname + dirlen;
> -*dirpath_p = dirpath;
Note that *dirpath_p was _always_ set.
> -
> -/* remove trailing slash from dirpath, unless it's the root dir */
> -/* ### Win32 check */
> -if (dirlen > 1 && dirpath[dirlen - 1] == '/') {
> -dirpath[dirlen - 1] = '\0';
> +testpath = dirpath;
> +if (dirlen > 0) {
> +rv = apr_filepath_root(&rootpath, &testpath, 0, ctx->pool);
> +}
> +
> +/* remove trailing slash from dirpath, unless it's a root path
> + */
> +if ((rv == APR_SUCCESS && testpath && *testpath)
> +|| rv == APR_ERELATIVE) {
> +if (dirpath[dirlen - 1] == '/') {
> +dirpath[dirlen - 1] = '\0';
> +}
> +}
> +
> +/* ###: Looks like a response could be appropriate
> + *
> + * APR_SUCCESS here tells us the dir is a root
> + * APR_ERELATIVE told us we had no root (ok)
> + * APR_EINCOMPLETE an incomplete testpath told us
> + * there was no -file- name here!
> + * APR_EBADPATHor other errors tell us this file
> + * path is undecipherable
> + */
> +
> +if (rv == APR_SUCCESS || rv == APR_ERELATIVE) {
> +*dirpath_p = dirpath;
> +if (fname_p != NULL)
> +*fname_p = ctx->pathname + dirlen;
> +}
> +else {
> +if (fname_p != NULL)
> +*fname_p = NULL;
>}
*dirpath_p is not set in the 'else' branch.
Also note that no callers expect those values to be set to NULL. Given that
the paths have been processed by Apache already, it's a good bet they are
always valid.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
release procedures (was: Re: Tagging .31 soon)
On Wed, Jan 23, 2002 at 09:50:40AM -0800, Ian Holsman wrote: >... > I think I'll just tag .31 and not roll it. If you have no intention of rolling a .31, then don't use that for the tag. A person can tag any time, but that must be with an intent to roll and release it. I think it is inappropriate to use Apache version numbers for what amounts to personal tags. Note that you also have a showstopper listed for a release. (in STATUS and/or at least with wrowe's veto). However, I believe the rule is more like, "the tarball can be released, but the *quality* is subject to consensus approval." In other words, it seems that you should be able to release the tarball, but would need to call it "alpha" or "development", but the group would shoot down things like "beta" or "final". [ this last part I'm a bit fuzzy on, so would like commentary from the *real* old-timers... ] Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/dav/fs lock.c repos.c
On Wed, Jan 23, 2002 at 06:28:06PM -, [EMAIL PROTECTED] wrote:
>...
> @@ -837,7 +844,8 @@
> return NULL;
>}
>
> -if (apr_file_info_get(&finfo, APR_FINFO_NORM, file) != APR_SUCCESS) {
> +rv = apr_file_info_get(&finfo, APR_FINFO_NORM, file);
Looking at the usage of 'finfo', this could be APR_FINFO_SIZE
>...
> @@ -1042,7 +1050,6 @@
> return err;
>}
>
> -#ifndef WIN32
>{
> dav_lock_discovery *ld;
> dav_lock_indirect *id;
> @@ -1071,7 +1078,6 @@
> return err;
>}
>}
> -#endif
Hmm. Debatable change. It could end up removing a lock record, then putting
it right back. I don't think it is a real problem, tho.
>...
> --- repos.c 14 Jan 2002 13:43:24 - 1.56
> +++ repos.c 23 Jan 2002 18:28:05 - 1.57
>...
> @@ -705,16 +733,12 @@
>dav_resource_private *ctx = resource->info;
>dav_resource_private *parent_ctx;
>dav_resource *parent_resource;
> +apr_status_t rv;
>char *dirpath;
>
>/* If given resource is root, then there is no parent */
>if (strcmp(resource->uri, "/") == 0 ||
> -#ifdef WIN32
> -(strlen(ctx->pathname) == 3 && ctx->pathname[1] == ':' &&
>ctx->pathname[2] == '/')
> -#else
> -strcmp(ctx->pathname, "/") == 0
> -#endif
> - ) {
> +ap_os_is_path_absolute(ctx->pool, ctx->pathname)) {
Bad and wrong.
The former tested for "is this the root". ap_os_is_path_absolute() is not
the same test. The end result is that none of the resources will have
parents, which is going to *completely* break the lock tests.
>...
> @@ -1171,13 +1195,21 @@
>else {
> const char *dirpath;
> apr_finfo_t finfo;
> +apr_status_t rv;
>
> /* destination does not exist, but the parent directory should,
>* so try it
>*/
> dirpath = ap_make_dirstr_parent(dstinfo->pool, dstinfo->pathname);
> - if (apr_stat(&finfo, dirpath, APR_FINFO_NORM, dstinfo->pool) == 0
> - && finfo.device == srcinfo->finfo.device) {
> +/*
> + * XXX: If missing dev ... then what test?
> + * Really need a try and failover for those platforms.
> + *
> + */
> +rv = apr_stat(&finfo, dirpath, APR_FINFO_DEV, dstinfo->pool);
> + if ((rv == APR_SUCCESS || rv == APR_INCOMPLETE)
> +&& (finfo.valid & srcinfo->finfo.valid & APR_FINFO_DEV)
> + && (finfo.device == srcinfo->finfo.device)) {
> can_rename = 1;
If the device doesn't exist or doesn't match, then it just does a copy.
>...
> @@ -1868,11 +1900,11 @@
>{
>const dav_liveprop_spec *info;
>
> -#ifndef WIN32
> -/* this property is not usable (writable) on the Win32 platform */
> +/* XXX this property is not usable (writable) on all platforms
> + * Need an abstract way to determine this.
> + */
>if (propid == DAV_PROPID_FS_executable && !resource->collection)
> return 1;
> -#endif
This is a bad change. Unless/until you get the query, then you should not be
removing this check. You're now advertising a property as being present, but
is ineffectual.
There are clients out there today which know and handle this custom
property. You're breaking them.
>...
> @@ -2036,10 +2068,11 @@
>
>void dav_fs_gather_propsets(apr_array_header_t *uris)
>{
> -#ifndef WIN32
> +/* XXX: Need an abstract way to determine this on a per-filesystem basis
> + * This may change by uri (!) (think OSX HFS + unix mounts).
> + */
>*(const char **)apr_array_push(uris) =
>"<http://apache.org/dav/propset/fs/1>";
> -#endif
>}
Same problem. The #ifndef should be restored unless/until you have the query
mechanism in place.
>...
> @@ -2077,16 +2110,8 @@
> what, phdr);
> (void) dav_fs_insert_prop(resource, DAV_PROPID_getetag,
> what, phdr);
> -
> -#ifndef WIN32
> -/*
> -** Note: this property is not defined on the Win32 platform.
> -** dav_fs_insert_prop() won't insert it, but we may as
> -** well not even call it.
> -*/
>(void) dav_fs_insert_prop(resource, DAV_PROPID_FS_executable,
> what, phdr);
> -#endif
And one more to put back.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: Tagging 1.3.23
On Mon, Jan 21, 2002 at 07:58:57PM -0500, Jim Jagielski wrote: > I'm also thinking of the # of patches OtherBill is folding in > as well... By "bumping" the tag, do you mean having the tag > say something like APACHE_1_3_23_R2 or something? Not APACHE_1_3_24 > right? :) :) > > Bill Stoddard wrote: > > > > The tarball hasn't been rolled, so if the change is simple, why not make the >change and > > bump the tag? This method has worked pretty good for 2.0 if we impose a >reasonable time > > limit between the tag and the roll (say 24 hours). +1 on Bill's suggestion. And no, it wouldn't be funny tags. We'd just point the tag at a different revision for that one file. For all intents and purposes, the fix is now part of the 1.3.23 tag. btw, Bill: you can/should still manually tag that .cvsignore to be correct. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: APR_BRIGADE_NORMALIZE
On Sun, Jan 20, 2002 at 10:36:47PM -0500, Cliff Woolley wrote: > > Can someone please remind me why APR_BRIGADE_NORMALIZE is absolutely > necessary? It's only being used in one place AFAICT (in core.c), and as > far as I know, that just means there's a bug we're patching around rather > than fixing. I was grudgingly okay with it until today, when it occurred > to me that removing all zero-length buckets would nuke any third-party > metadata buckets that might be in the brigade, meaning that it's a really > bad idea. There was a bug somewhere with zero length buckets. I don't remember, and I bet it doesn't exist now. I say torch the damned thing. You're quite right: removing zero length buckets is Bad Juju. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Directive "overloading"
On Sun, Jan 13, 2002 at 12:22:16PM -0500, Jim Jagielski wrote: > I've never been that happy with the current situation that regex pattern > matching for some directives (like Directory) have both an overloaded > version (Directory ~) and a full directive (DirectoryMatch). I am > planning on adding some regex matching to some proxy functions, > however, and I plan on doing so *only* via separate directives. > Speak up now if this causes excess heartburn for anyone... +1 +0 on torching the existing, overloaded forms. (and have Apache issue warnings for a few releases) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: reason for the segfaults during graceful shutdown of worker MPM
On Sat, Jan 12, 2002 at 09:25:30AM -0800, Brian Pane wrote: > Greg Stein wrote: >... > >null parents are ugly. isn't there another pool that has the right lifetime > >for that pool? > > There isn't a pool with the right lifetime in the current > worker design. The safe pool to use as the parent would be > the worker thread's pool, but the listener thread doesn't > have a way to use that pool for the accept. In Aaron's >... No overall MPM pool? Or an apache global pool? Anything...? :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: reason for the segfaults during graceful shutdown of worker MPM
On Thu, Jan 10, 2002 at 09:10:13PM -0800, Brian Pane wrote: >... > Is there any reason why the ptrans pool needs to be a child of > tpool? If we can use a null parent for ptrans, this problem should > go away. null parents are ugly. isn't there another pool that has the right lifetime for that pool? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: apache-1.3/src/main http_core.c
Option 2. I seem to recall the same conversation about structure extensions a couple years ago or so. Consensus seemed to say minor bump. Cheers, -g On Thu, Jan 10, 2002 at 11:05:40AM -0500, Rodent of Unusual Size wrote: > So.. should this be changed to > > 1. no MMN bump, > 2. a minor MMN bump, > 3. a major MMN bump, or > 4. reverted right out of 1.3? > > I'm personally cool with any except #4.. > -- > #ken P-)} > > Ken Coar, Sanagendamgagwedweinini http://Golux.Com/coar/ > Author, developer, opinionist http://Apache-Server.Com/ > > "All right everyone! Step away from the glowing hamburger!" -- Greg Stein, http://www.lyra.org/
Re: APACHE_2_0_30 tagged
On Tue, Jan 08, 2002 at 04:20:40PM -0800, Brian Pane wrote: > Aaron Bannert wrote: >... > >APACHE_2_0_30 has been tagged as promised. Let's get to testing this > >bugger and see if we can make it a beta! > > Don't we already know that it's not beta-quality, based on the > problems with 2.0.30 on daedalus? So?! Release it as an alpha quality build. And it is even possible to (gasp) inform potential users of the problems that we've seen and let them decide whether they may be affected. People really need to get out of the mindset of "this must be perfect before we can let people play with it." Give them the info and let *them* decide. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Time to tag (was Re: [PROPOSAL] apr_shm_t, a new shared memory API to replace old)
On Mon, Jan 07, 2002 at 05:30:15PM -0500, Bill Stoddard wrote: >... > Good points. Consider this though... we have not released a 2.0 beta yet that >included > prebuilt binaries. binbuild was seriously broken in 2.0.28, so much so, that it was >not > feasible to build binaries. I would not be against tagging but holding off going for >a > beta if we can get the four things on your short list completed in a reasonable >amount of > time. We gotta get 2.0 out in the hands of more people and the sooner the better. My >$.02. Great. Let's tag/roll/release this as an alpha then. Who says we can't call it alpha? If that is what we want to call it, then fine. We can also release it as "beta quality, but no binary builds this time." Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Time to tag (was Re: [PROPOSAL] apr_shm_t, a new shared memory API to replace old)
On Mon, Jan 07, 2002 at 02:22:45PM -0800, Justin Erenkrantz wrote: > On Mon, Jan 07, 2002 at 01:55:05PM -0800, Aaron Bannert wrote: > > Do I have any support for a tag right now? +1 >... > My only concern is that we're changing lots of APIs between betas. > I don't know if that is an overall concern or not. (I know David > has brought up this issue before.) If the APIs are going to change, then they're going to change. Whether that is between betas, or between beta and final. Forcing them to *not* change is a disservice to our users -- if better code requires an API change, then let's give them the better code. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Emacs stanza?
On Thu, Jan 03, 2002 at 10:51:22AM -0500, Rodent of Unusual Size wrote: > [sent last night, looks like it didn't get through..] > > I don't know how many people use Emacs to edit the Apache stuff, > but would anyone object to a stanza at the bottom of the source > files to help put Emacs in the right stylistic mood? To wit, It's all right with me! I already have all that set up by default, but it doesn't hurt to put it into the file, too. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Input Filtering and Pushback.
[ I'm still trying to catch up on several weeks of email... just a quick note on something here... ] On Mon, Dec 31, 2001 at 10:02:41AM -0800, Ryan Bloom wrote: > On Monday 31 December 2001 02:40 am, Justin Erenkrantz wrote: >... > > I think the best strategy would be to introduce a new input filter > > mode (call it PEEK and rename the current PEEK mode to EATCRLF) that > > I would recommend not re-using the PEEK mode, because that will > cause confusion to people who have written input filters already. Regardless, the existing PEEK ought to be renamed EATCRLF. Or even tossed entirely somehow. >... > > 5. Run through all buckets in the brigade and read/copy into > >the caller's buffer. (What happened to zero-copy?) > > We don't do zero-copy on input. The real benefit to zero-copy > is on output, because the data tends to be larger. There *should* be a benefit on input, too. Think about a PUT of a gigabyte file that you want to dump onto the disk. In the best of all worlds, this would map into a sendfile() from the socket to a file descriptor. >... > > 10. Call ap_get_brigade with PEEK. > >- Should this PEEK call block? Is that an option that the > > caller to ap_get_brigade specifies? I think ap_getline > > would indeed want it to be a blocking PEEK. > > If we are going to conitnue to extend input filtering, we should > probably split the mode from the blocking. Absolutely agree. Whether you have two parameters, or a single param with bitfields... *shrug* IMO, I like two params. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Get mod_dav to dynamically register its methods
On Sat, Dec 22, 2001 at 07:02:39PM +0100, Sander Striker wrote: > This patch gets mod_dav to register its methods dynamically. > Instead of relying on the fact that a method isn't registered > and comparing method strings, it now registers and compares > method numbers. Sander and I talked about this via AIM. He's going to do a new patch which fixes ap_method_register(). That function should return the method number if someone has already registered the method (which could easily happen across modules). Second, he's going to switch the DAV_M_ constants into an enumerated value. Overall, the patch looks great. It really cleans up dav_handler(), and the integer checks will be slightly faster (not that it matters right there, tho). For those unaware of the problem: with the introduction of the method registry, at some point, the server started using registered method numbers rather than M_INVALID for the unknown methods. In particular, this would happen as a result of a Limit(Except) referring to a new method and registering it. dav_handler() would then get a method number it doesn't understand; in particular, it wouldn't get M_INVALID for "REPORT" and it wouldn't ever respond to that method. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/build make_nw_export.awk
On Fri, Dec 21, 2001 at 05:15:46PM -0500, Cliff Woolley wrote: > On Fri, 21 Dec 2001, Brad Nicholes wrote: > > > realizing that this is just a hack. If we are going to use the AWK > > script to produce the export list, then it needs to be much smarter. It > > has to obey the same preprocessor rules as the compiler does. AP_DEBUG > > Forgive me for stating the obvious, but if this is the case, why not just > run the awk script through cpp? How about if we link the library and then use "nm" to list all the symbols and then find all symbols that start with "apr_" and export those. That will give us a *precise* set of symbols, without worry for whatever set of conditions were present at preprocessor or compilation time. I understand "nm" isn't portable, but most platforms have such a tool. On Windows, it is "objdump" (IIRC). I bet we can find a similar tool on all platforms. If not, then we can hope there is a platform specific way to link in all symbols, or that we don't need the hack, or that we can use a "pretty close" AWK script. Thoughts? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: .la files for Apache DSOs
On Thu, Dec 13, 2001 at 09:14:55PM -0500, Jeff Trawick wrote: >... > We already do link DSOs with -module, though we also throw on > -export-dynamic as well, which doesn't seem appropriate. Well, some modules export symbols for use by other modules. Maybe that is why? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: .la files for Apache DSOs
On Thu, Dec 13, 2001 at 06:00:12PM -0500, Jeff Trawick wrote: > Does anybody actually *want* these installed into prefix/modules by > "make install" and "apxs -i"? > > If so, please speak up. I'd be curious about *why*. I say "no". As modules, they aren't intended for further linking. Note that we should probably be linking the modules with libtool's -module switch. It would be interesting to see if that prevents the installation of the .la file. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Is there any reason
On Thu, Dec 13, 2001 at 03:10:29PM -0600, William A. Rowe, Jr. wrote: > to waste cycles on the insert_filters hook if we don't run a request? > Should this simply get pushed from ap_process_request_internal() into > ap_invoke_handler() as the first part of the handler-execution phase? Seems reasonable to me. The intent was always to run that hook right before the handling starts. It would let everything get set up in the request_rec, and then people can make choices about whether to insert filters. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/support Makefile.in
On Wed, Dec 12, 2001 at 07:39:55PM -, [EMAIL PROTECTED] wrote: > trawick 01/12/12 11:39:55 > > Modified:.acinclude.m4 configure.in >support Makefile.in > Log: > change Apache/apr/apr-util to use run-time linking on AIX > > currently, a kludge (-uXML_Parse) is needed to get a reference to expat > in the Apache executable programs; I think this is related to the fact > that expat libtool is getting generated a little differently than apr > libtool and is choosing to build a different flavor of shared library If you patch the build in apr-util/xml/expat/, then I'll synchronize your changes with the main Expat source control. That is: we can get the "right flavor" in whatever fashion you choose. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/experimental config.m4
I don't understand why an include doesn't go onto the INCLUDES variable. Why
the shift?
On Sun, Dec 09, 2001 at 03:32:02AM -, [EMAIL PROTECTED] wrote:
> trawick 01/12/08 19:32:02
>
> Modified:modules/experimental config.m4
> Log:
> Add -I for the zlib include dir to the right flag variable
>
> Revision ChangesPath
> 1.17 +1 -1 httpd-2.0/modules/experimental/config.m4
>
> Index: config.m4
> ===
> RCS file: /home/cvs/httpd-2.0/modules/experimental/config.m4,v
> retrieving revision 1.16
> retrieving revision 1.17
> diff -u -r1.16 -r1.17
> --- config.m4 2001/11/26 22:50:13 1.16
> +++ config.m4 2001/12/09 03:32:02 1.17
> @@ -53,7 +53,7 @@
>ap_save_ldflags=$LDFLAGS
>ap_save_libs=$LIBS
>if test "$ap_zlib_base" != "/usr"; then
> - APR_ADDTO(INCLUDES, [-I${ap_zlib_base}/include])
> + APR_ADDTO(CPPFLAGS, [-I${ap_zlib_base}/include])
> APR_ADDTO(LDFLAGS, [-L${ap_zlib_base}/lib])
> if test "x$ap_platform_runtime_link_flag" != "x"; then
> APR_ADDTO(LDFLAGS, [$ap_platform_runtime_link_flag${ap_zlib_Base}/lib])
>
>
>
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/filters mod_include.c
If you really want to get anal: if ((*tag == 'v' && strcmp(tag, "virtual") == 0) || ... You can avoid a whole strcmp. woo! (not) Then again, you'd just be obfuscating the damned code for little overall gain. Cheers, -g On Fri, Dec 07, 2001 at 03:09:54AM -, [EMAIL PROTECTED] wrote: > brianp 01/12/06 19:09:54 > > Modified:modules/filters mod_include.c > Log: > When checking for "file" or "virtual" as an argument to > "
server->port busted
The Port directive used to set the server->port variable. When Port was tossed, we also lost the use of server->port. That affects the response of ap_get_server_port() and ap_matches_request_vhost(). [ exercise for the reader to look into those ] The basic problem is now that the server doesn't know its own port when it needs to create a redirect or some kind of response. The ports are all in the Listen records, which aren't readily available. And we don't get a full mapping of the Listen stuff into the server_rec data. The problems in ap_get_server_port() may be relaxed if we didn't enable UseCanonicalName by default, but we do... Example case: tell Apache that it is listening on port 8080. That port number does not appear anywhere (not in a server_rec or a server_addr_rec). When a client makes a request, if they say "Host: localhost:8080", then the 8080 will appear in r->parse_uri.port. Of course, they could also lie on that port and the server wouldn't know any better. I'm not exactly sure what the right answer to this is. Kind of a fundamental problem with Apache not able to get a port number for "this server". Ideas? Thoughts? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Concatenation of small buckets in core_output_filter
On Mon, Nov 26, 2001 at 09:28:03AM -0800, Brian Pane wrote: >... > * Rather than creating a temporary brigade for concatenation, > create a heap bucket. Make it big enough to hold 8KB. Pop > the small buckets from the brigade, concatenate their contents > into the heap bucket, and push the heap bucket onto the brigade. > > * If we end up in the concatenation again during the foreach loop > through the brigade, add the small buckets to the end of the > previously allocated heap bucket. If the heap bucket size is > about to exceed 8KB, stop. > > Comments? You could actually scan the whole brigade and replace small buckets with a large heap bucket. For example: compress the first 20 5-byte buckets into a single 100-byte bucket, skip the 10k file bucket, then compress the next 50 10-byte buckets into a 500-byte bucket. The rule of thumb is simply "stop concatenating if you find a bucket bigger than your threshold." Whether you skip over those looking for more is a different question. Note that apr_brigade_write() is fine if you're writing to the "end" of a brigade. It doesn't work well "within", and it could also pose problems if you trigger its flushing behavior. That said, if you're concatenating and *moving* buckets (i.e. from the input brigade to a holding brigade), then you're always writing at the end. Also: why would this ever "exceed 8KB" ?? If that was the case, then you'd just send the brigade to the network rather than try to hold onto it. Finally: since you probably know the size of your concat-buffer, then you can allocate the heap bucket to be the proper size, rather than use a default 8k heap bucket. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Gack! Weirdo DAV bug.
On Fri, Nov 16, 2001 at 03:30:39PM -0800, Ryan Bloom wrote: > On Friday 16 November 2001 11:18 am, [EMAIL PROTECTED] wrote: > > Ryan Bloom <[EMAIL PROTECTED]> writes: >... > > > Option 3. :-) We have APIs to allow you to check the dynamically > > > registered methods. > > > > I'd already thought about this. But the methods hash is just > > NAME->number mapping. What would this gain us? We already have both > > the name and the number in the request structure. Maybe I'm missing > > something (I hope). > > You misunderstood me. We already have the APIs to do this. The same way > that you compare against M_INVALID, you could actually check against > REPORT directly. The current logic is basically: if (method_number != M_GET) return do_get(); if (method_number != M_PROPFIND) return do_propfind(); if (method_number != M_INVALID) return DECLINED; if (strcmp(method_name, "REPORT") == 0) return do_report(); if (strcmp(method_name, "MERGE") == 0) return do_merge(); We use the M_INVALID to shortcut a bunch of strcmp() operations on the method name. Switching that logic over to use the APIs is not going to save us much (if anything!) over the strcmp(). For each method, we'd have to call into the API to say "is it one?" or "is it one?" I'm going to have to look into this whole method registration thing. I saw it go in, but never bothered looking too closely. It didn't seem like it was going to monkey with the method number stored in the request. Eek. Dunno that the method stuff must change. At a minimum, mod_dav can just pre-register everything and keep the numbers around for use in the above logic. But I still want to get a look at the stuff and see what's there. I've got some particular experience with extended methods :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH 2] Re: [PATCH] mmap_setaside (with apr_mmap_dup)
On Thu, Nov 15, 2001 at 08:35:47PM -0800, Brian Pane wrote: >... > When I tried to implement a traditional reference-counting solution > (all apr_mmap_t's duplicated from the same original share a reference > count, and the mmap'ed segment is destroyed only when the refcount > reaches zero), I ran into threading problems with mod_file_cache. > It's possible for two threads to be doing a dup of the same > cached apr_mmap_t at the same time--if they're both delivering the > same statically cached file, and they both end up having to call > mmap_setaside. To avoid the race condition here, we'd need a lock > around the reference count. The lock API requires that we create the > lock from a pool, but there isn't a really suitable pool from which > to allocate the lock, given that its lifetime isn't necessarily tied > to that of a request or connection. The "suitable pool" is that of the apr_mmap_t itself. Note that the file cache would be putting that lock into its own long-lived pool. Not so hard, or I'm missing something :-) > Fortunately, this whole problem went away when I switched from the > reference-counted model to the transfer-of-ownership model. The latter > doesn't require any locking. Yah. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] mmap_setaside (with apr_mmap_dup)
On Wed, Nov 14, 2001 at 10:05:18PM -0800, Brian Pane wrote:
> Based on Cliff's suggestion, this patch introduces an apr_mmap_dup()
> function that's used to setaside mmap buckets without having to
> memcpy the content.
Cool.
> I gave up trying to do full reference counting semantics for
> duplicates of apr_mmap_t, because I couldn't find a suitable
> pool to own the locks that would be required in a threaded
> implementation.
Not sure what you mean here. I don't quite understand how threading comes
into play. apr_file_t and apr_mmap_t can't be used by multiple threads,
AFAIK. And I don't think they should either...
> Instead, apr_mmap_dup() lets the caller
> specify which of the two apr_mmap_t structs--the original one
> or the new one--is the owner of the mmap (and is responsible
> for destroying it).
Hrm. It would have been really nice to use apr_pool_is_ancestor(), but I
could see a case where you have disjoint pools:
A
/ \
B C
And you're trying to transfer ownership from B to C.
>...
> --- srclib/apr/mmap/win32/mmap.c 2001/06/27 22:07:24 1.6
> +++ srclib/apr/mmap/win32/mmap.c 2001/11/15 05:45:58
> @@ -62,10 +62,15 @@
>
> #if APR_HAS_MMAP
>
> -static apr_status_t mmap_cleanup(void *themmap)
> +APR_DECLARE(apr_status_t) apr_mmap_cleanup(void *themmap)
> {
Why is this APR_DECLARE'd now? And why the function rename?
In fact, doesn't this change the cleanup to a pascal-style function, and
thus not able to be used as a cleanup callback?
>...
> +APR_DECLARE(apr_status_t) apr_mmap_dup(apr_mmap_t **new_mmap,
> + apr_mmap_t *old_mmap,
> + apr_pool_t *p,
> + int transfer_ownership)
> +{
> +*new_mmap = (apr_mmap_t *)apr_palloc(p, sizeof(apr_mmap_t));
> +memcpy(*new_mmap, old_mmap, sizeof(apr_mmap_t));
> +(*new_mmap)->cntxt = p;
For the two dup() functions, you can use apr_pmemdup() to simplify this.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/filters mod_include.c mod_include.h
>... cvs commit email ... > > This passes all tests in the test suite. I've been seeing that lately on commits. Passing the tests doesn't mean the change was correct. The test suite only tests a small portion of Apache's functionality. Yes, tests should be run. But I just wanted to send out a word of caution: don't use them as a crutch. They will most definitely become one if you let them. THINK about your changes. Cheers, -g -- Greg Stein, http://www.lyra.org/
not in CVS? (was: Re: 2.0.28-beta release?)
On Tue, Nov 13, 2001 at 04:08:09PM -0500, Greg Ames wrote: >... > As it turns out, the docs/conf/httpd-*.conf files also have post-tag > changes. So changing/re-tagging them in cvs would be as complex as > changing the code. WHAT? Are you saying that I cannot produce the 2.0.28 tarball from CVS? That isn't right. -g -- Greg Stein, http://www.lyra.org/
Re: mod_include vs file descriptors
On Tue, Nov 13, 2001 at 11:19:54AM -0500, Cliff Woolley wrote: > On Tue, 13 Nov 2001, Brian Pane wrote: > > How about a "close-on-mmap" flag for file buckets, which > > mod_core would set when creating file buckets but mod_file_cache > > wouldn't? Don't make it so specific. Just insert the file (or mmap or any other object!) into the bucket with a *refcount*. When you transform the file into a bucket, you decrement the refcount. If it hits zero, then you close the file. The cache inserts with refcount==1. Everybody else does count==0. > I've proposed that before. But if it wasn't vetoed it was -0.5'ed, with > the reason being that a lazy closing of file descriptors and other > resources is one of the main points of the pool cleanup system; closing > all these things can take a good deal of time, and it's better to do it > after the client has received all of the data than during mainline request > processing. That is bogus, as has been pointed out. You have to close them sometime. Doing it sooner rather than later means that you have more file descriptors available for further processing in the request. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: 2.0.28-beta release?
On Tue, Nov 13, 2001 at 12:00:31PM -0800, Ryan Bloom wrote: > On Tuesday 13 November 2001 11:28 am, William A. Rowe, Jr. wrote: > > From: "Greg Ames" <[EMAIL PROTECTED]> > > Sent: Tuesday, November 13, 2001 12:56 PM >... > > > that file, do the PITA dance with the CHANGES file, probably do a little > > > testing, re-roll, rename the tarballs as beta. What do others think? I > > > could note that this happened in the CHANGES file since I have to mess > > > with it anyway. > > > > I'd suggest that you checkout on APACHE-2_0_28, tag as APACHE-2_0_28_ALPHA > > for historical reasons, then we can add APACHE-2_0_28_BETA, etc. > > No, there is 2.0.28, period. There isn't a 2.0.28-alpha and 2.0.28-beta > code base. There is one 2.0.28 codebase. You could have different versions > if the alpha/beta distinction was in the code, but it isn't. It is only in the >tarball > name. I'm with Ryan. 2.0.28 is exactly that. alpha and beta are *designations*. They are not code changes. 2.0.28 should be buildable from the label. If the CHANGES file was modified *outside* of that, then we have a problem. Ship the damned code. This is a BETA for crying out loud. We don't need to weasel patches in. "oh, just this one little fix." Fuck that. Beta means bugs. Beta means that we have a list of issues for people to be concerned with. Beta means people may have platform-specific problems. Beta is not GA. +1 on beta. Get a release out the door. Christ almighty... what's it take around here? The new release process was intended to get tarballs *out* to people. Not to be held up in a bunch of snippy little bug fix this, bug fix that. Ship it out with bugs. Call it alpha if it doesn't feel right. Call it beta if it feels good. If an ErrorDocument doesn't work in one case, then tell people "too bad. don't do that". If the server dies with a particular subrequest executed from some wonky CGI-provided SSI document, then say "get this patch." But we gotta get more releases out into the public's hands. 2.0.16 was crap. Should people really be using that? Not a chance. Give them 2.0.28. -g -- Greg Stein, http://www.lyra.org/
more buglets
* the proxy code still refers to conn->client_socket * perchild refers to conn->client_socket -g -- Greg Stein, http://www.lyra.org/
another bug(?) in the recent changes
Unless I'm mistaken, some code got dropped from connection.c. In the current CVS, it is at line 195. A call to apr_socket_close() and a return are missing. It looks like they disappeared when rev 1.87 was checked in. Right/wrong? Cheers, -g -- Greg Stein, http://www.lyra.org/
MPM design abuse (was: cvs commit: httpd-2.0/server/mpm/prefork prefork.c)
On Tue, Nov 13, 2001 at 07:15:36AM -, [EMAIL PROTECTED] wrote:
> rbb 01/11/12 23:15:36
>
> Modified:include http_connection.h
>server connection.c core.c
>server/mpm/prefork prefork.c
> Log:
> This allows modules to add socket descriptors to the pollset. I have
> only added this to the perfork MPM, and the others work without it.
> Tomorrow I will add it to the other MPMs.
>...
> --- core.c 2001/11/13 02:09:07 1.91
> +++ core.c 2001/11/13 07:15:36 1.92
> @@ -3329,6 +3329,19 @@
>return net->c;
>}
>
> +static int core_add_listeners(apr_pollfd_t *pollset,
> + apr_socket_t **listensocks, int num_listensocks)
> +{
> +int i;
> +ap_listen_rec *lr;
> +
> +for (lr = ap_listeners, i = 0; i < num_listensocks; lr = lr->next, i++) {
> +listensocks[i] = lr->sd;
> +apr_poll_socket_add(pollset, listensocks[i], APR_POLLIN);
> +}
> +return OK;
> +}
I think this is *very* wrong, and I'm even tempted to -1 the darn thing. I
think we need a bit more discussion before continuing here.
The problem is that now an MPM must implement ap_listeners for use by
core.c. The MPM must also use a pollset.
That is NOT right. The MPM is the thing that defines how sockets are to be
set up, listened to, and accepted. It then defines the mapping from those
sockets to backend workers.
Think about the above code: the MPM sets up ap_listeners, and then it calls
an external function to process them, to put them into a pollset that it
created. It's like a damned little GOSUB to get some of its own work done.
The core is not adding any value here. It grabs some data from the MPM (the
ap_listeners variable) and puts it right back into an MPM structure (the
passed pollset variable).
And the interface is just whacked anyhow. The ap_listeners is a global, but
the size of it is a passed parameter. If you want to do this right, then
ap_listeners should not be a global (we've got a serious over-reliance on
globals!), but should be a passed parameter. Yet if that is done, then
again: the hook is not adding any value. The MPM passes everything it needs;
the hook just runs a loop for the MPM. Kinda silly...
And with this structure, how is an MPM supposed to set up different sets of
listeners for different threads? Oh... sorry, but the MPM can't define that
any more. I think with a bunch of monkey business, the MPM might be able to
define different sets for child processes. But the MPM is going to be
working *around* the issue when it should be *in charge*.
This change violates the very notion of an MPM being the piece of code
responsible for socket and worker handling.
I like the code changes for reducing the socket usage from Apache proper.
But removing it from the MPM isn't right.
Please explain :-(
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Mon, Nov 12, 2001 at 11:49:08PM -, [EMAIL PROTECTED] wrote:
>...
> --- httpd.h 2001/10/23 17:26:57 1.168
> +++ httpd.h 2001/11/12 23:49:06 1.169
>...
> +typedef struct core_output_filter_ctx {
> +apr_bucket_brigade *b;
> +apr_pool_t *subpool; /* subpool of c->pool used for data saved after a
> + * request is finished
> + */
> +int subpool_has_stuff; /* anything in the subpool? */
> +} core_output_filter_ctx_t;
> +
> +typedef struct core_filter_ctx {
> +apr_bucket_brigade *b;
> +int first_line;
> +} core_ctx_t;
> +
> +typedef struct core_net_rec {
> +/** Connection to the client */
> +apr_socket_t *client_socket;
> +
> +/** connection record */
> +conn_rec *c;
> +
> +core_output_filter_ctx_t *out_ctx;
> +core_ctx_t *in_ctx;
> +} core_net_rec;
The distinction between core.c and connection.c is currently too nebulous to
try and keep code in one module or another. The whole premise of this change
was to *hide* the above structures. You can/should do so by moving them into
core.c. Pull functions from connection.c into core.c to enable that change.
Honestly, I can never find functions any more. If it has something to do
with serving http, then I have to look in *SEVEN* files:
server/connection.c
server/core.c
server/protocol.c
server/request.c
modules/http/http_core.c
modules/http/http_protocol.c
modules/http/http_request.c
The boundaries and purposes of each of these are so flimsy that it is not
obvious where things are found. I'd rather just cat them into one big file
and stop worrying about where to find the darned stuff.
>...
> --- core.c 2001/11/08 14:29:36 1.88
> +++ core.c 2001/11/12 23:49:06 1.89
>...
>static int core_input_filter(ap_filter_t *f, apr_bucket_brigade *b,
>ap_input_mode_t mode, apr_off_t *readbytes)
>{
>...
> +if (ctx->first_line) {
> +apr_setsocketopt(net->client_socket, APR_SO_TIMEOUT,
> + (int)(keptalive
> + ? f->c->base_server->keep_alive_timeout *
>APR_USEC_PER_SEC
> + : f->c->base_server->timeout * APR_USEC_PER_SEC));
> +ctx->first_line = 0;
> +}
> +else {
> +if (keptalive) {
> +apr_setsocketopt(net->client_socket, APR_SO_TIMEOUT,
> + (int)(f->c->base_server->timeout *
>APR_USEC_PER_SEC));
> +}
> +}
This logic is incorrect. first_line is only going to be set when the filter
is first installed. Thus, the keep_alive_timeout will never be used.
The old code would execute the "first_line" branch of logic on each entry to
ap_read_request().
>...
> +static conn_rec *core_create_conn(apr_pool_t *ptrans, apr_socket_t *csd,
> + int my_child_num)
> +{
> +core_net_rec *net = apr_palloc(ptrans, sizeof(*net));
> +
> +net->in_ctx = NULL;
> +net->out_ctx = NULL;
> +net->c = ap_core_new_connection(ptrans, ap_server_conf, csd,
> +net, my_child_num);
ARGH!!
ap_server_conf is a global. That used to be passed to the connection
creation function. It should continue to be passed. *Please* don't continue
propagating global variables :-(
>...
> --- perchild.c 2001/11/10 18:26:29 1.82
> +++ perchild.c 2001/11/12 23:49:07 1.83
> @@ -502,7 +502,7 @@
>ap_sock_disable_nagle(sock);
>}
>
> -current_conn = ap_new_connection(p, ap_server_conf, sock, conn_id);
> +current_conn = ap_run_create_connection(p, ap_server_conf, sock, conn_id);
There is an extra parameter there, but I think it should stay and the rest
should change :-)
Otherwise... a good move. Can we toss the pre_connection stuff?
Oh, and I clipped out the quoted text. The core_create_conn should be
APR_HOOK_MIDDLE. There is no reason for it to be "last." The MIDDLE is the
standard one to use when you just need to pick something and have no
particular concerns about where it falls. The core_create_conn is "just
another" hook.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: MPM re-write for network logic
On Mon, Nov 12, 2001 at 06:55:43PM -0800, Ryan Bloom wrote: >... > I am trying to remove the network logic from the MPMs, so that modules can > implement different transport layers. I am looking at using a couple of hooks to > accomplish this. The problem is that Windows just doesn't fit into this model at > all. I think your original premise is incorrect. MPMs are all about waiting on one or more sockets, accepting connections, handling those connections, and mapping units of work to workers. By its very nature, it depends upon the sockets. Apache as a whole should not know about the socket (only the core filters), but the MPM definitely should. It is the beast which maps *sockets* to *workers*. It has to know. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Remove mod_dir showstopper
The patch looks very good to me! On Mon, Nov 12, 2001 at 04:28:38PM -0500, Rodent of Unusual Size wrote: > Joshua Slive wrote: > > > > > How about "redirect-only-on-get"? Same length, more clewful.. :-) > > > > And some documentation would be nice. manual/env.html would be > > the correct place. > > Actually, even "redirect-only-on-get" isn't clewful enough; it > somehow needs to convey that the redirect involved only has > to do with directory resources. Hmm... we have several pieces of information "redirect", "directories", and "non-GET methods". That is a lot to put into a single name. I say forget trying... How about: redirect-carefully Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] apr_table WAS: What to do about tables?
On Mon, Nov 12, 2001 at 09:45:58PM +0100, Mladen Turk wrote: >... > If we can trust that crc32 uniquely identifies keys than we can use the > following patch... > It would be nice because it's very simple and shows overall 10 times > performance increase. Mladen: whenever you attach files or patches to a message, *ensure* they are in text/plain format. You keep attaching them as application/octet-stream. That is bad... -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] apr_table WAS: What to do about tables?
On Mon, Nov 12, 2001 at 10:02:43AM -0800, Brian Pane wrote: >... > By the way, I have an alternate implementation of a > table speedup that I wrote late yesterday. It caches > a 32-bit checksum for each key in the table and compares > checksums to decide whether it can skip the strcasecmp. > It's still O(n), but the idea is to reduce the constant > multiplier. (The checksum computation basically just > packs the first four bytes of the string into an int and > does some bit masking to normalize to all-uppercase. The bit masking won't work on EBCDIC machines. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0 STATUS
On Mon, Nov 12, 2001 at 09:28:40AM -0800, Aaron Bannert wrote: >... > Whether or not this is a showstopper is negotiable, and I think it > is completely reasonable to move ahead with a GA release if we are > ready. OTOH, I don't think we should do significant style updates *after* > a release. In the mean time I'd personally like to make an effort to > clean up the style and make it consistent and readable. Nothing is stopping you. If you want to make an effort, then just go ahead. It is impossible for somebody to say "don't spend your time with that; do instead." Of coures, they can veto your patch, but I'd wonder what kind of technical justification they'd have for that :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: What to do about tables?
On Sun, Nov 04, 2001 at 03:00:43PM -0800, Brian Pane wrote: >... > 2. Change the performance-sensitive fields in the httpd's request_rec >from apr_table_t (like r->headers_in) to some different data type >(e.g., "ap_http_headers_t") that supports O(log(n)) or O(1) get/set >operations > Pros: > * Performance improvement for the httpd > * Other code that uses apr_table_t isn't affected > Cons: > * Lots of changes required to the httpd core and all modules This is the most optimal approach. You can build a structure that is O(1) for the common headers (by using tokens for the headers rather than strings). Other headers can also be heavily optimized through a hash lookup. I think this custom type would be a wrapper around an apr_hash_t. > 3. Keep using apr_table_t for the fields in request_rec, and redesign >the internals of apr_table_t to support O(log(n)) access > Pros: >* Performance improvement for the httpd >* Almost no impact on code that uses APR > Cons: >* Changes required to code that uses apr_table_elts() (on the order > of half a dozen calls in Apache 2.0, and occasional usage in the > handful of large 3rd-party modules that I've checked) This helps users of apr_table_t in general, but most of those users should be using apr_hash_t instead. The best thing is to encourage them to change their data type. Optimizing apr_table_t simply reduces their impetus to change their code. The apr_table_t is interesting in that it can keep multiple values for a key. That is only really used for headers, and that can be best-solved by using a new type. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: 2.0 / POST read example
As Ryan points out: the old functions work quite fine. You can also toss a layer out (and a copy!) if you're willing to deal with brigades, and use ap_get_brigade(). Cheers, -g On Mon, Oct 29, 2001 at 12:14:54PM -0800, Ryan Bloom wrote: > > Use the same code that you would have used in 1.3, namely ap_setup_client_block, > ap_get_client_block, etc. The filtering is all done under the covers. > > Ryan > > On Monday 29 October 2001 12:05 pm, Dirk-Willem van Gulik wrote: > > Anyone a good pointer as to where to snarf proper 'post' read code; i.e. I > > need to get some post data processing done in a handler in a module - and > > want to do it properly -i.e. through the filter chain etc. > > > > Any place I can cut and paste this from :-) > > > > Dw -- Greg Stein, http://www.lyra.org/
Re: Request for Patch to 1.3.x
On Wed, Oct 03, 2001 at 11:19:34AM -0700, Kevin Mallory wrote:
>...
[ patch allows custom caching mechanisms ]
>...
> The patch simply adds a new callback (the 'filter callback') into the
> handling in buff.c's routine writev_it_all() and buff_write().
>
> When not registered, there is no performance impact to users. This
> filter callback makes it possible for SpiderCache to correctly intercept
> the request as it is being processed, thus allowing our product to perform
> dynamic page caching.
+1 on including this patch.
I see no bad effects, and it has definite utility.
Cheers,
-g
>...
> *** orig_buff.c Tue Aug 21 17:45:34 2001
> --- spidercache_buff.cTue Aug 21 17:45:35 2001
> ***
> *** 356,361
> --- 356,365
> {
> int rv;
>
> + if (fb->filter_callback != NULL) {
> + fb->filter_callback(fb, buf, nbyte);
> + }
> +
> #if defined(WIN32) || defined(NETWARE)
> if (fb->flags & B_SOCKET) {
> rv = sendwithtimeout(fb->fd, buf, nbyte, 0);
> ***
> *** 438,443
> --- 442,450
> (size_t) SF_UNBOUND, 1, SF_WRITE);
> #endif
>
> + fb->callback_data = NULL;
> + fb->filter_callback = NULL;
> +
> return fb;
> }
>
> ***
> *** 1077,1082
> --- 1084,1095
> static int writev_it_all(BUFF *fb, struct iovec *vec, int nvec)
> {
> int i, rv;
> +
> + if (fb->filter_callback != NULL) {
> + for (i = 0; i < nvec; i++) {
> + fb->filter_callback(fb, vec[i].iov_base, vec[i].iov_len);
> + }
> + }
>
> /* while it's nice an easy to build the vector and crud, it's painful
>* to deal with a partial writev()
> *** orig_buff.h Tue Aug 21 17:45:34 2001
> --- spidercache_buff.hTue Aug 21 17:45:35 2001
> ***
> *** 129,134 ****
> --- 129,138
> Sfio_t *sf_in;
> Sfio_t *sf_out;
> #endif
> +
> + void *callback_data;
> + void (*filter_callback)(BUFF *, const void *, int );
> +
> };
>
> #ifdef B_SFIO
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c
On Mon, Oct 22, 2001 at 04:36:46AM -, [EMAIL PROTECTED] wrote: > wrowe 01/10/21 21:36:46 > > Modified:server core.c > Log: > Fix two typos in the last patch... compiles/tests clean... Greg, please > run this against Apache.org until the first core and post the results. Can we stop putting non-code commentary in the CVS logs? Three years from now, that comment is going to be senseless... If you want to comment on something, then wait for the commit message to hit the mailer, then respond to it, to this mailing list, with your commentary. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Leaks in 2.0.16 beta
We don't make the individual fixes available. Our only real mechanism for distributing changes is to say, "get an updated version." Otherwise, you can crawl through the CVS repository to find the related changes. I'd recommend looking at when the CHANGES file was modified each time; you should be able to find the related commits near those dates. I'm not sure offhand, but I believe we make available archives of the commit mailing list. That would probably be even easier to scan through to find the actual changes/commits. Cheers, -g On Fri, Oct 19, 2001 at 11:45:08AM -0700, Sunitha Kumar wrote: > Folks, > I saw these were fixed in 2.0.18 ( in the Changes file), How can I get > these fixes, individually. > thanks, > > Fixed potential FILE* leak in http_main.c [Ben Laurie] > > Don't leak the DIR * on HEAD request for a directory. [Robert Thau] > > Create Files, and thus MMAPs, out of the request pool, not the > connection pool. This solves a small resource leak that had us > not closing files until a connection was closed. In order to do > this, at the end of the core_output_filter, we loop through the > brigade and convert any data we have into a single HEAP bucket > that we know will survive clearing the request_rec. > [Ryan Bloom, Justin Erenkrantz <[EMAIL PROTECTED]>, > Cliff Woolley] > > -- > Sunitha Kumar > http://www.cisco.com > -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c
I brought this up once before. I think it was Aaron that suggested an "optimization" which changed a pcalloc to a palloc. I noted that doing things like that are troublesome for long term maintenance. Bam. Empirical evidence here. Changing pcalloc to palloc should only be done when we have specific information that it is *really* helpful. Cheers, -g On Fri, Oct 19, 2001 at 02:05:48AM -, [EMAIL PROTECTED] wrote: > wrowe 01/10/18 19:05:48 > > Modified:server core.c > Log: > Guys... please be _careful_ when you pcalloc -> palloc!!! > > Resolves GAmes' segfault observations > > Revision ChangesPath > 1.76 +3 -1 httpd-2.0/server/core.c > > Index: core.c > === > RCS file: /home/cvs/httpd-2.0/server/core.c,v > retrieving revision 1.75 > retrieving revision 1.76 > diff -u -r1.75 -r1.76 > --- core.c 2001/10/16 11:54:06 1.75 > +++ core.c 2001/10/19 02:05:48 1.76 > @@ -183,7 +183,9 @@ > memcpy(conf->response_code_strings, base->response_code_strings, > sizeof(*conf->response_code_strings) * RESPONSE_CODES); >} > - > +else > +base->response_code_strings = NULL; > + >conf->d = new->d; >conf->d_is_fnmatch = new->d_is_fnmatch; >conf->d_components = new->d_components; > > > -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/proxy mod_proxy.c
If it is bogus, then *leave a comment*. Somebody down the road is going to
go and nuke that cast, not knowing *why* it should be there. Commentary in a
CVS log entry helps nobody.
Cheers,
-g
On Thu, Oct 11, 2001 at 02:04:11PM -, [EMAIL PROTECTED] wrote:
> wrowe 01/10/11 07:04:11
>
> Modified:modules/proxy mod_proxy.c
> Log:
> Aye, it's bogus. Something very odd about short terniary results on
> this C[++] compiler here, wants to promote to the conditition's type :-/
>
> Revision ChangesPath
> 1.60 +2 -1 httpd-2.0/modules/proxy/mod_proxy.c
>
> Index: mod_proxy.c
> ===
> RCS file: /home/cvs/httpd-2.0/modules/proxy/mod_proxy.c,v
> retrieving revision 1.59
> retrieving revision 1.60
> diff -u -r1.59 -r1.60
> --- mod_proxy.c 2001/09/28 10:33:39 1.59
> +++ mod_proxy.c 2001/10/11 14:04:11 1.60
> @@ -141,7 +141,8 @@
> if (!(r->parsed_uri.hostname
> && !strcasecmp(r->parsed_uri.scheme, ap_http_method(r))
> && ap_matches_request_vhost(r, r->parsed_uri.hostname,
> - r->parsed_uri.port_str ? r->parsed_uri.port :
>ap_default_port(r {
> + (apr_port_t)(r->parsed_uri.port_str ? r->parsed_uri.port
> + : ap_default_port(r) {
> r->proxyreq = PROXYREQ_PROXY;
> r->uri = r->unparsed_uri;
> r->filename = apr_pstrcat(r->pool, "proxy:", r->uri, NULL);
>
>
>
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_ssl input filtering...
On Fri, Oct 05, 2001 at 03:39:47PM -0700, Ryan Bloom wrote: > On Friday 05 October 2001 03:34 pm, William A. Rowe, Jr. wrote: > > > Now PLEASE understand that maxbytes 0 (originally, the -1 idea) doesn't > > say 'read everything from this socket' --- it leaves the best-fit for > > the underlying filter to decide. If core wants to give back an even > > number of IP frames, then fine. If SSL wants to give back an even > > number of decoded SSL packets, also fine. It will not mean read until > > EOS, ever. It's up to the underlying filter to decide what is optimal > > for max 0, without allocating a bunch of otherwise useless frame buffers. > > > > And the more that I look at this, the more we need a push-back model, > > because the scope of 'this' filter doesn't live as long as the parent > > filter (with request and connection scopes, respectively.) > > A push-back model is not needed. A filter should not be passing back more > data than the parent can use for this request. If we need to push data back, > then there is a problem. When asks for X bytes of data, it is taking > responsibility for using all X bytes. I *strongly* agree with Ryan here. Pushback is *not* needed. If filters want to have a read mode of "give me a 'nice' amount", then that is a separate question. (and the HTTP_IN will determine 'nice' based on request boundaries) But if a filter ever says bytes, then f->next *cannot* ever return more than that amount. IMO, we ought to fix mod_ssl and proxy before worrying about optimizing for "return a nice amount". Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: 1.3.21 fails on NetWare... please check other platforms..
On Fri, Oct 05, 2001 at 04:44:21PM -0400, Cliff Woolley wrote: > On Fri, 5 Oct 2001, Brad Nicholes wrote: > > > Shouldn't this be @@DocumentRoot@@ rather than @@ServerRoot@@ since > > /manual/ is a subdirectory of /htdocs/ ? > > But /manual/ is not a subdirectory of /htdocs/ anymore (or at least it > doesn't get installed there). That's the whole point. Um... gotta ask Ken about this. He vetoed a change in this area. If the change related to the veto hasn't been backed out, then we may have to yank 1.3.21 Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c
Note that all of this becomes *much* easier if we just switch over to
directly reading the socket.
Cheers,
-g
On Fri, Oct 05, 2001 at 08:54:19AM -, [EMAIL PROTECTED] wrote:
> jerenkrantz01/10/05 01:54:19
>
> Modified:server core.c
> Log:
> Allow the core input filter to handle AP_NONBLOCK_READ request for a
> finite number of bytes (i.e. *readbytes > 0).
>
> ap_brigade_partition does a blocking read. So, what we should do is
> apr_bucket_read on the socket for non-blocking. If we get less than
> what they asked for, that's okay and we should just return that amount.
> If they were non-blocking, we should always be non-blocking.
>
> Ryan, Greg, and others can figure out if ap_brigade_partition should
> be tweaked to handle AP_NONBLOCK_READ natively. I'm of a mixed mind,
> but this addresses the short term need.
>
> Revision ChangesPath
> 1.67 +9 -0 httpd-2.0/server/core.c
>
> Index: core.c
> ===
> RCS file: /home/cvs/httpd-2.0/server/core.c,v
> retrieving revision 1.66
> retrieving revision 1.67
> diff -u -r1.66 -r1.67
> --- core.c 2001/10/05 02:27:48 1.66
> +++ core.c 2001/10/05 08:54:19 1.67
> @@ -2853,7 +2853,16 @@
>apr_bucket *e;
>apr_bucket_brigade *newbb;
>
> +if (mode == APR_NONBLOCK_READ) {
> +e = APR_BRIGADE_FIRST(ctx->b);
> +rv = apr_bucket_read(e, &str, &len, mode);
> +
> +if (len < *readbytes)
> +*readbytes = len;
> +}
> +
>apr_brigade_partition(ctx->b, *readbytes, &e);
> +
> /* Must do split before CONCAT */
>newbb = apr_brigade_split(ctx->b, e);
>APR_BRIGADE_CONCAT(b, ctx->b);
>
>
>
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server core.c
On Fri, Oct 05, 2001 at 12:55:59PM -0700, Ryan Bloom wrote: > On Friday 05 October 2001 12:21 pm, Justin Erenkrantz wrote: >... > > So, I'm thinking a new ap_get_brigade mode called > > AP_BLOCKING_PARTIAL_MODE might do the trick. It tells the core input > > filter that it is okay to return less than that number of bytes. It > > issues *one* blocking read and returns at most *readbytes (or len > > returning from bucket_read if smaller) back in the brigade. I think > > this would make mod_ssl much happier. =) > > Nope. The filters should always been in this mode. A filter should never > wait until it has all of the data that was requested before it returns. A filter > that is called with blocking mode does NOT mean that it must fulfill the entire > request, it just means that it must have at least one byte of data. This is the > same way that blocking reads work with sockets, and filters should work the > same way. Completely agree. That is what I was just explaining to Madhu. The "blocking" mode for ap_get_brigade() *will* return data, but it never has to return all that was requested. It must return a brigade with one byte of data, and/or an EOS bucket. (that is: you could block and just return an EOS; you could also return N bytes plus an EOS) The non-blocking mode could return a zero length brigade. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/docs/conf ldap.conf proxy.conf ssl.conf httpd-std.conf httpd-win.conf
On Thu, Oct 04, 2001 at 08:40:38PM -, [EMAIL PROTECTED] wrote: > wrowe 01/10/04 13:40:38 > > Modified:docs/conf httpd-std.conf httpd-win.conf > Added: docs/conf ldap.conf proxy.conf ssl.conf > Log: > Split the significant modules into segregated module configs. > > Probably should do the same with negotation/autoindex, since those > are _so_ huge, but not this afternoon on my schedule. Woah. No way... we spent a lot of time and pain getting from three configs back to just one. And now we're just reversing that. The ldap.conf shouldn't even be present. That isn't part of httpd-2.0 so why is the config in there? proxy.conf is a "whole" 20 lines or so. Splitting that out is just a damned pain in the butt. Please explain... this just doesn't seem right at all. Multiple configs are nasty nasty nasty. -g -- Greg Stein, http://www.lyra.org/
Re: Configure for 1.3.21 is semi-"broken"
SUMMARY: no worries for 1.3.21; the effect is a less-than-optimal fatal error message. Will patch in a moment. - Whoops... yes, there *is* a logic error there. The logic should be checking for the presence of ./lib/expat-lite Oh... crap. And yes... now I see what you mean about the typo managing to make this work. grr grumble. All righty. My original note stands about the purpose of the check (re: the point about wanting to disable the use of the system expat). But the check there is wrong. This will *not* affect the 1.3.21 release. The check is simply to provide a nice error message when lib/expat-lite is *removed* from the source distribution. We don't do that, and specifically decided against offering that as a choice in *our* distributions (third parties could always choose to do so). Should lib/expat-lite be missing, then in the *default* case, RULE_EXPAT will be set to no, and we'd be set. The real problem comes up with a redistributor or third-party module setting RULE_EXPAT to yes and no system expat, and no expat-lite. In this case, the "nice" error message will fail to trigger, and we'll end up with a failure trying to config/build the (missing) expat-lite directory. Thanks for locating this Jim! Cheers, -g On Thu, Oct 04, 2001 at 11:37:55AM -0400, Jim Jagielski wrote: > Just saw this in Configure after Cliff's note (which is resolved): > > > ## Add in the Expat library if needed/wanted. > ## > > # set the default, based on whether expat-lite is bundled. if it is present, > # then we can always include expat. > if [ "x$RULE_EXPAT" = "xdefault" ]; then > if [ -d ./lib/expat-lite/ ]; then > RULE_EXPAT=yes > else > RULE_EXPAT=no > fi > fi > > if [ "x$RULE_EXPAT" = "xyes" ]; then > if ./helpers/TestCompile lib expat; then > echo " + using system Expat" > LIBS="$LIBS -lexpat" > else > if [ "$xRULE_EXPAT" = "xyes" ]; then > > echo "ERROR: RULE_EXPAT set to \"yes\" but is not available." > exit 1 > fi > echo " + using builtin Expat" > EXPATLIB="lib/expat-lite/libexpat.a" > APLIBDIRS="expat-lite $APLIBDIRS" > CFLAGS="$CFLAGS -DUSE_EXPAT -I\$(SRCDIR)/lib/expat-lite" > fi > fi > > Note the line above the ''. Luckily, this section still works > as required (lucky typo)... But there's a mismatch of logic here, regarding > what the EXPAT rule really means... Right now, if there's a system > expat lib, it will *always* be chosen, and there's no way to bypass > that. Up to now, I think we've always been using expat-lite... Because > of this, it's pretty important that we test out 1.3.21 as well as > possible, especially with those platforms that provide expat. I > don't think there's a need to hold off on 1.3.21 at present though. > > Sorry I didn't notice this sooner... After the dust settles I'll > adjust this section to deal with the new logic. > -- > === >Jim Jagielski [|] [EMAIL PROTECTED] [|] http://www.jaguNET.com/ > "A society that will trade a little liberty for a little order >will lose both and deserve neither" -- Greg Stein, http://www.lyra.org/
Re: Configure for 1.3.21 is semi-"broken"
On Thu, Oct 04, 2001 at 11:37:55AM -0400, Jim Jagielski wrote: > Just saw this in Configure after Cliff's note (which is resolved): >... > Note the line above the ''. Luckily, this section still works > as required (lucky typo)... But there's a mismatch of logic here, regarding > what the EXPAT rule really means... Right now, if there's a system > expat lib, it will *always* be chosen, and there's no way to bypass > that. Up to now, I think we've always been using expat-lite... Because The *intent* is to always choose the system Expat over our bundled version. This also happens to be the same behavior that we now have in apr-util. RULE_EXPAT in Apache 1.3 means "give me Expat". Doesn't matter *where* it comes from, as long as it is available in the process. Point being that we want to use the system's .so when possible, so that other things in the Apache process which need Expat will link against the *same* .so. In the current world, if Apache brings Expat symbols into the process, and something down the line also brings in Expat (because it doesn't know Expat has already been loaded), then we get all kinds of problems. The specific scenario is that mod_perl runs a Perl script which loads the standard XML::Parsers::Expat module; that module knows *nothing* about Apache. All it can know about is the system Expat. Apache has no need to provide its own, so choosing the system version is always preferable. Since the Expat API is way stable, this works quite fine. >... > Sorry I didn't notice this sooner... After the dust settles I'll > adjust this section to deal with the new logic. Um... *why* would you want to disable the use of the system version in favor of the builtin one? I don't see and can't imagine a use case for that. (which is why I coded the selection this way) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] some fixes for ap_http_filter
On Thu, Oct 04, 2001 at 10:01:37PM -0700, Justin Erenkrantz wrote: >... > On Thu, Oct 04, 2001 at 09:37:51PM -0700, Ian Holsman wrote: > > how about we back out the filter changes so that the rest of the > > server isn't broken. > > I'd rather not see us back this change out. But, it is your > prerogrative to veto it. I believe we need to chase this > problem down and fix all of the filters. Agreed. We have taken a large step forwards. Very large, thanks to Justin. I identified these problems back in April or so, but never got the time to carry them through (I started to make some changes once, but for various reasons a good chunk was backed out). Thankfully, Justin found the time and inclination. To back these out now would just return us to the broken state we were in before. Things *happened* to work because we'd hacked stuff up enough to make it work. But the code was *wrong*. Why should we revert to something that we know is wrong? If we want mod_ssl or mod_proxy to work, then we can fix them. They *are* broken, if they have made improper assumptions. But breaking the core to solve their problems is not the right answer. Backing it out and waiting for a complete patch isn't possible either. That would be a huge and complex patch. It would not be reviewable, so we'd just end up applying it without any real review. And hoping for the best... With Justin's patch, we were able to review it, verify that it was a proper step forward, and get it checked in. There were a couple additional patches that he made, based on that review (suggestions from Ryan and myself). The *next* step is to complete the propagation of fixes throughout the code base. >... > Basically, these changes can only go as fast as the people > helping out. And, there's only one of me. =) -- justin Well said. I can happily contribute knowledge, but am so backed up on various projects that helping with coding just falls further and further behind. (sigh) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_ssl input filtering...
On Thu, Oct 04, 2001 at 08:12:42PM -0700, Justin Erenkrantz wrote: > On Thu, Oct 04, 2001 at 09:09:46PM -0400, MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1) >wrote: >... > > 'not necessarily. The renegotiation request can come from the > > ssl_hook_Access() also - in which case ssl_hook_process_connection has no > > business whatsoever.. > > What is the deal if renegotiation is set? It doesn't do anything > of interest, does it? Why can't OpenSSL handle this transparently? To renegotiate, OpenSSL must send data to the client. Since OpenSSL doesn't have a socket, it needs the help of mod_ssl to deliver stuff to the client. That is why the input/output filters are tied together -- you try to read, need to renegotiate, send data to the client, read the result. [ caveat: this is only based on something that I recall Ben saying once ] >... > > I'm a novice here and 'obviously missing something - can somebody tell me > > why should a application not be given whatever it's asking for - especially > > if it's geniune (think SSL) ?.. Also, I guess there has to be a > > differentiator b/w a protocol and a application here.. A protocol should to > > be given all the data it asks for (and in the format it asks for) - the Nope. It asks for X and we'll give it *up to* X. If the app doesn't get the full X, yet it wants more, then it can always call again for more data. This is standard behavior for non-blocking systems (e.g. sockets and pipes). >... > I believe we have to read from the core in determinately-sized > chunks. I don't think we can just say, "Give me everything." Absolutely. The -1 mode can kind of do that, but it is so far beyond bogus that we should not be building mod_ssl that way :-) Therefore, you have to have some kind of buffer size for reading from the next filter. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] mod_ssl input filtering...
On Thu, Oct 04, 2001 at 01:03:32PM -0700, Justin Erenkrantz wrote:
>...
> --- modules/ssl/ssl_engine_io.c 2001/10/04 17:50:39 1.37
> +++ modules/ssl/ssl_engine_io.c 2001/10/04 19:54:22
>...
> static apr_status_t ssl_io_filter_Input(ap_filter_t *f,
> apr_bucket_brigade *pbbOut,
> -ap_input_mode_t eMode,
> +ap_input_mode_t mode,
> apr_off_t *readbytes)
> {
> apr_status_t ret;
> -SSLFilterRec *pRec = f->ctx;
> +SSLFilterRec *ctx = f->ctx;
> +apr_status_t rv;
> +apr_bucket *e;
> +apr_off_t *tempread;
>
> -/* XXX: we don't currently support peek */
> -if (eMode == AP_MODE_PEEK) {
> +/* XXX: we don't currently support peek or readbytes == -1 */
> +if (mode == AP_MODE_PEEK || *readbytes == -1) {
> return APR_ENOTIMPL;
> }
> +
> +/* Return the requested amount or less. */
> +if (*readbytes)
> +{
> +/* churn the state machine */
> +ret = churn_input(ctx, mode, readbytes);
> +
> +if (ret != APR_SUCCESS)
> + return ret;
> +
> +APR_BRIGADE_CONCAT(pbbOut, ctx->b);
> +return APR_SUCCESS;
> +}
> +
> +/* Readbytes == 0 implies we only want a LF line.
> + * 1024 seems like a good number for now. */
> +*tempread = 1024;
*tempread ?? You might want to assign something to tempread first :-)
>...
> +pos = memchr(str, APR_ASCII_LF, len);
How about a new brigade function first?
I don't know much about the rest of how this filter works, but I am
seriously suspicious of the removal of the renegotiation code.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: --enable-ssl=shared
Agreed.
mod_ssl.so should pull in the libraries that it needs, whether they are .a
or .so libraries.
Cheers,
-g
On Wed, Oct 03, 2001 at 12:33:07PM -0400, MATHIHALLI,MADHUSUDAN (HP-Cupertino,ex1)
wrote:
> You're right.. libcrypto/ssl.a should get linked with mod_ssl.so rather than
> httpd. I did try it out initially - with a small hack into the ssl makefile,
> but then gave up - as it involved modifying the autoconf script(something
> which i don't know).
>
> Thx
> -Madhu
>
> -Original Message-
> From: Doug MacEachern [mailto:[EMAIL PROTECTED]]
> Sent: Wednesday, October 03, 2001 8:47 AM
> To: [EMAIL PROTECTED]
> Subject: Re: --enable-ssl=shared
>
>
> On Wed, 3 Oct 2001, Aaron Bannert wrote:
>
> > So you're saying that libssl.so and libcrypto.so aren't showing up
> > when you run ldd on either httpd or mod_ssl.so? Just for reference,
> > what is ldd giving you for httpd and mod_ssl.so? (You may not want to
> > copy it here, but ldd -v is interesting too).
>
> no, mine is linked against lib{crypto,ssl}.a
> there are no .so's in /usr/local/ssl/lib
> i'm just noticing now for the first time lib{crypto,ssl}.so in /usr/lib
>
> but if mod_ssl is built shared, -lssl -lcrypto should get linked against
> mod_ssl.so rather than httpd, right?
>
> % ldd bin/httpd
> libaprutil.so.0 => /home/dougm/ap/prefork/lib/libaprutil.so.0
> (0x40017000)
> libapr.so.0 => /home/dougm/ap/prefork/lib/libapr.so.0 (0x40028000)
> libm.so.6 => /lib/libm.so.6 (0x40052000)
> libcrypt.so.1 => /lib/libcrypt.so.1 (0x40071000)
> libnsl.so.1 => /lib/libnsl.so.1 (0x400a)
> libdl.so.2 => /lib/libdl.so.2 (0x400b7000)
> libexpat.so.0 => /home/dougm/ap/prefork/lib/libexpat.so.0
> (0x400ba000)
> libpthread.so.0 => /lib/libpthread.so.0 (0x400e1000)
> libc.so.6 => /lib/libc.so.6 (0x400f7000)
> /lib/ld-linux.so.2 => /lib/ld-linux.so.2 (0x4000)
>
> % ldd modules/mod_ssl.so
> libc.so.6 => /lib/libc.so.6 (0x40032000)
> /lib/ld-linux.so.2 => /lib/ld-linux.so.2 (0x8000)
--
Greg Stein, http://www.lyra.org/
Re: mod_ssl update...
On Wed, Oct 03, 2001 at 08:28:06AM -0700, Justin Erenkrantz wrote: >... > Most of the other filters have their logic separated by input and > output. mod_ssl combines them into one function. I think that > it makes sense to split it out so that input is only handled by > one function and output is handled by another function. I'm not > sure why we have one function attempting to handle input/output. > Is there a reason why churn() must exist? Input and output are > not related to each other. For SSL, yes: they *are* related. It is possible that OpenSSL may need to send some bytes to the client in order to read some bytes. I think this is tied up with the "renegotiation" stuff. I don't really know *why* it would need to write to the client during a read, but I know it does. That is why Ben designed it as tied-together input and output filters. >... > > I don't think so..As long as we can gather the *full client data* and pass > > it across, OpenSSL is happy.. The catch here is to capture all the data > > that's sent by the client, and not to break it into small chunks/pieces. > > What the real deal is that OpenSSL was assumming that it'd get the > socket bucket back from the core filter and it'd be able to do whatever > it wanted with it. That's not possible anymore. Yup. I'm guessing the logic doesn't like running out of input. When it does, and it can't simply exit saying "this is all I have right now" (e.g. it is mid-step during some protocol bits), then it needs to call ap_get_brigade() again. > If it is possible to have the core do the heavy lifting (i.e. reading > from the socket), that is preferred. I'm just not sure how mod_ssl > is reading from the socket at all. It calls ap_get_brigade *and* > SSL_read - that is a violation of the input filter semantics. Either > it reads from the socket on its own or it relies on the core for all > of the input. I don't think we ever give it the socket. From looking at the code, it appears we do an apr_bucket_read() to get some data, write that into some kind of buffer with BIO_write(), then SSL_read() will return decrypted data based on what it finds in the BIO. > > It's definitely possible to use the SSL_* functions - but, then we'll have > > to expose the socketfd's et al.. Also, it'd be deviating from the other > > modules of apache, where the filters are *almost* forced to be used. I'd > > prefer to have the ap_get_brigade_* functionality to read/write the data > > from the socket. Euh... no. I believe the basic concept is sound. We simply need to fix the interaction between the brigade fetched from f->next and the BIO/SSL functions. > Yes, I'd prefer it to rely on core, but there are things like *readbytes > 0 that are completely bogus in the core with SSL. That's why I'm not > sure we can use the core filter anymore. mod_ssl should never pass readbytes==0 to f->next. As you point out, that is bogus. So the next question is "why does it do that?" Simple answer is that the code is broken. If a caller asks for a line (readbytes==0), then it blindly passes that to the next filter. Instead, it should ask f->next for BUFSIZE bytes or somesuch, decode them, then parse a line out and return that. "How did it work before?" Nobody ever asked it for readbytes==0 through various happenstance circumstances. The API for input filters said that somebody *could* do that, but it just didn't happen. Can you say, "fragile" ? Thought so. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: ssl is broken
ffer of the requested size and do a apr_socket_read() right into that buffer. Wrap a bucket around it and place it into the passed-brigade. Of course, you might not read everything requested, and you might also want to limit the allocation to some maximum number of bytes. (note that the current code uses apr_brigade_partition() which blocks until the requested number of bytes is read) > The CORE_IN filter reads directly from the socket. If you want to provide a > readline and readbytes here, both blocking and nonblocking, that's fine. It > doesn't mitigate the fact that the ssl filter will transform any readline into > read bytes, and that an HTTP filter must handle readline after recieving some > readbytes, and handle ALL the chunking behavior. No problem. > > Ryan and Greg, how do you guys see this working? I yield to > > your wisdom here... If the CORE_IN read from the socket > > directly as you both have suggested, how would (or should) > > mod_ssl interact? Why should mod_ssl care *how* data is read? Why should it care whether the data comes from a socket? Feh. mod_ssl says "give me N bytes" and the *next* filter returns *up to* that number of bytes. That next filter *might* be CORE_IN. CORE_IN reads from the socket, placing buckets into the passed-brigade (per the comment above). > > I see it being a much simpler task to write a module that > > replaces CORE_IN than trying to work around it. It's not No. As Ryan said, "bogus". mod_ssl reads from the "next" filter. It can do that quite fine. Why should it care about sockets? Why should we remove CORE_IN when the SSL filter happens to be installed? > > that much code - and I think mod_ssl could even ditch > > some of the lesser-supported modes (readbytes==-1 and PEEK > > which it already doesn't support). We'll probably end up > > removing them in core at some point. -- justin Those modes should be fixed independent of mod_ssl. The "peek" thing really isn't, and I don't even know whether and how Netscape's "padding" interacts with SSL on the wire (do they still pop out of the encrypted pipe? maybe it knows better than to send them?) It Netscape still puts that \r\n down the encrypted pipe, then mod_ssl would need to deal with seeing that stuff and tossing it. > If you have to replace CORE_IN, then your implementation of CORE_IN is > broken. For that too I would veto the patch you've committed, if this > can't be fixed to allow the old functionality. It doesn't have to be replaced. *PLEASE* ... Ease up on your veto threats. Take a breath and stop to learn what is happening. > Maybe filtering needs some pushback so each higher level filter can avoid > hanging on to bytes it doesn't want or need, and push them back at a lower > level filter that can keep them in their queue for the next transaction > within the request or the next request within the connection. At this point, we don't need any concept of pushback. A filter should return up to what was asked and no more. But I seriously doubt that is the problem here. I'm guessing that mod_ssl was coded "knowing" that the brigade it got back from f->next was a socket bucket. Or that the brigade had certain properties when reading from it. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: ssl is broken
On Wed, Oct 03, 2001 at 02:34:57PM -0700, Roy T. Fielding wrote: >... > I think we have to get off the notion that recent modifications have > broken mod_ssl Hell yah. I'm going to follow up to this thread in a sec, but just wanted to chime in with a high-level comment here. Justin's recent changes have *improved* the input filter stack. The problems that we're seeing at higher levels are because those higher levels made assumptions. At the moment, the various filters are all coupled together, when they should be completely independent. > -- it has yet to work correctly with filters. Any module > that makes implementation assumptions about the implementation of the > upstream filter, or the nature of the data being passed through the filter, > is broken, and I personally think that includes every implementation of > filters in httpd. That is why we have a showstopper item about rethinking > the input filters. The output filters seem to be quite fine. The input filters have problems, but Justin just made a big step in fixing them. There are a couple more steps to do. The problem is that people are screaming about the first step and saying "back up" rather than seeing the target and saying "take the next two steps" Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: BaseAddress.ref needed?
On Wed, Oct 03, 2001 at 09:56:40AM +0200, Günter Knauf wrote: > Hi Bill, > can you please explain if every module really needs an entry in BaseAddress.ref? I >tested with many modules without an entry and it seems to work with the linker >defaults... > If it is needed how should it be done then with 3rd party modules? Yes, it is needed. Without it, the linker will assign the same, fixed address to every module. At runtime, those modules will then need to be relocated to some address. Using BaseAddress.ref, we can effectively do the relocation at link time. Sure, it is possible some other code is present and a relocation is forced, but that will be atypical. Relocation is expensive. Doing it at link time rather than load time is a huge win. 3rd party modules can do whatever they want. Invariably, they will probably get relocated and/or clash with other 3rd party modules. But we *do* have control over Apache itself and can help those. There are further optimizations with assigning addresses that we probably don't do. Specifically, if there are "gaps" in the address space, then you end up wasting space in the processor's mapping tables. As a result, you want to pack the modules as tightly as possible. Of course, that tight-packing is at odds with the optional loading of modules. If we do the packing, then decide not to load something, then we end up with a hole. [ and you can extend that to third party modules if we attempt to act as a registry for them. it goes without saying that people won't have every third party module, so each apache process would have *huge* holes, thus wasting significant processor tables ] Does that answer your questions? :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: ssl is broken
On Tue, Oct 02, 2001 at 08:34:30PM -0500, William A. Rowe, Jr. wrote: > From: "Justin Erenkrantz" <[EMAIL PROTECTED]> > Sent: Tuesday, October 02, 2001 8:29 PM > > > On Tue, Oct 02, 2001 at 01:24:25PM -0700, Doug MacEachern wrote: > > > with current cvs all httpd-test ssl tests hang, stacktrace is same for > > > all... > > > > I bet it's related to the input filtering changes. > > > > I have no clue how the whole mod_ssl input filtering works, but I'll > > try to take a look. My guess is that mod_ssl is just a big quagmire. > > That, or your input filtering changes were the big quagmire. Either way, > that sure sounds like the interaction that broke ssl. I'll lay good odds that it is in mod_ssl rather than the input filtering changes. Even *if* it is the latter, at least we have a codebase that is much more maintainable now. Cheers, -g -- Greg Stein, http://www.lyra.org/
who is using the Expat bundled with Apache 1.3?
When Expat was added to Apache a couple years ago, part of the impetus was to make XML parsing a standard feature for the web server -- modules could count on it being present for their use. So now the question arises, who is using the Expat that is included with Apache 1.3? Was the goal of enabling XML usage successful? Note: this is only relative to Apache 1.3; the basis for the query is related to some technical problems with its inclusion in Apache; Apache 2.0 doesn't have the same problems since it will prefer an installed Expat when possible. Note 2: this is just an informational query; I'm not sure that we'd rip it out (there could still be people who don't regularly read this list); I think the answer is to fix Configure to find the system lib when possible. Cheers, -g -- Greg Stein, http://www.lyra.org/
[ejw@cse.ucsc.edu: RE: Protocol Action: Versioning Extensions to WebDAV to Proposed Standard]
FYI >From here, some minor edits (grammatical and the like) go into the draft, then it is assigned an RFC number. Cheers, -g - Forwarded message from Jim Whitehead <[EMAIL PROTECTED]> - From: "Jim Whitehead" <[EMAIL PROTECTED]> Subject: RE: Protocol Action: Versioning Extensions to WebDAV to Proposed Standard To: <[EMAIL PROTECTED]>, "WebDAV" <[EMAIL PROTECTED]> Date: Mon, 1 Oct 2001 10:58:35 -0700 Yes!!! What excellent news. Five and a half years ago we began working on the vision of the Web as a writeable medium, one that could be used for remote collaborative development of software projects, large documentation efforts, and other large clusters of inter-related documents. With the approval of the DeltaV protocol as a Proposed Standard, we have taken a large step forward towards making this vision a tangible reality. Geoff Clemm receives my thanks and respect for the key role he played in the design of the DeltaV protocol, and in pushing it forward for so long. However, it was clearly a collaborative effort, involving the Chair, Jim Amsden, the document authors, the DeltaV design team, and the DeltaV working group. All of your tireless work, sending posts, making reviews, clarifying points, led to a high quality technical standard. Specification in hand, we are now well along the rough road to the common byte. I look forward to working with you all in the months and years ahead to ensure this specification leads to broad interoperability. - Jim > The IESG has approved the Internet-Draft 'Versioning Extensions to > WebDAV' as a Proposed Standard. > This document is the product of the Web Versioning and Configuration > Management Working Group. The IESG contact persons are Ned Freed and > Patrik Faltstrom - End forwarded message - -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/modules/generators mod_status.c
On Mon, Oct 01, 2001 at 03:51:07PM -, [EMAIL PROTECTED] wrote: > wrowe 01/10/01 08:51:07 > > Modified:modules/generators mod_status.c > Log: > Clean up some warnings by summing bytecounts into apr_off_t holders > instead of ulongs. Ah! Good stuff. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server protocol.c
On Sat, Sep 29, 2001 at 08:48:59AM -, [EMAIL PROTECTED] wrote: > jerenkrantz01/09/29 01:48:59 > > Modified:modules/http http_core.c http_protocol.c >server protocol.c > Log: > Remove the lameo create_req hack and delay the addition of the HTTP_IN > filter until after we have read the headers. This eliminates the status > hack that was in http_protocol.c and makes it all around better. > > server/protocol.c now directly adds HTTP_IN filter - should we create a > specific hook for this? (Could we do this as a post_read_request hook?) > I'm not terribly sure, but let's move it down to the lowest possible > place in ap_read_request. We can change this detail later as we see fit. Don't worry about it. As long as protocol.c deals with HTTP (specifically, it manages the reading of the headers, among many other things), then there isn't a problem with adding the HTTP_IN filter. If somebody wants to make protocol.c "pure", then they're going to have a lot more to worry about than the HTTP_IN thing. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Tag time?
On Fri, Sep 28, 2001 at 06:31:57PM -0400, [EMAIL PROTECTED] wrote: >... > Ah... Hate to butt in here but if the Justin's original reason for > rewriting the entire socket input 'front door' was (as he said ) > because of something about it NOT handing client->host > Transfer-encoding very well then I'm not sure it isn't worse off now > than it was before. The code was to clean up a bunch of problems in the input filtering. There shouldn't be any real functional change (and I didn't see any). However, adding input filters should actually have a chance to work now. > I have been looking and looking at the patch and someone want > to tell me where it checks for TE: which is the only way to > REALLY know how the Transfer-Encoding will end? ( Blank > CR/LF following CR/LF following 0 byte length, or no? ). It did not before, so it doesn't now. For the most part, this is a rearrangement of code. There is a lot of cleanup and rationalization to the code. It should be much easier to fix/extend the code now. > Near as I can tell it just relegates 'extra' CR/LF back to stream > as 'useless noise' instead of actually knowing for sure if it > was a proper end to the Transfer-Encoding. Yup. > Caveat: I have NOT had time to REALLY pour over this HUGE > patch so I may be totally off-base... but I think this fact alone > is what Ryan is trying to explain... there hasn't been enough > TIME to look at this HUGE/CRITICAL patch. It *has* been reviewed. Given that we're in commit-then-review mode, and that two people are fine with the patch, then it can/should go in. If further problems are discovered in the patch, then they can be fixed in the repository rather than before the patch is applied. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Tag time?
On Sat, Sep 29, 2001 at 11:50:27PM +0200, Eli Marmor wrote: > Justin Erenkrantz wrote: >.. > > I'd like to get the new input filtering code in. It's a major change, >... > Input filtering in mod_ext_filter is done only partially, and commented > out. > > Is it going to be finalized too? Note: modules/experimental/mod_ext_filter That "experimental" in there means just that :-) ... it could be updated if somebody wants to. Or it could remain the same... :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-proxy/docs mod_proxy.html
On Thu, Sep 27, 2001 at 01:33:10AM +0200, Graham Leggett wrote: > [EMAIL PROTECTED] wrote: > > > add the ProxyHTTPOverrideReturnedErrors directive documentation > > Would anyone object to me changing this to "ProxyErrorOverride" - the > above directive is *way* too long... +1 -- Greg Stein, http://www.lyra.org/
Re: [PROPOSAL] apr-client
On Wed, Sep 26, 2001 at 04:22:46PM -0700, Roy T. Fielding wrote:
>...
> > Nah. This has utility outside of httpd. Specifically, Subversion is an
> > excellent candidate. I also know that Covalent has a similar library that
> > they use internally ("apua", I believe). So that is candidate #2. And then
> > you have your flood program for candidate #3. Of course, mod_proxy would use
> > it, but that can fall under Roy's httpd placement model.
> >
> > And then there are the ones we don't know about...
>
> All the more reason why it shouldn't be part of the Apache Portable Runtime
> project. If you want to create a new project for it, that's fine with me,
> but the only thing it has in common with APR is that it will use APR.
Hmm. Good point, but I think it also depends a bit on how you look at APR.
As a "Runtime", it can contain any kinds of functionality. And the
"Portable" is just an adjective.
Alternatively, if you have a "runtime to create portability" ... then yah:
your point would hold.
Just playing the devil's advocate :-)
I'm still in favor of apr-client. If we want to rename the top-level CVS
directory at some point, then fine...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PROPOSAL] apr-client
On Wed, Sep 26, 2001 at 04:15:30PM -0700, Aaron Bannert wrote: > On Wed, Sep 26, 2001 at 03:52:18PM -0700, Greg Stein wrote: > > > FWIW, flood in httpd-test already has 80% of what Sander suggested. > > > > Sander and I already talked about stealing code from flood and mod_proxy. :-) > > *ahem* ;) > > In all seriousness, I think we've already got most of the code we want. > (Flood already does or can do some of the things Sander suggested). Yup. However, (IMO) we would be started from the API and moving code to fit that API. That is going to impose some difficulty in terms of stealing code, but *any* prior work is going to ramp up apr-client quickly. >... > I have no problem with either the push or pull methods, but I wanted to > make the point that flood could benefit from both request generation > brigades/filters as well as response filtering. Excellent. It would be kind of cool for flood to get out of the socket business and concentrate on its purposes: exercising, regression, and testing. >... > > Absolutely. That is why my previous email mentioned that I'd be happy to see > > a directory / cvs module get started so a roadmap could be created. I would > > want to see the API first, built from our expertise with the brigades and > > filtering systems, and with a lot of the ideas embodied in Neon's APIs. > > +1 > (once we get it rolling we can move this kind of discussion there and > start writing design docs) I'm guessing that we would keep the discussion on the dev@apr list rather than spinning up another list. But I have no strong opinions there. >... > I don't really care where it goes as long as it can be used independently > of the core httpd source. Independent of httpd effectively means APR. I guess you could be an httpd subproject, but this has nothing to do with an HTTP server. [ there is no way the board would establish a new PMC for this, speaking as one of those board members :-) ] Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PROPOSAL] apr-client
On Wed, Sep 26, 2001 at 03:33:12PM -0700, Justin Erenkrantz wrote:
> On Wed, Sep 26, 2001 at 03:21:42PM -0700, Greg Stein wrote:
> > But that said, I'm am a BIG +1 on adding an http client library into the
> > ASF's APR project. Whether people want that to go into apr-util or into a
> > new apr-client, I'm not too concerned.
>
> FWIW, flood in httpd-test already has 80% of what Sander suggested.
Sander and I already talked about stealing code from flood and mod_proxy. :-)
> The major thing it is missing is bucket/brigade/filters - which is
> why I'm getting pissy about the API change because I want to move
> flood to this. (It is also missing chunking and pipelining which I
> was working on when I got interrupted when I saw how httpd handles
> it and started to rewrite the input filtering.)
>
> The one thing I would change in Sander's proposal is that I detest
> callbacks.
It should not be hard to build it such that push or pull models could be
used here.
There are four points of push/pull:
1a) app pushes request into apr-client, which then transfers it onto the
network. this is like the output filter chain of httpd.
1b) app gives a callback to apr-client, which it uses to pull data and
deliver it to the network. this is similar to httpd pulling data via the
input filter chain.
2a) apr-client pushes response to an app's callback
2b) app pulls response from apr-client as it becomes available
An application can choose 1a or 1b, and 2a or 2b for how it handles the
request/response process. The question boils down to "who is in control?" In
1a and 2b, the app is in control. In 1b and 2a, the library is in control
(based on network conditions).
When Sander mentioned callbacks, he was really referring to 2a. (that aspect
came up in our conversation; we didn't discuss 1b much).
> And, I think the API would need to be a bit more formal.
Absolutely. That is why my previous email mentioned that I'd be happy to see
a directory / cvs module get started so a roadmap could be created. I would
want to see the API first, built from our expertise with the brigades and
filtering systems, and with a lot of the ideas embodied in Neon's APIs.
> And, the build process cleaned up. And, more testing.
> Yadda, yadda, yadda.
Of course :-)
> And, in discussions with Roy, I think he was thinking a client
> library should be a part of httpd not APR. But, I don't care
> one way or another. -- justin
Nah. This has utility outside of httpd. Specifically, Subversion is an
excellent candidate. I also know that Covalent has a similar library that
they use internally ("apua", I believe). So that is candidate #2. And then
you have your flood program for candidate #3. Of course, mod_proxy would use
it, but that can fall under Roy's httpd placement model.
And then there are the ones we don't know about...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PROPOSAL] apr-client
On Wed, Sep 26, 2001 at 11:14:05PM +0200, Sander Striker wrote: > Hi all, > > I wish to propose a new library: apr-client. btw, I'm +1 on all points in here. Sander and I talked about this quite a bit last night over IRC. I'm hoping for a general go ahead, a creation of a STATUS and or roadmap/design [for apr-client], and then being able to dump a bunch of stuff in there. Note that I believe a simple client could be scrapped together in a couple evenings. It could support sessions, pipelining, multithread, brigades, and filters. Adding the auth, SSL, and response [XML] parsing is the bulk of the work. >... > The filters are a nice area for code reuse. The logic > for the client is ofcourse reversed, so we can have > a chunk filter on the request side and a dechunk on the > response side. This is just an example, replace > chunk/dechunk with gzip/unzip and you have another. > Same goes for ssl. This way the same code gets used in > multiple places which results in more extensive testing > of this code (which is a nice side effect). This would be good. However, I believe that it would probably be a good amount of work to convert Apache's filters over to use apr-client's filters. IMO, the latter's filter system will be a bit simply (in particular, NO references to things like f->r and f->c). That implies a design difference in the two systems. So: excellent goal, but some recognition of the work is needed, too. > You might ask yourself why the world would need yet > another http client library. Well, there seem to be > only two good ones: curl and neon. Curl is bloated > (has ftp, gopher, etc support and is more a general > network client library). Agreed. > Neon is good, but LGPL. Also, > it doesn't tie in nicely with apr (example is malloc/free > usage in it, which requires the user to malloc a chunk > of mem, fill it, pass it to neon which then frees it). The license is a big one for me (having apr-client and its ASF license means we could replace proxy code with it; LGPL is a tougher decision for bundling with ASF code). The integration isn't so big in my mind, but an APR-based library would certainly eliminate a lot of dupliation (prolly 40% of neon deals with stuff like allocation, strings, md5, base64, etc). And it would also enhance portability. Also, Neon does not yet support pipelining (Joe intends to, but it will be a big chunk of work). Those four items (license, reduce dup, pipelining, portability) push me towards the +1 here. That said: Neon is an excellent library. I do and would continue to recommend it to people. I don't see SVN switching away from it for 1.0. But post 1.0, picking up the pipelining and the tight APR integration will be big win. [unless apr-client somehow managed to stabilize really fast] I just think that the ASF projects could use it, and I think getting a nicely licensed, fully capable client library would be great. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PROPOSAL] apr-client
Umm... Ryan... *what* are you talking about? :-) I do not recall ever discussing a client library in there. Nor as a motivation for APRUTIL. I'm happy to be disproved on that :-), but I'd rather see a separate library started for this. An http client library is a big chunk of code. I just counted the LOC in aprutil just now. 24 kLOC. I would estimate that apr-client to be in the neighborhood of 7k to 10k LOC. That is a *big* change over today's apr-util. But that said, I'm am a BIG +1 on adding an http client library into the ASF's APR project. Whether people want that to go into apr-util or into a new apr-client, I'm not too concerned. Let's say: +1 on creating apr-client -0 on adding to apr-util [ but +1 if apr-client rejected :-) ] Cheers, -g On Wed, Sep 26, 2001 at 02:46:44PM -0700, Ryan Bloom wrote: > > There is supposed to be an http client library as a part of apr-util. > It's just that nobody has actually written it yet. I would prefer to not > create another library for this, because this client lib was one of the > reasons that apr-util was created in the first place. > > Ryan > > On Wednesday 26 September 2001 02:14 pm, Sander Striker wrote: > > Hi all, > > > > I wish to propose a new library: apr-client. >... -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 3 of httpd filter rewrite...
mode)) != APR_SUCCESS) {
> +return rv;
> +}
> +
> +pos = memchr(buff, APR_ASCII_LF, len);
Please move the declaration for pos into this block. I had to search a long
ways back to find that one. And it should probably be const char * also.
>...
> --- modules/http/http_core.c 2001/08/25 23:43:18 1.282
> +++ modules/http/http_core.c 2001/09/24 20:46:51
>...
> +static int ap_http_create_req(request_rec *r)
> +{
> +if (!r->main)
> +{
> +ap_add_input_filter("HTTP_IN", NULL, r, r->connection);
> +}
> +return OK;
> +}
Per my previous email, I think this should be inserted at a different point
in the code flow. It will also remove the need to hook create_request.
>...
> --- modules/http/http_protocol.c 2001/09/17 13:12:37 1.362
> +++ modules/http/http_protocol.c 2001/09/24 20:46:51
> @@ -481,265 +481,117 @@
> return AP_HTTP_METHODS[methnum];
> }
>
> -struct dechunk_ctx {
> -apr_size_t chunk_size;
> -apr_size_t bytes_delivered;
> +static long get_chunk_size(char *);
> +
> +typedef struct http_filter_ctx {
> +int status;
See previous comments about "status". Basically: this is a nasty hack and
the comments (or lack thereof!) do nothing to note that, nor to elucidate
what is going on.
>...
> +BODY_NONE /* must have value of zero */,
Doesn't need to be zero... (see comments before)
>...
> +apr_status_t ap_http_filter(ap_filter_t *f, apr_bucket_brigade *b, ap_input_mode_t
>mode, apr_off_t *readbytes)
Wow. This function seems like it is going to be dramatically simpler. Hard
to see through all of this mess, though :-)
I'm +1 on the patch, but with the caveat that *many* more comments need to
be added. There are still hacks all over the place (from before Justin's
patch, and at least one new one). I tried to comment the hacks before;
thankfully, Justin's patch removes most of those. But there are still more,
and we can't just let those slide...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: proxy - a return.
On Tue, Sep 25, 2001 at 05:45:40PM -0700, Ryan Bloom wrote: > On Tuesday 25 September 2001 04:13 pm, Graham Leggett wrote: >... > > Right now, what is the best way of returning mod_proxy to the tree? Is > > it > > > > a) checking in the latest copy of proxy, relying on the old httpd-proxy > > tree for history. > > b) moving the ,v files across so that history is carried forward into > > httpd-2.0 tree? > > B. That history is incredibly important since proxy is basically a complete > re-write for 2.0. > > Of course, Greg is likely to disagree with me (this is one of our long-standing > disagreements), so wait for him to respond too, please. Thanks for the consideration, Ryan... Well, an import is a bit different than moving files around in the tree. I tend to advocate "cvs add" for moving files, rather than mucking with ,v files. Moving or copying ,v files means that files can appear when you check out old copies (by tag if you don't remove them, but a checkout-by-date will always produce spurious files). But bringing files into a tree is a bit different, and having that history can be handy. Of course, now we get into the ugly part. httpd-2.0/modules/proxy/Attic contains a bunch of files. A given ,v file cannot exist in *both* the directory and its Attic. Thus, introducing (say) mod_proxy.h into modules/proxy means that it must be removed from the Attic. Thankfully, it appears that we moved all that history over to httpd-proxy. Which, in turn, *may* mean that there is nothing in the Attic that is not already in the httpd-proxy history. But "may" is the operative word. The head revision of the files in proxy/Attic are where we deleted them. The safety check is to look at HEAD-1 and see if that revision matches the same over in httpd-proxy. What we are looking for is changes in httpd-2.0/modules/proxy/Attic which are not in httpd-proxy. Should we find any, then we need to decide what to do with them. Next is whether any files were deleted over in httpd-proxy. The files would also need to be moved over to modules/proxy/Attic (with the similar care for any overwrites). Are we having fun yet? :-) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Worker MPM defaults
On Mon, Sep 24, 2001 at 04:02:45PM -0700, Ryan Bloom wrote: >... > We don't have owners of code. We are a team of people working on a single > project. Giving one person ownership implies that they have more control > over that section of code than somebody else. I completely agree with Ryan. While some people may argue that isn't the best thing for the code, it *is* the best thing for the group and the community. And (IMO) that is at least as important as the code, if not more. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Apache 2.0 release plan
On Mon, Sep 24, 2001 at 01:45:32PM -0700, Brian Pane wrote: >... > To me, a useful release plan is one that: > * tells developers working on httpd-2.0 what the important things to > work on for 2.0-GA are, And the developers will continue to work on what they *want* to work on. There is no hierarchy, and no task assignments in (most) open source projects. If somebody is interested in getting a release done, then they will identify the showstoppers (which are ideally already listed in STATUS) and work on those. The hope is that non-release-interested developers are not getting in the way of release developers. I believe that is the case. Nobody is really committing drastic revamps to the core without some review (e.g. Justin's input filtering fixes). Changes to modules can (almost) never impact the release stability. > * tells 3rd-party module developers what sort of interface they can > expect, and Those can change up until release, and even after that. Hopefully, we keep that at a minimum, but Apache is about writing the best code possible. And that can mean that interfaces may have to change. > * tells end users what feature set and performance characteristics > they can expect. We aren't organized enough to do that. That requires a management process, which is more or less forbidden :-) > The "tag when you believe it is appropriate" model is an effective process > for software builds, but not necessarily for product releases. Depends upon your point of view. Given the circumstances, I believe it to be very effective. It is simply that Apache has had so much change over the past couple years, that we still are not at a stable spot. For example, we're still finding interesting things to do with the brigades. We're still refining their functionality. We've got architecture issues with input filtering. And that is just one piece of the code. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Mon, Sep 24, 2001 at 12:27:21PM -0700, Justin Erenkrantz wrote: > On Mon, Sep 24, 2001 at 03:14:22PM -0400, Cliff Woolley wrote: > > On Mon, 24 Sep 2001, Justin Erenkrantz wrote: > > > > > The brigades should not be returning more data than is asked for > > > ever. > > > > Right. But the only increment the API allows to ask for are one whole > > bucket's worth at a time. [ cliff referring to buckets and brigades; the real problem with the filters not the brigade API ] > util_filter's ap_get_brigade takes in the length to read. The Unfortunately, it takes it as a pointer-to-length. That is wrong -- it should just be an apr_off_t length value. I changed that at one point in April or so. However, that broke something else and Ryan reverted most of the patch. I forget the full reason there, but it was definitely related to some of the other issues we've been discussing around your patch over the past couple days. > translation from httpd->apr_util in the core filter loses this > distinction. The core filter should be splitting the buckets and > only returning the requested amount to HTTP_IN (and it'd have to > handle the 0 case when we want to read a line). Absolutely. And to support the 0 case, I suggested a set of brigade functions to help out: apr_brigade_getline(bb, line, sizeof(line)) apr_brigade_pgetline(bb, &line, pool) apr_brigade_splitline(bb, &newbb) The first two would use the last function to implement the getline process. The reason for the first two forms is for memory management issues. You may not want to copy the line into a pool, but on the other hand, you may want the whole line even if it doesn't fit your buffer. Note on the getline() case: you'd need to distinguish between "the line filled the buffer" and "the line is longer than the provided buffer". Maybe a returned length could indicate that somehow. This getline stuff is in the httpd STATUS file under the item for revamping the input filter system. By making brigade functions, you could do a getline anywhere in the code. Consider the case where you have a decompression filter and a higher filter asks for a line. The filter would decompress into a buffer or a brigade, then see the "0" and need to return a line. Having the function available makes it easy for him. If you isolate the line-from-brigade functionality to CORE_IN, then it will limit what others can do. > Since the > core is inherently connection-based, I see no reason why the > buffering can't be done there. Currently, it passes the > socket bucket up the chain. It certainly can. Zero complaints here. Hell... loud applause if you are going to spend the time to fix it! >... > The problem is that HTTP_IN has the socket bucket to begin with (which > has -1 length - i.e. indeterminate). In order to properly handle a > request-centric HTTP filter, that filter must not get the -1 bucket. > It must be hidden by the core filter. Yup. Note that (strictly speaking) you may not even need the socket bucket. You could read straight from the socket, create a bucket around that, and insert the result into the passed-in brigade. If you use a socket bucket, then you're going to have to have an internal brigade. Then you're going to read, split, and shift some buckets from your internal brigade over to the passed-in brigade. YMMV, but consider the case of avoiding a socket bucket. > (This is the way I see it...) -- justin Cool. Me too :-) Like I said, I was just hoping that your patch would not require all of this extra work. That it could be applied as an incremental improvement. But, alas... that doesn't seem to be the case. So the first step would be adjusting the return amounts, then applying your patch. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Mon, Sep 24, 2001 at 11:58:40AM -0700, Justin Erenkrantz wrote: > On Mon, Sep 24, 2001 at 01:32:02AM -0700, Greg Stein wrote: > > Eww. No... just move the filter back to the connection and you're fine. We > > can make it a request filter later on. > > Actually, you can't make it a connection-based filter because there > is no way to retrieve any request information (such as determining > what the body type is) from a connection filter. So, we can't merge > dechunk and http_in unless we have access to the current request. > This means that HTTP_IN must be a request filter in order to get it > to understand HTTP. Hmm. All right... > What is the problem with using the pool's userdata? I don't see > the "Eww" that you guys do. Or, what is the problem with getting > the socket to behave appropriately (i.e. I want 10 bytes, give me > 10 not 8192)? It is a hack. I see it as using the userdata as a global variable. You drop off some data and go pick it back up later on. The code and data flow is obtuse and non-obvious. When somebody sees the read or write to the userdata, they're going to go, "what is going on?" > The brigades should not be returning more data than is asked for > ever. Fix the wording: not the brigades but the *filters*. And yes: I totally agree. I've said that a few times recently :-) I was just hoping that your patch could be done independently of the limiting thing. And that we could go fix the limited-return in a future step. > What happens then is that we now enter a condition where the > caller may not be able to handle the data (i.e. I only wanted 10 > bytes you gave me 8192, oops). This is fundamentally incorrect - > changing scopes doesn't seem to be the answer. -- justin All right. If you're willing to tackle the limited return, then that would seem to be a good first step. The second step would be the patch you just posted. IOW, could we get "limited return" first, then the reorg patch second? Or are the two things too comingled right now? Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
const char *tenc, *lenp;
> +tenc = apr_table_get(f->r->headers_in, "Transfer-Encoding");
> +lenp = apr_table_get(f->r->headers_in, "Content-Length");
> +
> +if (tenc) {
> +if (!strcasecmp(tenc, "chunked")) {
> +char line[30];
>
>...
> +if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> +return rv;
> +}
> +ctx->state = BODY_CHUNK;
> +ctx->remaining = get_chunk_size(line);
The above section of code, and some of the stuff following is duplicated
from ap_setup_client_block(). There should be a comment about this
duplicated code. Especially because this copy doesn't contain all of the
error checking and responses of the ap_setup_client_block version. A next
step would be to figure out how to remove that duplication.
Specifically, I'd think that we figure out some of this information when we
insert the [request form of this] filter. The ap_read_request() function
would set up the filter context and set it when the filter is added; the
context would indicate the particular parameters for body reading.
[ maybe at some point in the future, the filter can read the request line,
headers, body, etc, and do all of the computation and checking itself ]
>...
> +else if (ctx->state == BODY_CHUNK)
> +{
> +char line[30];
> +
> +ctx->state = BODY_NONE;
>...
> +/* We need to read the CRLF after the chunk. */
> +if ((rv = ap_getline(line, sizeof(line), f->r, 0)) < 0) {
> +return rv;
> +}
Is the BODY_NONE thing so that the ap_getline() recursion thingy works
properly, and returns a line?
>...
> +/* This had better be equal to zero now! */
> +if (ctx->status)
> +ctx->remaining -= total;
What better be zero? status or remaining?
I'm not sure why ctx->status would ever be zero. Could you explain? Is that
at some point where r->status becomes OK (rather than HTTP_OK). A lot of
stuff seems to key off ctx->status != 0. I don't see why...
>...
> AP_DECLARE(long) ap_get_client_block(request_rec *r, char *buffer, apr_size_t
>bufsiz)
Probably just leave this be, since we don't have the apr_brigade_to_buffer
function for now.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Sun, Sep 23, 2001 at 09:35:31PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 09:22 pm, Justin Erenkrantz wrote: > > On Sun, Sep 23, 2001 at 08:34:00PM -0700, Ryan Bloom wrote: >... > I don't see this at all. It is not possible to specify how much is read from the > socket itself. I just read through the bucket_socket code, and we always > try to read APR_BUCKET_BUFF_SIZE. This seems to mean that we are > going to read more from the socket than we should, and the second request > will be lost. Ryan's quite right. >... > > register a cleanup with the request pool that setasides everything > > that was read but not processed (i.e. all buckets except for the > > socket) to now live as long as the connection pool. The question > > now becomes how do we retrieve this data back. My first guess would > > be to store it via a pool userdata option, but maybe we could shove > > it back down into the core (which definitely breaks some abstractions). > > So, I think I'm leaning towards the userdata - use "HTTP-IN-" > > to specify the uniqueness should work. Eww. No... just move the filter back to the connection and you're fine. We can make it a request filter later on. > No. I fought against this VERY strenuously last time, and I will fight it again. Me, too :-) > This is why it needs to be a connection filter. To keep the abstraction valid, > the data being read from the socket is stored in the filter's context, the same > way it is for the output filters. For now, yes: a connection filter is appropriate. Later, when we can guarantee that the request-level filter will never read past the end of the request (i.e. it never requests more from the lower-level input filter), then we can move it to a request filter (which, as I mentioned before has interesting properties). Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Sun, Sep 23, 2001 at 09:27:58PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 09:21 pm, Greg Stein wrote: >... > > There is a create_request hook which inserts the HTTP_IN filter when the > > request is created. > > > > This seems like a good idea: each instantiation of the HTTP_IN filter is > > for *one* request and one request only. We can be much more sure that state > > is not spanning requests. > > No, this is a bad idea, and I still don't see how it works for pipelined requests. > The problem is that it is impossible to make sure that you only read a specific > amount from the socket. Whoops. Hell ya... you're so right. All right. Until CORE_IN can return only a specified amount, the new, combined filter should remain a connection filter. The associated per-request logic in this patch should go away. The code will put the socket bucket (and other unread/unused data) into the filter's context, which will survive until the next request. Justin... want to try a third pass? :-) btw, does the test suite actually try out persistent connections? There was a problem with persistent connections for a good long while, until I fixed it last week. (while I like the flatten function, maybe we can leave that out of this patch for now; shoot for that one at another time, and replace similar uses across the board (rather than one spot)) In some future pass, we can reinstate the per-request behavior, but that is a discussion for another time... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Rework httpd buckets/filtering
On Sun, Sep 23, 2001 at 03:04:54PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 02:53 pm, Greg Stein wrote: > > On Sun, Sep 23, 2001 at 08:33:07AM -0700, Ryan Bloom wrote: > > > None of this can be committed until it has passed all of the tests in the > > > perl test framework. The last time somebody (me) mucked with these > > > filters it broke all of the CGI script handling. I would also like to > > > hear from Jeff about this, because when we wrote the filters originally, > > > we had input filtering use this design, and Jeff and Greg were adamant > > > that filters needed to be able to return more than they were asked for. > > > > Euh... not sure if you typoed. Input filters should *NOT* return more than > > they were asked for. Never. > > > > If a filter did that, it could end up passing data up to a filter that is > > not responsible for that data. For example, reading past the end of a > > request, or reading past the headers yet that portion of the body was > > supposed to go into a newly-inserted unzip or dechunk filter. > > > > I can think of more examples, if called upon :-) > > And I am saying, that was the original design, and Jeff Trawick and Greg > Ames Ah. An unqualified "Greg" in your post :-) Shouldn't he be OtherGreg, following in OtherBill's footsteps? hehe... > posted a couple of examples that proved that design incorrect. The typical example that I recall was related to decompression filters. "If my caller asks for 100 bytes, then I read 100 bytes from the lower level, and decompress to 150 bytes, then my caller is going to receive 150 bytes." The correct answer is "return the 100 bytes they asked for, and set the other 50 aside for the next invocation." Consider the case where you have a multipart message coming in, and the entire message is compressed (a Transfer-Encoding is used for the message; Content-Encoding is used to compress the entities in the multipart). Next, let us say that each multipart is handled by a different filter (say, because they are handling different Content-Encodings, such as base64 or quoted-printable). To apply some symbology here: M is the message-body filter which is decompressing the message according to the Transfer-Encoding header. E1 and E2 are the entity-related filters, handling each entity in the multipart message. Now, let's say that E1 asks for 100 bytes. M returns 150. But E1 *only* wanted 100 bytes. Those other fifty were supposed to go to E2. Oops :-) Instead, M should decompress the incoming data, return 100 (to E1) and put the other 50 aside for the next filter invocation. E2 comes along and asks for 300 bytes. We return the 50 we had set aside. (M might decide to read more data, but it is also allowed to return less than asked for, assuming the caller will simply ask again for more) If Jeff/OtherGreg :-) have an example where returning more than asked is *required*, then it should be brought up again. I can't imagine a case where it would ever be safe to return more than is desired. [ other examples may relate to character decoding, where you want to avoid returning a partial multibyte character, so you return N+3 to get the rest of the character in the response; I say that you return N-1 and put rest of the character aside for the next invocation ] Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Take 2 of the http filter rewrite
On Sun, Sep 23, 2001 at 08:34:00PM -0700, Ryan Bloom wrote: > On Sunday 23 September 2001 06:58 pm, Justin Erenkrantz wrote: >... > > Other significant changes is that the HTTP_IN (ap_http_filter) is > > now a request-level filter rather than a connection-level filter. > > This makes a lot more sense to me. +1 >... > I don't see how this works at all for pipelined requests. You moved the HTTP_IN > filter from a connection filter to a request filter. But, the only connection filter >we > have is the CORE_iN filter. All that filter does, is to pass the socket up to the > HTTP_IN filter. The problem is that HTTP_IN will be destroyed in between requests, > so the second request will be lost. I am continuing to read the code, so I may see > what I am missing, but in the mean time, Justin, if you could explain that to me, > it would make my review go a lot faster. There is a create_request hook which inserts the HTTP_IN filter when the request is created. This seems like a good idea: each instantiation of the HTTP_IN filter is for *one* request and one request only. We can be much more sure that state is not spanning requests. Even better, this enables things like the Upgrade: header (RFC 2616, S14.42). One request specifies an upgrade, and the next request should be handled by a different protocol. [ of course, that still won't work since we'll continue inserting the HTTP_IN filter for each request, but making HTTP_IN a request filter takes us a step in the right direction. ] Along the mantra of "an input filter should not return more than is asked for", we can also be sure that the HTTP_IN filter is going to receive exactly the amount of data that it asked for, and no more. Thus, the next request will still be sitting in the next filter down. Okay. Now all that said. Some random notes about the current code: * CORE_IN is returning a new socket bucket each time. That socket bucket goes into a brigade associated with the request, so it gets destroyed with the request. Not really a problem since the actual socket is cleared by a pool, not by the bucket. When the next request asks for more data, it will get a new socket bucket. I don't think this is The Right Thing, but until we pass an "amount requested" parameter through the input filter chain, this will work quite fine. * The uses of ap_getline are destined to eventual failure, which I've noted before. They always read from the top of the filter stack -- even if you have a mid-level filter (HTTP_IN) trying to read a line(!). This is solved again by passing "amount requested" down thru the input chain, where one of the "amounts" is a qualified constant saying "one line". We support that read-request mode through a brigade function to read a line into a pool-allocated buffer, into a provided buffer, or maybe into a new brigade (e.g. split a brigade at the newline point, and return the initial brigade). I need to review the code some more. As Justin pointed out, the massive change to http_protocol.c makes that hard to review. Other notes may be coming soon... Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] Rework httpd buckets/filtering
quot;
> > - ### it presumes that the next filter up will *only* call us
> > - ### with readbytes set to the Content-Length of the request.
> > - ### that may not always be true, and this code is *definitely*
> > - ### too presumptive of the caller's intent. the point is: this
> > - ### filter is not the guy that is parsing the headers or the
> > - ### chunks to determine where the end of the request is, so we
> > - ### shouldn't be monkeying with EOS buckets.
> > -*/
> > -if (*readbytes == 0) {
> > -apr_bucket *eos = apr_bucket_eos_create();
> > -
> > -APR_BRIGADE_INSERT_TAIL(b, eos);
> > -}
Somebody has to return an EOS somewhere.
Consider the highest-level input filter. How does it know when it has read
all of the data for the current request? It needs to see an EOS bucket.
I'll wait for the new patch and review again. All the formatting changes
were getting in the way.
But I really think the proper tack is to fix the amount-returned, and to
combine the HTTP_IN and DECHUNK filters.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
On Thu, Sep 20, 2001 at 07:54:22PM -0700, Ryan Bloom wrote:
> On Thursday 20 September 2001 05:48 pm, Greg Stein wrote:
...
> > Calling pop_cleanup() on every iteration is a bit much. Consider the
> > following patch:
>
> Why is it a bit much? I just took a quick look at it, it is an if, and three
>assignments.
Heh. It is a bit much when you consider you consider a thread in October
1999 discussing using NULL rather than ap_null_cleanup:
http://www.humanfactor.com/cgi-bin/cgi-delegate/apache-ML/nh/1999/Oct/0189.html
In that case, an "if" was considered too much :-)
> I would assume that any compiler worth it's salt would inline this function as well.
Very bad assumption. But this isn't the place to discuss compilers.
> This patch also keeps the LIFO behavior, which is important, because it means
> that it is much less likely that an item allocated out of the pool when the cleanup
> was registered will no longer be there when the cleanup is run.
I am telling you that dependency upon items in the pool is bogus. You cannot
do that. Thus, LIFO means diddly squat.
Consider a case where A inserts its cleanup, expecting to have B present. At
some point between then and the pool cleanup, B goes and removes its
cleanup, then inserts some other cleanup. This new one, B', will now run
*before* A's cleanup.
Gee. Guess what? Your oh-so-needed LIFO behavior has been screwed. B is
*not* present for A to use.
The *only* guarantees you have are between parent and child pools. Within a
pool, you've got nothing.
> > while ((c = p->cleanups) != NULL) {
> > p->cleanups = NULL;
> > run_cleanups(c);
> > }
>
> Which function is this in? I have looked, but the only place that I can find to
> put this code would be in apr_pool_clear, around the run_cleanups code.
Yes, it would go in apr_pool_clear. And a similar one for the child cleanup
case.
> > Basically, the above code processes the cleanups in batches. Everything
> > that was initially registered is processed, then everything registerd
> > during the first cleanup round, etc.
>
> This makes it more more likely that a variable in the same pool that was available
> when the cleanup was registered would not be available when your cleanup
> ran. I would really want to see a performance analysis before we broke that
> behavior.
Broke what behavior? Registering a cleanup in a cleanup doesn't work now.
The new behavior is, "if you register it, I'll run it after I'm done running
these other cleanups."
Stop. Take a breath. Think about it. LIFO doesn't matter.
-g
--
Greg Stein, http://www.lyra.org/
Re: pool cleanup
On Thu, Sep 20, 2001 at 07:02:58AM -0700, Ryan Bloom wrote: > On Wednesday 19 September 2001 02:21 pm, Greg Stein wrote: >... > > They are not strictly LIFO. You can remove a cleanup and insert a new one > > at any time. Let's say that the cleanup list looked like: > > > > cleanups: A > > > > and you add a new one to the "front": > > > > cleanups: B A > > > > and now case 1, where A needs to rejigger its cleanup param a bit: > > > > cleanups: A' B > > > > or case 2, where A simply removes its cleanup: > > > > cleanups: B > > > > > > Case 2 actually happens quite often. > > This is all true, but it is also orthogonal to this conversation. Partly. The conversation moved into "what can you do in a cleanup". If you want to look at the simple issue of registering cleanups... okay. But when people were expecting to be able to do "anything" in a cleanup... that is intrinsically incorrect. > The question we are > trying to answer here, is can you register a cleanup within a cleanup. Aaron posted a patch, but it introduces too many function calls in the processing. I posted one that is much more optimal, processing the cleanups in batches. That would fix your issue. > If we are in > the middle of running the cleanups, and somebody actually calls cleanup_run > or cleanup_kill from within a cleanup, they are broken and it may not work. My above case wasn't talking about doing those from within a cleanup (which is definitely and always wrong). I was showing how the cleanups could be reordered; therefore, how you cannot depend upon particular cross-cleanup ordering dependencies. Thus, you are actually somewhat limited in what kinds of things you can truly do in a cleanup. > It also doesn't make any sense, because the reason to run a cleanup, is to perform > some action sooner than you would have otherwise, but in this case, we are going > to perform that action in a few seconds anyway. I don't get this part. A cleanup is to do just that: clean up after yourself when the pool goes away. It provides a point in time for specific types of actions. I'm not sure how that gives you "sooner"; if anything, a cleanup is for running things later. > Since the two cases above require a programer to either remove or run a cleanup, > they don't really make sense in the context of registering a cleanup within a >cleanup. > This means that is safe to register a cleanup within a cleanup, assuming the code > is patched correctly. Agreed. My point was addressing the "arbitrary work in a cleanup" meme that was brought up. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: [PATCH] fix cleanups in cleanups
On Thu, Sep 20, 2001 at 11:18:55AM -0700, Aaron Bannert wrote:
>...
> Does this fix it for you? All testmem tests passed for me and your code
> above properly flushes "Cleanup" to the file.
>
> (Someone needs to check my work on run_child_cleanups() and make sure
> that the popping is necessary. It looked to be in the same situation.)
Calling pop_cleanup() on every iteration is a bit much. Consider the
following patch:
while ((c = p->cleanups) != NULL) {
p->cleanups = NULL;
run_cleanups(c);
}
You don't even have to change run_cleanups or run_child_cleanups.
Basically, the above code processes the cleanups in batches. Everything that
was initially registered is processed, then everything registerd during the
first cleanup round, etc.
It does not maintain the LIFO behavior where cleanup A registers cleanup B
and expects B to run *just after* cleanup A finishes. If A wanted that, then
it could just calll B. But the most important part: all cleanups *do* get
run.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Thu, Sep 20, 2001 at 12:53:39AM -0700, Alex Stewart wrote: > On a largely unrelated note, but something I found a little ironic given > the nature of this list: > > Aaron Bannert wrote: > > > >http://www.apachelabs.org/apache-mbox/199902.mbox/<[EMAIL PROTECTED]> > > Please note that the above is not a valid URL. Specifically, the "<" Agreed. >... > Whoever does the software behind apache-mbox (I take it this is > mod_mbox?) might want to take note that it's spitting out invalid URLs.. The URLs produced by mod_mbox are fine. Aaron must have posted an unescaped version of the URL. (go to a mod_mbox page and view the source...) Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: Q1: Rollup Release Format - Score So Far...
On Wed, Sep 19, 2001 at 04:32:12PM -0400, Joshua Slive wrote: > > -Original Message- > > From: Graham Leggett [mailto:[EMAIL PROTECTED]] > > > The winner seems to be that the Apache group releases a rollup release > > with all (at least apr, apr-util, httpd-2.0, httpd-proxy, httpd-ldap) > > the projects included, and that release is called "apache-2.x.x.tar.gz". > > Am I right on this one? > > I have no problem with that (although it seems that the only difference > between this and what the group has always done is that everything lives in > different cvs repositories). Seems that way. We can have multiple CVS repositories and then make decisions on which ones go into the rollup. For example, we could include mod_proxy and mod_ldap repositories, but exclude the mod_pop3 repository. And on into the future... > But I think we also have concensus that the name shouldn't be "apache". > "apache-httpd-2.x.x.tar.gz" seems better. Agreed. I am +0 on httpd-2... and +1 on apache-httpd-2... btw, no dates in those either (a suggestion from otherbill). The version number tells us what we need to know. Cheers, -g -- Greg Stein, http://www.lyra.org/
pool cleanup (was: Re: New post-log-transaction hook?)
On Wed, Sep 19, 2001 at 12:16:24PM -0700, Ryan Bloom wrote: > On Wednesday 19 September 2001 11:37 am, William A. Rowe, Jr. wrote: > > From: "Greg Stein" <[EMAIL PROTECTED]> > > Sent: Wednesday, September 19, 2001 1:26 PM >... > > > The problem is cross-dependency between the cleanup actions. One can > > > destroy something another cleanup needs. If you restrict their actions to > > > "simple" things, then the cross-dependencies are (hopefully!) removed. > > > > Really? No. Cleanups are run as a LIFO stack. Anything that existed when > > something was added to the pool must exist when that something is removed > > from the pool. They are not strictly LIFO. You can remove a cleanup and insert a new one at any time. Let's say that the cleanup list looked like: cleanups: A and you add a new one to the "front": cleanups: B A and now case 1, where A needs to rejigger its cleanup param a bit: cleanups: A' B or case 2, where A simply removes its cleanup: cleanups: B Case 2 actually happens quite often. > > IMHO, we need to make subpool scrubbing an actual LIFO cleanup as well, so > > that will also be true of subpools. Subpools are entire entities. There shouldn't be any cross-dependencies between those. Care in ordering isn't necessary. > > Considering how we use pools for dynamic libraries and the rest, it's > > absolutely vital that they are unspun from the pool in LIFO order of their > > creation. Ah. That is a good example... had forgotten about that one (I even ran into this once). Using the symbology from above, what happens if B is using code from a dso that installed A? What happens if A is unloaded and reloaded, thus rearrange the queue as in case 1. The point is: I've observed problems with cleanup ordering. And the DSO thing is particular nasty. At one point, I was considering adjusting Apache's pool structure to better order when the DSOs were unloaded. For example, if we loaded DSOs into Pool X and then used subpool Y for all module-related allocations (e.g. pconf == Y and X is internal). > I agree with Bill. Having reviewed the code quite deeply yesterday, pool > cleanups follow a very clean rule, and registering a cleanup from within > a cleanup will always work if done correctly. Huh? If you register a cleanup within a cleanup, it *won't* be run. We grab the list, then run it. Most types of changes to that list won't be seen; particularly insertions at the front of it. > If you have data in a pool > when the cleanup is registered, it is gauranteed to be there when the cleanup > is run. The pool data might, but associated resources may not be. For example, the apr_file_t structure might still be in the pool (simply cuz it can't be freed), but the underlying file descriptor could be closed. > Anything else is completely broken. Depends on your interpretation :-) I think the cleanup behavior is very well defined, and has certain restrictions on the kinds of things that you can do in there. And one of those restrictions is watching out for dependending on something else in your pool. As a result, arbitrary work can definitely be affected. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Wed, Sep 19, 2001 at 01:52:12PM -0400, Rodent of Unusual Size wrote: > Greg Stein wrote: > > It isn't a bug. Cleanups are for just wrapping things up, > > not doing work. > > If that's the authoritative answer, then we need to provide > a supported way for 'doing work' at cleanup time. > > > You might not even be able to open and use that file if > > your pool is n the process of being destroyed. > > That sounds like utter tripe. If you can't depend on the > pool lasting at least until your cleanup routine ends, > then the whole cleanup mechanism is seriously borked. AFAIK > it isn't, so I think the above assertion *is*. The problem is cross-dependency between the cleanup actions. One can destroy something another cleanup needs. If you restrict their actions to "simple" things, then the cross-dependencies are (hopefully!) removed. Consider a platform that creates a subpool to manage a set of file descriptors for the apr_file_t objects. That subpool is linked from the userdata of specified pool. At cleanup time, that subpool has been tossed, so you cannot create files on the given pool. Or maybe it is as simple as APR registers a cleanup on the pool to do some cleanup, which clears out some basic structure needed for creating FOO objects. And that APR's cleanup runs before yours. The basic issue is that cleanups are generally non-deterministic. A cleanup could run before another and blast some basic service, preventing the second cleanup from doing "work". Think about database connection pools, shared memory stuff, temporary files, etc. The list goes on and on. That said: if you want to create a second list of "do some work when the pool is cleared" type thing, then fine. As long as its understood that *that* list is for non-destructive work, and the cleanups are for destructive work. The work-list can run before subpools are tossed, which means the pools are "unchanged". Cheers, -g p.s. "utter tripe" indeed... that was rather inflammatory... -- Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Wed, Sep 19, 2001 at 11:17:36AM -0500, William A. Rowe, Jr. wrote: > From: "Ryan Bloom" <[EMAIL PROTECTED]> > Sent: Wednesday, September 19, 2001 9:52 AM > > > > On Tuesday 18 September 2001 06:09 pm, Greg Stein wrote: > > > I agree with OtherBill. > > > > > > Cleanups are not always the answer. When they are run, many things > > > associated with that pool could be torn down already because their cleanups > > > have already run. > > > > Jon is already using a pool cleanup to solve this problem. In fact, his > > initial thought was to use a pool cleanup, but he couldn't register cleanups > > from within a cleanup. Adding a new hook was a second solution from Jon, > > because he just needed to solve his problem. > > Note (for those not paying that close attention) just trying to open a file within > a cleanup, do something, and close it will attempt to create a cleanup :( > > This bug still must be fixed. It isn't a bug. Cleanups are for just wrapping things up, not doing work. You might not even be able to open and use that file if your pool is in the process of being destroyed. Doing significant work at cleanup time, *especially* things which involve that same pool, is flat out prone to bugs. The cleanups are not ordered (they shouldn't be), and the fact they are running means that the pool's usability is marginable. Note that the pool's subpools have already been destroyed. It isn't a bug. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker fdqueue.c fdqueue.h
On Tue, Sep 18, 2001 at 06:33:14PM -0700, Aaron Bannert wrote:
> On Tue, Sep 18, 2001 at 06:01:35PM -0700, Greg Stein wrote:
>...
> > >...
> > > --- fdqueue.c 2001/09/18 21:14:18 1.6
> > > +++ fdqueue.c 2001/09/18 23:09:12 1.7
> > > @@ -73,7 +73,6 @@
> > > */
> > >static int ap_queue_empty(fd_queue_t *queue)
> > >{
> > > -/*return (queue->head == queue->tail);*/
> > >return (queue->blanks >= queue->bounds - 1);
> >
> > What are ->blanks and ->bounds for? The test is easier done as:
>
> remnants of when fdqueue was more complicated and offered more functions.
Understood. I meant "there is more cleanup that can be done."
> > return queue->tail == 0;
> >
> > Toss ->blanks and rename ->bounds as size or capacity or something. It is
> > only an upper-bound, so "bounds" isn't quite right semantically.
>
> I've been had for name changes in the past, so I opted for the least changes
> to reach correctness. As soon as these other major worker changes start
> settling down, I'll definately go through and do names changes and minor
> format fixes where appropriate for posterity.
Name changes are no big deal if you're improving clarity. Gratuitous changes
are usually bad :-)
In this case, name changes are going to be very localized, so it's easy. And
tossing ->blanks also simplifies things. ->tail and ->blanks are redundant.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
Re: cvs commit: httpd-2.0/server/mpm/worker worker.c
On Wed, Sep 19, 2001 at 06:34:11AM -, [EMAIL PROTECTED] wrote: > jwoolley01/09/18 23:34:11 > > Modified:server/mpm/worker worker.c > Log: > I was kinda hoping those (void)some_function() and (request_rec *)NULL > casts would go away before this committed, but alas I didn't say anything. > :-) This gets rid of them and a few others just like them that I also > found in worker.c. Um. I'm leaning towards a -1 on removing those (void) casts. The functions return a value, and it is not being used. The (void) makes it clear that the values are being discarded. And generally, a discarded value isn't a Good Thing, so those (void) casts leave markers for somebody to fix the code at some future time. Cheers, -g -- Greg Stein, http://www.lyra.org/
Re: New post-log-transaction hook?
On Tue, Sep 18, 2001 at 06:52:35AM -0400, Jeff Trawick wrote: > Greg Stein <[EMAIL PROTECTED]> writes: > > > 2) move the ap_lingering_close inside ap_process_connection, then call it > >from with ap_process_connection. This *almost* works. All MPMs have a > >call to ap_process_connection followed by a call to ap_lingering_close. > >The only MPM that does other funny stuff in there is the winnt MPM. > >Conceivably, you could pass a flag that says "I'll manage the shutdown, > >thankyouvermuch", and winnt would do its close followed by the > >post-connection hook. > > It is nice for ap_lingering_close() to be handled in the MPM. It > shouldn't be too hard for the MPM to implement its own lingering close > in a manner that doesn't consume one thread per closing connection > (e.g., dedicate one thread per MPM to do all the lingering close > logic). Of course there could always be another hook :) Great idea, and yes: switching to a hook would still provide for a complex lingering close capability. Cheers, -g -- Greg Stein, http://www.lyra.org/
