Re: SHA1 and Base64
--- Greg Stein [EMAIL PROTECTED] wrote: Well, it has been suggested that we create a new APR utility library. The discussion on that *just* opened today. It looks like we have several +1 on the concept, and I posted an outline of my thoughts on it. It's cool with me. That thread started while I was writing my email. =-) In any case... what I'm saying is that your idea may have merit, but it is probably easier to start with just one more library and go from there. As long as somebody can figure out what to put in it, then that's cool. --Cliff __ Do You Yahoo!? Yahoo! Shopping - Thousands of Stores. Millions of Products. http://shopping.yahoo.com/
apr-util comments
Okay, I realize that apr-util was just born :), and what's in there right now is basically just a first pass to facilitate the move to the httpd-2.0 repository for Apache. So thiss is as much to make sure that I understand the intentions as anything else. 1) I assume that the ap_* prefix on files/etc hasn't been changed to apr_* yet simply to ease the transition to httpd-2.0, and that the namespace change will come in a later pass. Right? 2) Shouldn't src/encoding/ap_sha1.c really go in src/crypto/? SHA1 is a hashing algorithm, not an encoding algorithm. Right? 3) Might the src/buckets directory want to be called src/data (hmm... maybe confusing) or src/datastructures (hmm... kind of long) or something like that? (Somebody please think of a more concise name than I have.) I'm trying to anticipate where it is that tables/hashes/whatever would go if/when they get moved out of APR. This sort of follows as a simplification of my suggestion last week that the data structures could stand alone as a separate library beneath APR. Overall, it looks really good so far. =-) --Cliff __ Do You Yahoo!? Yahoo! Shopping - Thousands of Stores. Millions of Products. http://shopping.yahoo.com/
Re: [PATCH] Buckets: add copy function, ap_bucket_split_any(), etc
--- Greg Stein [EMAIL PROTECTED] wrote: I've applied the patch because it is a good start. However, ap_bucket_split_any() is not yet right. If a caller says split at 1, then it will probably fail because the read() won't read in that much. You need to read enough buckets, until you read the bucket that contains the split point. *then* do the split. I completely agree. It was done this way because it was all OtherBill and I could get Ryan to agree to after a lengthy debate on new-httpd. What *I* wanted to do was to make split a native function of sockets and pipes that just happened to be a little overloaded. (See my original posting and the ensuing debate in the thread Implementing split() on pipe buckets?, beginning 11/12.) The short-short version is that I wanted to change pipe_read() to pipe_readn(), a function that obeys the len parameter. pipe_split() would pass in point as the length to read, pipe_read() would pass in IOBUFSIZE. It was fairly straightforward. Ryan disliked it because it cluttered the buckets code with non-atomic operations. Personally, I couldn't see why the read() function was allowed to be non-atomic but the split() function was not. But whatever. I'll let you do the arguing this time. =-) --Cliff __ Do You Yahoo!? Yahoo! Shopping - Thousands of Stores. Millions of Products. http://shopping.yahoo.com/
Re: cvs commit: apr-util/src/buckets ap_buckets_refcount.c
--- [EMAIL PROTECTED] wrote: wrowe 00/12/08 08:10:33 Modified:.aprutil.dsp include ap_buckets.h src/buckets ap_buckets_refcount.c Log: Buckets build again on Win32 Thanks. I'm still not 100% clear on when to use *_DECLARE_NONSTD instead of *_DECLARE. I knew you'd fix it if I got it wrong. =-) --Cliff __ Do You Yahoo!? Yahoo! Shopping - Thousands of Stores. Millions of Products. http://shopping.yahoo.com/
[PATCH] fix APR_HAVE_CRYPT_H build breakage on Unix
The recent change to use APR_HAVE_*_H macros in the Apache support files forgot one little bit, which broke the build. --Cliff Index: configure.in === RCS file: /home/cvspublic/apr/configure.in,v retrieving revision 1.193 diff -u -r1.193 configure.in --- configure.in2000/12/21 20:59:24 1.193 +++ configure.in2000/12/21 21:38:39 @@ -255,7 +255,7 @@ AC_CHECK_HEADERS(ByteOrder.h) AC_CHECK_HEADERS(conio.h) -AC_CHECK_HEADERS(crypt.h) +AC_CHECK_HEADERS(crypt.h, crypth=1, crypth=0) AC_CHECK_HEADERS(ctype.h, ctypeh=1, ctypeh=0) AC_CHECK_HEADERS(dir.h) AC_CHECK_HEADERS(dirent.h, direnth=1, dirent=0) __ Do You Yahoo!? Yahoo! Shopping - Thousands of Stores. Millions of Products. http://shopping.yahoo.com/
Re: [PATCH] ap_brigade_partition()
--- Greg Stein [EMAIL PROTECTED] wrote: Now that we're actually using this function, I figured it was time to dig in and spend the time with this patch. :-) Cool. Just so you know, I've been kind of out-of-touch with the list for a while, since the 1000+ messages that accumulated while I was on vacation have been a rather daunting pile to tackle. If I've missed anything else related to this subject, please let me know. *) what happens when you find a bucket with e-length == -1 ? AFAICT, the function doesn't handle it. I'm thinking a check at the top of the loop for a length==-1 and a read(), then do the point/length compare stuff. Oops. I think you're right... that was a hidden semantic that seems to have been lost in the optimization process. *) after_point looks wrong for the manual-split case. It appears that you'd end up with (B1a, B1b, B2) and return B2 rather than B1b. Oops... you're right on that one, too. Again, it seems that I've optimized away the correct behavior. ;-] I'll fix these two things and resubmit the patch (I guess the new patch should also include an update to the byterange filter). I'll try to do this today, but it might be tomorrow... my Linux filesystem went kaflooey on me and I'm still trying to sort it all out. --Cliff __ Do You Yahoo!? Yahoo! Auctions - Buy the things you want at great prices. http://auctions.yahoo.com/
[PATCHv2] apr_brigade_partition()
--- Greg Stein [EMAIL PROTECTED] wrote:
Now that we're actually using this function, I figured it was time to dig in
and spend the time with this patch. :-)
*) what happens when you find a bucket with e-length == -1 ? AFAICT, the
function doesn't handle it. I'm thinking a check at the top of the loop
for a length==-1 and a read(), then do the point/length compare stuff.
*) after_point looks wrong for the manual-split case. It appears that you'd
end up with (B1a, B1b, B2) and return B2 rather than B1b.
Both of these problems should now be taken care of. My bad. =-)
One note: I changed a conditional from (point len) where le is returned from
apr_bucket_read() to (point e-length) to match a later test for (point ==
e-length).
I added a comment explaining why this is okay (namely that len == e-length).
In every
currently existing bucket type, if read wants to return some data that is less
than what
the bucket contains, it always morphs the bucket into two buckets and adjusts
the
e-length values. I also am 99% sure that Ryan told me at one point that the
current
design doesn't allow for partial bucket reads anyway, which makes sense given
what I've
seen. I guess I really don't understand the point of the len parameter to
apr_bucket_read(), since the value it returns is easily fetched from e-length
at any
time. Most bucket types compute it from end - start as opposed to just using
e-length,
but the two should be equal (if they're not, it's a bug AFAICT).
I also added an outline for the changes necessary to the byterange filter in
Apache
(which is why I copied this message to new-httpd)... but all it does is make
the filter
compile with the new API, not do anything useful with the new information. The
part I
left out is that I don't know what the correct way to report an error from
apr_brigade_partition is in terms for handling byteranges.
Didn't we have another function? A copy() function or something? What
happened to that one? (or should I troll my mail archive)
Uhm... apr_brigade_partition has gone through many phases, of course...
split_any, then
brigade_split, and now brigade_partition. But AFAIK, it's the only remaining
function in
question. I mean, we added the bucket copy functions a while back, if that's
what you're
thinking of... other than that, I have no idea what you're talking about. ;-)
I'm attaching the patch rather than inlining it because it contains some long
lines that
would wrap with my mail client. Hope that doesn't cause anybody too much grief.
--Cliff
__
Do You Yahoo!?
Yahoo! Auctions - Buy the things you want at great prices.
http://auctions.yahoo.com/Index: modules/http/http_protocol.c
===
RCS file: /home/cvspublic/httpd-2.0/modules/http/http_protocol.c,v
retrieving revision 1.287
diff -u -r1.287 http_protocol.c
--- modules/http/http_protocol.c2001/01/28 04:07:03 1.287
+++ modules/http/http_protocol.c2001/01/29 04:25:53
@@ -227,7 +227,7 @@
char *current;
char *bound_head;
int clength = 0;
-apr_status_t rv;
+apr_status_t rv, rv2;
int found = 0;
if (!ctx) {
@@ -328,8 +328,11 @@
APR_BRIGADE_INSERT_TAIL(bsend, e);
}
-e = apr_brigade_partition(bb, range_start);
-e2 = apr_brigade_partition(bb, range_end + 1);
+rv = apr_brigade_partition(bb, range_start, e);
+rv2 = apr_brigade_partition(bb, range_end + 1, e2);
+if ((rv != APR_SUCCESS) || (rv2 != APR_SUCCESS)) {
+/* XXX: what is the correct action here??? */
+}
ec = e;
do {
Index: srclib/apr-util/buckets/apr_brigade.c
===
RCS file: /home/cvspublic/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.3
diff -u -r1.3 apr_brigade.c
--- srclib/apr-util/buckets/apr_brigade.c 2001/01/19 07:01:59 1.3
+++ srclib/apr-util/buckets/apr_brigade.c 2001/01/29 04:26:13
@@ -123,48 +123,47 @@
return a;
}
-APU_DECLARE(apr_bucket *) apr_brigade_partition(apr_bucket_brigade *b,
apr_off_t point)
+APU_DECLARE(apr_status_t) apr_brigade_partition(apr_bucket_brigade *b,
+apr_off_t point,
+apr_bucket **after_point)
{
apr_bucket *e;
const char *s;
apr_size_t len;
+apr_status_t rv;
if (point 0)
-return NULL;
+return APR_EINVAL;
APR_BRIGADE_FOREACH(e, b) {
-/* bucket is of a known length */
-if ((point e-length) (e-length != -1)) {
-if (APR_BUCKET_IS_EOS(e))
-return NULL;
-point -= e-length;
-}
-else if (point == e-length) {
-return APR_BUCKET_NEXT(e);
-}
-else {
+if ((point e-length) ||
Re: [patch] Time fixes for Win32
--- [EMAIL PROTECTED] wrote: +/* Leap year is any year divisible by four, but not by 100 unless also + * divisible by 400 + */ +#define IsLeapYear(y) ((!(y % 4)) ? (((!(y % 400)) (y % 100)) ? 1 : 0) : 0) + If y is divisible evenly by 400, it's automatically divisible by 100... you don't need to check both. =-) --Cliff __ Get personalized email addresses from Yahoo! Mail - only $35 a year! http://personal.mail.yahoo.com/
Re: [patch] Time fixes for Win32
--- Cliff Woolley [EMAIL PROTECTED] wrote: If y is divisible evenly by 400, it's automatically divisible by 100... you don't need to check both. =-) That's what I get for talking before I think... I guess you actually DO have to check, since you have to be sure that if it's not divisible by 400 that it's also not divisible by 100. Nevermind... my bad. :) --Cliff __ Get personalized email addresses from Yahoo! Mail - only $35 a year! http://personal.mail.yahoo.com/
Re: cvs commit: apr/strings apr_strings.c
--- [EMAIL PROTECTED] wrote:
+APR_DECLARE(void *) apr_memdup(apr_pool_t *a, const void *m, apr_size_t n)
+{
+void *res;
+
+if(m == NULL)
+ return NULL;
+res = apr_palloc(a, n);
+memcpy(res, m, n);
+return res;
+}
+
Nice. Minor question, though: why is this called apr_memdup instead of
apr_pmemdup?
I thought all of the like the standard function, but into a pool instead of
the heap
functions were prefixed with a 'p'.
Also, should it check for failure of apr_palloc, or is that being too anal?
--Cliff
__
Do You Yahoo!?
Get personalized email addresses from Yahoo! Mail - only $35
a year! http://personal.mail.yahoo.com/
RE: brigade/bucket insertion macros
-Original Message- Or do you mean a full patch to mass-cleanup all callers of the macros throughout Apache? I meant the latter. I can apply what you already sent in about an hour, I need to get to the office soon. All that's left is the rest of the cleanup. :-) Ahh. =-) Well in that case, sure, I can do that. It'll take me a little time though... I'm tied up until about lunchtime EST. But I'll definitely get this submitted this afternoon. --Cliff
RE: apr_bucket_pipe_creat and friends
Doug MacEachern [mailto:[EMAIL PROTECTED] wrote: i can take care of these no problemo. Cool, thanks. --Cliff
RE: docco problem (was RE: Chunk filter problem?)
Didn't you say that mod_include used them the other direction? Does this mean that mod_include is now wrong? I said that I thought it was, yeah, but that was after only a very brief inspection. It turns out that it's just playing tricky games, I think. I'm still trying to digest mod_include. =-) But anyway, if they were backwards, we'd be seeing scrambled web pages and other very wacky behavior. Somebody who knows mod_include better than I do could verify this if they had a second... I don't suppose anyone would object if I documented [the ring macros]? =-) Heh... no :-) I didn't think so. ;-] It's on my short list. --Cliff --- Cliff Woolley [EMAIL PROTECTED] 804-244-8615 Charlottesville, VA
RE: bucket deletion, macro caps
-Original Message- Cliff, you just hit on two things that I have spent a lot of time thinking about, so I'll give my opinions to add to your own. Glad it's not just me. =-) I realized that combining them wasn't as easy as you would think. The first option is to always destroy whenever you call APR_BUCKET_REMOVE. This doesn't work though, because it means that we can't move a bucket from one brigade to another [APR_BUCKET_REMOVE(e);APR_BRIGADE_ADD(...)]. Clearly. And there are probably some other valid reasons to want to do this... Anyway, you're right, this wouldn't work. The second option is much more appealing. I would just put a call to APR_BUCKET_REMOVE(e) inside of apr_bucket_destroy(). This is safe to do, because we can't destroy a bucket without removing it from the brigade, and if the bucket isn't in the brigade, the macro is basically a no-op. I thought about that, too. But I was worried that it might be possible to create a bucket and then immediately destroy it, and the -prev and -next pointers might be garbage. Upon further inspection, I now see that apr_bucket_do_create uses APR_RING_ELEM_INIT, so this would work. So it's either overloading apr_bucket_destroy() or adding a third macro. I'm game either way. (2) I'm ambivalent about the lingering non-capitalization of apr_bucket_destroy|read|split|setaside|copy. On the one hand, I am against renaming personally, but if you are going to rename, do it now. :-) Somehow I knew you'd say that. =-) I'm starting to think that if we take care of (1), then (2) won't matter all that much, and we can get by with ignoring it. (Besides, after more thought, I'm swinging back toward thinking that it's more important for apr_bucket_destroy() to match apr_bucket_foo_create() than for it to be capitalized just because it's a macro.) --Cliff
RE: Working platforms - HP-UX 11.00
--- Jeff Trawick said: (by the way, we don't build with the HP compiler on HP 10.20; need to look lots further into which compiler I have access to and why it doesn't like some seemingly benign declarations in APR) That doesn't surprise me... HP-UX's native compiler has been known to do some massively stupid things. I don't know about sendfile being braindead, but the compiler certainly is IMO. Anyway, if you need me to test with either the native compiler or gcc on a 10.20 box, I've still got accounts on a couple... --Cliff
RE: cvs commit: apr/build config.guess config.sub
-Original Message- Um... how does this not infect the APR tree with the GPL? My understanding is that we can redistribute the *output* of GNU development tools like this, but not the actual tools themselves. If we can't distribute this file, then we can't use autoconf/libtool. Like the file says, we need these files, because we are more portable than the GNU tools we rely on. I quote from config.guess (config.sub has the same block): As a special exception to the GNU General Public License, if you distribute this file as part of a program that contains a configuration script generated by Autoconf, you may include it under the same distribution terms that you use for the rest of that program. Looks okay to me... --Cliff --- Cliff Woolley [EMAIL PROTECTED] 804-244-8615 Charlottesville, VA
APR + netware?
Just out of curiosity, is anyone (presumably someone at Novell) working on a port of APR to Netware? I'm not volunteering or anything, because I know nothing about Netware programming and don't have a box to do it on anyway... I'm just curious. It hadn't dawned on me until today for some reason that there are no Netware files at all in APR. Hmph. --Cliff --- Cliff Woolley [EMAIL PROTECTED] 804-244-8615 Charlottesville, VA
Re: add rename symbols to ap*_compat.h
On Tue, 20 Feb 2001, Greg Stein wrote: Okay, people. Here is your chance to vote. add 2.0 symbol renames to ap*_compat.h: -0: Greg, Doug I don't know if I get to vote or not, but I'm -0 anyway. :-] --Cliff
RE: mod_userdir segfault (segfault type #3)
-Original Message- There is a bug report about this too. Please fix it. :-) There's a patch in the PR (actually very similar to what Manoj originally committed with the function and later removed). But it's not threadsafe (which seems to be why Manoj removed it in the first place). My first thought would be to take the code in APR's apr_get_user_directory() (in userinfo.c) that figures out which getpwnam to use and split out into its own function, apr_getpwnam(). But getpwnam seems to be an inherently Unix thing... so what does that mean for Win32? Just return APR_ENOTIMPL? What about the parameter list, which would include a 'passwd **'? Should it be a 'void **' on Win32? Or should I leave the function completely undefined on Win32? Alternatively, we could just duplicate the little bit of #ifdef magic from apr_get_user_directory() that figures out which version of getpwnam() is threadsafe and pull it into mod_userdir... but that doesn't seem like a very APR-ish way to do things. Thoughts? --Cliff
apr_sigwait/SunOS compile break
Just a heads up on something I ran across a day or so ago in case you all get a chance to look at it before you roll again (I probably won't have a chance to look at it before then). I'm still having problems with an incorrectly detected number of arguments to sigwait on SunOS as of Friday, I think, and AFAIK no patches have gone in to fix it. I know this was mentioned on the list earlier this week... were any conclusions reached? Basically, it's a problem of SunOS having different numbers of arguments for sigwait depending upon what is #defined... I seem to recall it was a problem with the configure script not including all the right CFLAGS. Just wanted to bring it back up in case a new tarball is being rolled again soon, in case there's a chance it can get fixed before then. If the next tarball is going to be a few days off, I'll look into it myself. --Cliff
Re: monday morning breakage
On 26 Feb 2001, Ben Collins-Sussman wrote: #if APR_HAVE_SYS_SENDFILE_H #include sys/sendfile.h #endif And, of course, my system has no sys/sendfile.h. :) I know that there's been a recent thread on this list about sendfile.h, but I have to admit that I didn't read it. (*blush*) Can anyone tell me what the problem is? I assume that that APR_HAVE_SYS_SENDFILE_H is being defined when it shouldn't be. More likely, APR_HAVE_SYS_SENDFILE_H is *not* defined when it *should* be. The APR_HAVE_* macros are always defined (hence the #if rather than #ifdef), with a value of 0 or 1. If the macro is not defined at all, then you get a syntax error in the preprocessor (#if SOME_UNDEFINED_MACRO == syntax error). --Cliff
Re: cvs commit: apr configure.in
On Mon, 26 Feb 2001, Jim Jagielski wrote: Use APR_CHECK_HEADERS instead Before I go any further, I want to rethink the name. Maybe APR_FLAG_HEADERS (want to avoid confusion here) or APR_HEADERS_SET. Suggestions welcome. APR_CHECK_HEADERS doesn't bother me... it makes it clear that it serves basically the same purpose as AC_CHECK_HEADERS, but has added functionality for APR. It also wouldn't hurt my feelings any if APR_CHECK_HEADERS automagically determined the variable name to flag with a 1 or a 0 by doing basically this s/\//_/g; s/\.//g; on the header name. Hence sys/sendfile.h becomes the flag variable sys_sendfileh automatically. That would avoid typo problems like the one this morning, and it would be just one less step that's necessary. The downside is that it implies a little bit of extra work in configure... shrug --Cliff
Re: Bucket API cleanup issues
On Mon, 26 Feb 2001 [EMAIL PROTECTED] wrote: 3) pool_bucket_cleanup() is completely bogus AFAICT. I've added this comment to the code, which describes the problems pretty well: 4) The same problem applies to file buckets that have been split/copied when APR_HAS_MMAP: when one of them gets read it gets made into an MMAP. ALL of the file buckets should be converted at the same time to reference the same MMAP. I disagree about how to fix this. The simple solution is to just abstract on level, such as: bucket - bucket - bucket | | | shared shared shared | | | - | pool_bucket So is what you're saying that struct apr_bucket should be a member of TWO rings... one brigade as usual plus one ring (brigade) of siblings buckets that point to the same resource? I'd thought of that, but didn't think anyone would buy it. I'm fine with the idea. But I *must* be missing something... how does that keep us from traversing a list of buckets when we convert types? (PS: Remember that the shared level is going away.) If I've missed your point, please elaborate. 5) apr_bucket_destroy() should allow the type-specific destructor to free the bucket itself, too (needed to solve problem #3). No. This was done on purpose so that we can cache the buckets themselves. If the type-specific destructor frees the bucket itself, then we can't cache the freed buckets. It also makes it very difficult to convert buckets from one type to the next. DOH!! Yep, you're exactly right... that won't work. Scratch #5. 6) Should mmap_destroy() call apr_mmap_delete(m-mmap) after when the last reference to the mmap bucket is destroyed? No. If you do that, then caching MMAP's becomes impossible. The apr_mmap_delete should be left up to the pool cleanup. Okay, no problem. I'd just noticed it when I was going through the code and thought I'd bring it up, but I wasn't married to the idea. I figured it was left out for a reason but didn't know what it was. Scratch #6. 7) socket_read() and pipe_read() should return APR_SUCCESS even in an EOF condition, as discussed on new-httpd a week or so ago. I haven't made this change yet, though I have put an XXX comment in the code about it, because it requres fixing the input filters in Apache to figure out EOS on their own. Looking for suggestions on how to do that. Didn't we decide that buckets should return EOF? I could be wrong, but I was under the impression that buckets needed to return EOF in order for things to work. No, that's not what we decided. See message [EMAIL PROTECTED] on new-httpd from Ben, which says: -- bl And I'm, once more, completely confused about what should be bl happening when the socket hits EOF in blocking and non-blocking bl cases! rbb IMHO it is really simple. When a socket hits EOF, it should rbb simply remove the socket from the brigade. Period. That's it. rbb The socket_read function should never return EOF. There is no rbb such thing as EOF to a filter. Filters only know about EOS. bl Cool. Again, this makes perfect sense to me. It does, bl incidentally, have to transmute to an empty bucket, so as not to bl invalidate the pointer held at a higher level... -- Other messages in the thread from both Greg and myself support this conclusion, as with one from Greg, [EMAIL PROTECTED]: -- bl ... So, the fault is that APR_EOF should _never_ be returned. ... gs Exactly what I said. Never return EOF. Return zero-length buckets. gs Don't requeue if (internally) you hit an EOF. -- Those were the last words on the matter before the subject of the thread diverged. Each of these should be a separate patch, and preferably with a few hours in between each patch. They are. =-) --Cliff
Re: Bucket API cleanup issues
On Tue, 27 Feb 2001, Greg Stein wrote: Euh... I don't think we want another ring. A simpler idea is to have the apr_bucket_pool structure contain a pointer to an apr_bucket_heap structure. At cleanup time, you do the following: Ahh, (he says as the little light over his head comes on). This makes sense. I like. 6) Should mmap_destroy() call apr_mmap_delete(m-mmap) after when the last reference to the mmap bucket is destroyed? A better approach is to have the cache insert the MMAP bucket with a refcount starting at 2 (one for the cache, one for the bucket). When the last bucket goes away, you'll still have a refcount of 1 and the MMAP will remain alive. Good idea. Reading from the input filter **stack** is different. Let's say that case (1) occurs and you've read the zero bytes out of the brigade. The brigade is now empty, so you do an ap_get_brigade() for more. This drops all the way down to the core_input_filter. It says, sorry, I gave you everything I had and returns APR_EOF. *NOW* is where the EOS is detected. I confused the issue by even mentioning the filters. All I care about right now is what should be returned by the bucket read functions, which we've determined is *never* APR_EOF. If the filters want to return APR_EOF, then they're welcome to, but the buckets themselves don't. That should do the trick. Save this MsgID somewhere :-) ;-] I figured it was more expedient to do a little research and quote what was said than to do the well, I had this impression, no, I had another impression back-and-forth routine. hehe Thanks, Cliff
Re: cvs commit: apr-util/buckets apr_buckets_simple.c
On 28 Feb 2001 [EMAIL PROTECTED] wrote: Fix some warnings related to the fact that you can't do arithmetic with a void *. Oops, sorry about that. Didn't get the warning on my Linux box. Thanks for the fix. --Cliff
apr_bucket_init_types?
Why do we need an array of all the bucket types, and therefore apr_bucket_init_types() and apr_bucket_insert_type()? I seem to recall that this was necessary in the very early days of the bucket API. But I can't see any reason to keep them anymore. (The error bucket gets by without registering its type, for example.) Nothing uses the array that gets built. Can anyone think of a reason I shouldn't delete the array and these two functions? --Cliff
Re: apr_bucket_init_types?
On Tue, 27 Feb 2001, Cliff Woolley wrote: Nothing uses the array that gets built. Can anyone think of a reason I shouldn't delete the array and these two functions? Alright, they're going away. apr_bucket_insert_type() doesn't even work right. (The second assignment should be to *newone, not newone.) =-) const apr_bucket_type_t **newone; newone = (const apr_bucket_type_t **)apr_array_push(bucket_types); newone = type; --Cliff
Re: cvs commit: apr-util/buckets apr_brigade.c
On 28 Feb 2001 [EMAIL PROTECTED] wrote: buf = malloc(APR_BUCKET_BUFF_SIZE); b = apr_bucket_heap_create(buf, APR_BUCKET_BUFF_SIZE, 0, NULL); APR_BRIGADE_INSERT_TAIL(bb, b); +b-length = 0; /* We are writing into the brigade, and + * allocating more memory than we need. This + * ensures that the bucket thinks it is empty just + * after we create it. We'll fix the length + * once we put data in it below. + */ } Oooohh... good catch. Thanks, Ryan. This was one of the cases where the original code was updating start and end but not length, but got away with it because heap_read calculated length every time from end-start. In the process of my translation to a no-end world :), I lost that line. Martin and Jean-frederic, does this fix the corrupt data you were seeing? --Cliff
Re: cvs commit: apr-util/buckets apr_brigade.c
On 28 Feb 2001 [EMAIL PROTECTED] wrote: buf = malloc(APR_BUCKET_BUFF_SIZE); b = apr_bucket_heap_create(buf, APR_BUCKET_BUFF_SIZE, 0, NULL); On a side note, we currently have two macros that do almost the same thing: APR_BUCKET_BUFF_SIZE (=9000) which determines the size of the buffer for brigade buffering and HUGE_STRING_LEN (=8192) which determines the size of the buffer for reading from files, pipes, and sockets. Can we standardize on one of the two? Personally, I prefer the name of APR_BUCKET_BUFF_SIZE, but kind of like the 8KB size of the other. --Cliff
Re: cvs commit: apr-util/buckets apr_brigade.c
On Wed, 28 Feb 2001 [EMAIL PROTECTED] wrote: I like making APR_BUCKET_BUFF_SIZE == 8192 [and using it] Okay, I'll do that. and removing the other. HUGE_STRING_LEN comes from apr_lib.h, not from within aprutil, and other parts of APR (and maybe Apache, too) are using it. It ought to be namespace protected (APR_HUGE_STRING_LEN) (don't I remember past discussion about this?). At that point, we could just #define APR_BUCKET_BUFF_SIZE APR_HUGE_STRING_LEN But then, is APR_BUCKET_BUFF_SIZE still useful? Its name is probably more meaningful. --Cliff
Re: Bucket API cleanup issues
On Tue, 27 Feb 2001, Greg Stein wrote:
Euh... I don't think we want another ring.
A simpler idea is to have the apr_bucket_pool structure contain a pointer to
an apr_bucket_heap structure. At cleanup time, you do the following:
...
So what you get here is a single allocation of a heap bucket. Then, you
lazily repoint all other buckets to the new heap bucket.
I came up with a really cool idea that's an even greater simplification of
this. It's coded up and ready to go, but I figured I'd give people a
chance to comment before I commit.
Basically, instead of having the pool bucket contain a pointer to a heap
bucket and having to fool with manipulating reference counts and keeping
track of when to destroy the old apr_bucket_pool struct, the
apr_bucket_pool *contains* the apr_bucket_heap struct that it will become
as its first element.
Since the apr_bucket_heap has an apr_bucket_refcount as ITS first element,
the pool and heap bucket share an apr_bucket_refcount (ie, no more
twiddling with reference counts).
All you have to do in the cleanup routine is malloc/memcpy out of the pool
and onto the heap and set p-data to NULL, which is the flag to later
pool_read() calls that they should morph their buckets. All that entails
is changing b-type to apr_bucket_type_heap... even the data pointer is
already correct because the apr_bucket_heap is the first element of
apr_bucket_pool.
After the morph, heap_read() and heap_destroy() take right over where
pool_read() and pool_destroy() left off. When the last reference is
heap_destroy'ed, heap_destroy() free's the b-data. As far as it knows,
b-data is just an apr_bucket_heap, but it unwittingly cleans up the
entire apr_bucket_pool struct for us since the b-data pointer never
changed.
=-)
Patch attached for comments.
--Cliff
Index: buckets/apr_buckets_pool.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_pool.c,v
retrieving revision 1.14
diff -u -d -r1.14 apr_buckets_pool.c
--- buckets/apr_buckets_pool.c 2001/02/28 17:52:43 1.14
+++ buckets/apr_buckets_pool.c 2001/02/28 19:46:48
@@ -57,71 +57,87 @@
static apr_status_t pool_bucket_cleanup(void *data)
{
-apr_bucket_pool *h = data;
-apr_bucket *b = h-b;
-apr_off_t start = b-start;
-apr_off_t length = b-length;
+apr_bucket_pool *p = data;
+apr_bucket_heap *h = p-heap; /* p-heap == data */
-b = apr_bucket_heap_make(b, h-base, b-length, 1, NULL);
-b-start = start;
-b-length = length;
/*
- * XXX: this is fubar... only the *first* apr_bucket to reference
- * the bucket is changed to a heap bucket... ALL references must
- * be changed or at least notified in some way. Fix is in the
- * works. --jcw
+ * If the pool gets cleaned up, we have to copy the data out
+ * of the pool and onto the heap. But the apr_bucket's that
+ * point to this pool bucket need to be notified such that
+ * they can morph themselves into a true heap bucket the next
+ * time they try to read. To avoid having to manipulate
+ * reference counts and b-data pointers, the apr_bucket_pool
+ * actually _contains_ an apr_bucket_heap as its first element,
+ * so the two share their apr_bucket_refcount member and you
+ * can typecast a pool bucket struct to make it look like a
+ * regular old heap bucket struct.
*/
+h-base = malloc(h-alloc_len); /* XXX: check for failure? */
+memcpy(h-base, p-base, h-alloc_len);
+p-base = NULL;
+
return APR_SUCCESS;
}
static apr_status_t pool_read(apr_bucket *b, const char **str,
apr_size_t *len, apr_read_type_e block)
{
-apr_bucket_pool *h = b-data;
+apr_bucket_pool *p = b-data;
+const char *base = p-base;
-*str = h-base + b-start;
+if (base == NULL) {
+/* pool has been cleaned up... morph to a heap bucket */
+apr_bucket_heap *h = p-heap;
+b-type = apr_bucket_type_heap;
+base = h-base;
+}
+*str = base + b-start;
*len = b-length;
return APR_SUCCESS;
}
static void pool_destroy(void *data)
{
-apr_bucket_pool *h = data;
+apr_bucket_pool *p = data;
-apr_pool_cleanup_kill(h-p, data, pool_bucket_cleanup);
if (apr_bucket_shared_destroy(data)) {
-free(h);
+apr_pool_cleanup_kill(p-pool, p, pool_bucket_cleanup);
+free(p);
}
}
APU_DECLARE(apr_bucket *) apr_bucket_pool_make(apr_bucket *b,
- const char *buf, apr_size_t length, apr_pool_t *p)
+ const char *buf, apr_size_t length, apr_pool_t *pool)
{
-apr_bucket_pool *h;
+apr_bucket_pool *p;
+apr_bucket_heap *h;
-h = malloc(sizeof(*h));
-if (h == NULL) {
+p = malloc(sizeof(*p));
+if (p == NULL) {
return NULL;
}
/* XXX: we lose the const qualifier here which indicates
* there's something screwy with the API...
*/
-
Re: cvs commit: apr-util/buckets apr_buckets_mmap.c
On 28 Feb 2001 [EMAIL PROTECTED] wrote: pass the right value to free() to prevent segfaults and corrupted heaps and other nasties Ugghghhh... damn me. I'm just causing all sorts of problems, aren't I? sigh --Cliff
Re: Bucket API cleanup issues
On Wed, 28 Feb 2001, Greg Stein wrote: 1) it is hard to tell that p and h refer to the same bucket. p-heap.foo is more obvious than h-foo. Fine by me. 2) Nit: I'm getting tired of these single letters for the bucket stuff. a as a variable for a bucket? e? Blarg. Using h and p fall right into that camp; especially p, given its use as a pool or char pointer in so many other contexts. I might suggest bh and bp. We've more or less standardized on one letter meaning a bucket, though... fixing this would be a massive change. For now, I think we need to stick with one letter for consistency. I'd be just as happy if that letter were 'b' (I don't know where a and e came from, but they're definitely there), or at least a letter mnemonic to the type of bucket it is (as I've done here). 3) You have two base members. That will be confusing as hell over the long haul. I'd recommend using just heap.base. Guess how to detect when a pool bucket is no longer in a pool? Right in one! Just set pool to NULL. And *please* put some comments to this effect in the apr_bucket_pool structure declaration(!) Done. 4) And no... I don't think we need to check malloc() results. If NULL is returned, then we'll segfault right away. That's fine in my book since we'd otherwise just call abort(). (if you really want to check, then call abort() on error or a pool's abort function) Yeah... I just put that comment in because there are comments just like it throughout the buckets code where there's a possibility of failure that we're not checking for, in case we decide at some point down the road to go back and put in checks for them. shrug That's it for now. I'll re-review when you check it in. Thanks. --Cliff
apr_brigade_cleanup()
Does anybody have any problem with me exposing apr_brigade_cleanup() as a
public function? It would not replace apr_brigade_destroy(), but it's
useful in some situations (eg Apache's mod_include) where there's a single
brigade object that you'd like to reuse by cleaning it out and putting
more buckets in it later. mod_include does this already with the brigade
it keeps in its ctx... the availability of apr_brigade_cleanup() to it
would reduce unnecessary code verbosity. The alternative, of course, is
to say that callers doing this should destroy the brigade when they want
to clean up and create a new one in its place, but that seems like a
waste of cycles and memory to me.
--Cliff
Index: buckets/apr_brigade.c
===
RCS file: /home/cvs/apr-util/buckets/apr_brigade.c,v
retrieving revision 1.12
diff -u -d -r1.12 apr_brigade.c
--- buckets/apr_brigade.c 2001/02/28 17:35:42 1.12
+++ buckets/apr_brigade.c 2001/04/01 22:18:41
@@ -67,7 +67,7 @@
#include sys/uio.h
#endif
-static apr_status_t apr_brigade_cleanup(void *data)
+APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data)
{
apr_bucket_brigade *b = data;
apr_bucket *e;
@@ -85,6 +85,7 @@
*/
return APR_SUCCESS;
}
+
APU_DECLARE(apr_status_t) apr_brigade_destroy(apr_bucket_brigade *b)
{
apr_pool_cleanup_kill(b-p, b, apr_brigade_cleanup);
Index: include/apr_buckets.h
===
RCS file: /home/cvs/apr-util/include/apr_buckets.h,v
retrieving revision 1.87
diff -u -d -r1.87 apr_buckets.h
--- include/apr_buckets.h 2001/03/01 04:46:13 1.87
+++ include/apr_buckets.h 2001/04/01 22:18:42
@@ -589,6 +589,20 @@
APU_DECLARE(apr_status_t) apr_brigade_destroy(apr_bucket_brigade *b);
/**
+ * empty out an entire bucket brigade. This includes destroying all of the
+ * buckets within the bucket brigade's bucket list. This is similar to
+ * apr_brigade_destroy(), except that it does not deregister the brigade's
+ * pool cleanup function.
+ * @tip Generally, you should use apr_brigade_destroy(). This function
+ * can be useful in situations where you have a single brigade that
+ * you wish to reuse many times by destroying all of the buckets in
+ * the brigade and putting new buckets into it later.
+ * @param b The bucket brigade to clean up
+ * @deffunc apr_status_t apr_brigade_cleanup(apr_bucket_brigade *b)
+ */
+APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data);
+
+/**
* Split a bucket brigade into two, such that the given bucket is the
* first in the new bucket brigade. This function is useful when a
* filter wants to pass only the initial part of a brigade to the next
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: apr-util/hooks apr_hooks.c
On 3 Apr 2001 [EMAIL PROTECTED] wrote: Index: apr_buckets.h === RCS file: /home/cvs/apr-util/include/apr_buckets.h,v retrieving revision 1.88 retrieving revision 1.89 diff -u -r1.88 -r1.89 --- apr_buckets.h 2001/04/02 19:02:15 1.88 +++ apr_buckets.h 2001/04/03 00:22:07 1.89 @@ -600,7 +600,7 @@ * @param b The bucket brigade to clean up * @deffunc apr_status_t apr_brigade_cleanup(apr_bucket_brigade *b) */ -APU_DECLARE(apr_status_t) apr_brigade_cleanup(void *data); +APU_DECLARE_NONSTD(apr_status_t) apr_brigade_cleanup(void *data); Crap... I tried, sorry. Thanks for the fix. If someone can give me a deterministic way to figure out which one to use without having to actually compile it under Windows, I'd much appreciate it. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets code question
On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
Oh buckets macro designer, is this is what we expected?
apr-util\buckets\apr_buckets_socket.c(142) : warning C4702: unreachable code
...
Not exactly. I've never seen a compiler complain about this macro before.
To save the others a little homework, it's complaining about
apr_bucket_do_create(), which is a macro that looks like this:
do {\
apr_bucket *b, *ap__b; \
b = calloc(1, sizeof(*b)); \
if (b == NULL) {\
return NULL;\
} \
ap__b = do_make;\
if (ap__b == NULL) {\
free(b);\
return NULL;\
} \
APR_RING_ELEM_INIT(ap__b, link);\
return ap__b; \
} while(0)
The only unreachable code that I see is the while(0), which is supposed
to get compiled away anyhow. do {} while(0) is the preferred way to do
multiline macros (to allow the macro user to place a semicolon after the
macro invocation without causing a syntax error), so I'll agree with
Ryan: it's a bug in your compiler.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: Buckets code question
On Wed, 11 Apr 2001, William A. Rowe, Jr. wrote:
} while (0);
return ap__b;
} while(0);
}
So the final While(0); is definately unreachable. No compiler error.
My only question, why do {} while(0); rather than {} ?
It's not do {} while(0); rather than {} , it's that rather than {}; .
The semicolon is added by the caller of apr_bucket_do_create(), who treats
it as a function call.
apr_bucket_do_create(); ---the semicolon
{}; is a syntax error on some platforms because empty statements are not
allowed, and here the semicolon is essentially delimiting an empty
statement which occurs between the closing brace and the semicolon.
do {} while(0); is actually the preferred way to get around this issue,
since it gets compiled away by an optimizing compiler anyhow.
Our problem, obviously, is the explicit return within the block. Maybe we
could change the call to look like this:
{
apr_bucket *the_new_bucket;
apr_bucket_do_create(the_new_bucket, ...);
return the_new_bucket;
}
And adjust apr_bucket_do_create to look like this:
#define apr_bucket_do_create(ap__b,do_make) \
do {\
apr_bucket *b; \
b = calloc(1, sizeof(*b)); \
if (b == NULL) {\
return NULL;\
} \
ap__b = do_make;\
if (ap__b == NULL) {\
free(b);\
return NULL;\
} \
APR_RING_ELEM_INIT(ap__b, link);\
} while(0)
While this causes some very minor redundancy in the
apr_bucket_foo_create() functions, it has several benefits:
(1) both warnings and errors eliminated
(2) the magical return is made explicit, which makes
apr_bucket_foo_create() easier to understand for someone not
familiar with apr_bucket_do_create()'s innards.
Thoughts?
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: Buckets code question
On Wed, 11 Apr 2001, Greg Marr wrote:
I agree. Having macros that look like functions, but have return
statements in them, is bad. It also prevents those macros from being
made into inline functions without changing all the places that call
them.
I'll consider that three +1's on concept... me, Greg Marr (yes?), and
FirstBill. I'll touch this up and commit it this afternoon. I'd also
like to remove the return NULL; lines in the macro. There are two ways to
do that:
1) ap__b = NULL; continue;
2) get rid of the malloc failure checking entirely
3) ignore this issue and leave the return NULL's in there.
I prefer #1, but I predict being overruled by the group on this since many
of you think it's better to segfault than attempt to gracefully handle
malloc failures. So assuming #2 is preferred by the group, does everyone
like the following as the new apr_bucket_do_create macro?
#define apr_bucket_do_create(ap__b,do_make) \
do {\
apr_bucket *b; \
b = calloc(1, sizeof(*b)); \
ap__b = do_make;\
APR_RING_ELEM_INIT(ap__b, link);\
} while(0)
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: Buckets code question
On Wed, 11 Apr 2001 [EMAIL PROTECTED] wrote:
#define apr_bucket_do_create(ap__b,do_make) \
do {\
apr_bucket *b; \
b = calloc(1, sizeof(*b)); \
ap__b = do_make;\
APR_RING_ELEM_INIT(ap__b, link);\
} while(0)
+1
Upon further inspection, the also magical apr_bucket *b is also now
unnecessary since ap__b is no longer tested for NULL. So
apr_bucket_do_create() collapses even further down to just:
#define apr_bucket_do_create(ap__b,do_make) \
do {\
ap__b = calloc(1, sizeof(*ap__b)); \
ap__b = do_make;\
APR_RING_ELEM_INIT(ap__b, link);\
} while(0)
And callers do something like this:
{
apr_bucket *b;
apr_bucket_do_create(b, apr_bucket_foo_make(b, foo));
return b;
}
But this is screwy for two reasons: the double assignments to ap__b in the
macro and the seeming call to rv = apr_bucket_foo_make(b, foo) BEFORE
calling apr_bucket_do_create(b, rv) (if apr_bucket_do_create() were a
function).
It seems to me that apr_bucket_do_create() has outlived its usefulness.
It's only a few lines long at this point, which are less tricky-looking
when done inline than when done as a macro. So what I'm going to do
unless someone objects strongly is to nix it altogether and just do this:
#define APR_BUCKET_INIT(b) APR_RING_ELEM_INIT((b), link)
APU_DECLARE(apr_bucket *) apr_bucket_foo_create(apr_foo_t *foo)
{
apr_bucket *b = (apr_bucket *)calloc(1, sizeof(*b));
APR_BUCKET_INIT(b);
return apr_bucket_foo_make(b, foo);
}
Note that I've switched the effective order of APR_BUCKET_INIT() and
apr_bucket_foo_make, but it shouldn't matter since the two never touch
each other's data (and if they did, it would be better for the init to
happen first anyway).
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: apr-util/include apr_buckets.h
On 11 Apr 2001 [EMAIL PROTECTED] wrote: jwoolley01/04/11 12:07:11 Modified:server error_bucket.c .CHANGES buckets apr_buckets_eos.c apr_buckets_file.c apr_buckets_flush.c apr_buckets_heap.c apr_buckets_mmap.c apr_buckets_pipe.c apr_buckets_pool.c apr_buckets_simple.c apr_buckets_socket.c include apr_buckets.h Log: Removed apr_bucket_do_create() macro, which was causing warnings about unreachable code in some compilers (notably MSVC). What used to be done by this macro is now done inline in the various apr_bucket_foo_create() functions. [Cliff Woolley] I presume that your warnings are now gone, right? --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: [PATCH] First-cut of APR_READWRITE locks...
A few nits:
@@ -626,7 +626,7 @@
dnl BSD 4.4 originated 'q'. Solaris is more popular and
dnl doesn't support 'q'. Solaris wins. Exceptions can
dnl go to the OS-dependent section.
-int64_t_fmt='#define APR_INT64_T_FMT lld'
+int64_t_fmt='#define APR_INT64_T_FMT qd'
int64_value=long long
long_value=long long
elif test $ac_cv_sizeof_long_double = 8; then
Remind me why this change is necessary? What about the comment above that
that gives a good reason not to use qd?
Index: locks/os2/locks.c
===
RCS file: /home/cvspublic/apr/locks/os2/locks.c,v
retrieving revision 1.27
diff -u -r1.27 locks.c
--- locks/os2/locks.c 2001/03/19 12:43:25 1.27
+++ locks/os2/locks.c 2001/04/28 20:06:44
@@ -142,7 +142,10 @@
return APR_OS2_STATUS(rc);
}
-
+apr_status_t apr_lock_acquire_rw(apr_lock_t *lock, apr_readerwriter_e e)
+{
+return APR_OS2_STATUS(APR_ENOTIMPL);
+}
apr_status_t apr_lock_release(apr_lock_t *lock)
{
This should just be return APR_ENOTIMPL;. Only use APR_OS2_STATUS to
convert from an os-style error to an APR-style error code.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Sat, 28 Apr 2001, Justin Erenkrantz wrote: Aren't some of the resource leakages in httpd coming from the fact that the buckets aren't cleaning up after themselves? They do reference counting, but my guess is that they should close their private structures as well when they are no longer around. There's a sideband discussion on this. Ryan's strongly opposed to closing the files/mmaps when the last bucket goes away, as I believe he pointed out in another response to this message, because it incurs an extra syscall during the request proper. I've argued against that to little/no avail. mod_core opens a file and gives it apr_bucket_file_create. After that point, shouldn't the bucket own the fd and close it when it is done? That's what Greg and I have said. But we just can't convince Ryan. And admittedly he has a good reason... it's a toss-up. There are pros and cons either way. 1.3 did it by letting the pool do the work, so I guess we should too. I'm going to implement this in my local tree and see if it blows up things spectacularly. -- justin There are issues to be aware of if you're going to try it. For example, you can't count on an MMAP surviving if its underlying file gets closed. Assuming Ryan doesn't object too strongly, I plan on changing the file buckets very soon to be more like pool buckets, in that when one file bucket gets MMAPed, all of its siblings get lazily morphed to MMAP-type as well. The main issue you need to watch out for is in which cases you can safely close the file--that's tough to do with the current code but should be easy with the change I propose. Try it on MMAP's if you want... that should be safe, or at least safer than closing the files. PS: Another reason not to close the files right now is that it would break the module that caches FD's. I've got a proposed fix for that as well. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Sat, 28 Apr 2001 [EMAIL PROTECTED] wrote: I didn't get a chance to respond to that message off-list, but I would not be opposed to making all references to a file bucket morph to MMAP buckets when one does. Cool deal. That's +1 from me and Ryan (and I'm assuming Greg, since he didn't object in our sideband discussion). I'll do that ASAP. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Sat, 28 Apr 2001 [EMAIL PROTECTED] wrote: Yes, which is exactly the bug that Dean pointed out over three months, ago, and which I posted a potential fix for. Cliff Woolley is actually implementing it AFAIK. I didn't know I was, but I am now. =-) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Sat, 28 Apr 2001 [EMAIL PROTECTED] wrote: I didn't know I was, but I am now. =-) Didn't mean to volunteer you. If you don't want to, or don't have the time, just say so, and I'll do it tomorrow. I thought in one of your messages you had said you were, which is why I didn't do it earlier today. No problem. I think Justin might beat me to it (not trying to volunteer HIM, mind you grin). It'll get done one way or the other. =-) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Sat, 28 Apr 2001, Roy T. Fielding wrote: On Sat, Apr 28, 2001 at 09:12:16PM -0700, [EMAIL PROTECTED] wrote: the file is closed. We need a loop at the end of the core_output_filter that just does the conversion to a heap bucket. The logic should be that anything in the pipeline with a size greater than a minimum write (around 4KB I think -- there is a symbol for it somewhere) should be written out to the socket and freed before the worker moves on to looking for a next request. Basically, we only want to hold onto data if we don't have enough in the buffer to justify writing what we have now. Right, that's the idea. Anything under the threshold, though, will get buffered into a heap bucket and the file/mmap/whatever buckets in the original stream get destroyed so that r-pool can get safely cleaned up. (And it's 8KB, I'm 99.9% sure. AP_MIN_BYTES_TO_WRITE ought to be the symbol of which you speak.) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Mon, 30 Apr 2001, Bill Stoddard wrote: I lost the original context for this discussion... Exactly when will a file bucket morph to an MMAP bucket? My concern is not to break using apr_sendfile(), which on some OSes can provide a big performance boost. I thought about that, and it is a potential issue, but I believe I have a cure. Basically, all references to a file bucket would morph (lazily, as with pool buckets) to MMAP-type when any one of them gets run through apr_bucket_read() (and therefore MMAP'ed). The problem is this: since the MMAP is created in the same pool as the apr_file_t, we leak MMAP's for file descriptors that are repeatedly passed through apr_bucket_read(). Take, for example, mod_file_cache in Apache. Each time a file descriptor is served up, we leak another MMAP. That's bad. There are only three solutions that I can think of: (1) Let mmap_destroy() in the buckets code delete the MMAP when the last bucket referencing it goes away. This has been basically vetoed by Ryan, citing the fact that it increases the number of system calls that happen during request processing. (2) Put two pools in apr_file_t: one that contains the file and one (possibly the same) that any apr_mmap_t's of that file should be stored in. This would work but is a hack, is altogether ugly, potentially wastes time repeatedly MMAPing the file, and I myself am just about -1 on it. (3) Make all file bucket references lazily morph to MMAP type when any one of them does (as I'm proposing). This is good for preventing resource leakage, but could potentially limit the use of apr_sendfile(), particularly in the case of mod_file_cache. On the other hand, this would actually be *good* during a regular request, since the work of MMAPing the file would only have to be done once, ever, for any given open file. So the only real problem is how to fix the mod_file_cache case, where the FD could potentially be sendfile'd on later requests and it's more performant to do that than to use an MMAP just because we have one available. Here's how that's done, and why this change is good: The beauty is in the lazy morphing. mod_file_cache will be changed to no longer use ap_send_fd, but instead keep a cached file bucket for each FD it caches. This is good for another reason (it saves a malloc per request), but that's beside the point. For each request, it makes an apr_bucket_copy() of its cached file bucket and sends that down the filter stack. If at some point during request processing, that copy gets changed to an MMAP bucket, then all of its siblings (including the master one in the cache) will also have access to that MMAP and will morph automatically the next time they're read. But the master copy in the cache will never BE read! So the copy in the cache always remains of file type, not mmap type, and copies of the master made to serve future requests will start out as file type as well (enabling sendfile for those future requests). Even better, if one of those later requests decides it does need to do a read on the file bucket and would have MMAPed it, it discovers that an MMAP of the file is already available, it just changes its type and reads the MMAP with virtually zero extra work incurred. Sound good? =-) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Mon, 30 Apr 2001, Bill Stoddard wrote: the next time they're read. But the master copy in the cache will never BE read! So the copy in the cache always remains of file type, not mmap type, and copies of the master made to serve future requests will start out as file type as well (enabling sendfile for those future requests). Even better, if one of those later requests decides it does need to do a read on the file bucket and would have MMAPed it, it discovers that an MMAP of the file is already available, it just changes its type and reads the MMAP with virtually zero extra work incurred. Does apr_bucket_copy() just make a duplicate bucket in a different pool using the same fd? Should be minimal overhead if true. I don't see any problems with caching the whole file bucket rather than just the apr_file_t. I am concerned with MMAP'ing a file and leaving that MMAPed file associated with the cached file bucket. It is reasonable to cache literally 100's of open file descriptors (on Windows anyway) but I have a big concern about the hit on memory of MMAPing the contents of those files. Guess it is better than the MMAP leak though :-) apr_bucket_copy() calloc's a new apr_bucket struct and directs its data pointer at the same private data entity as the original bucket (in this case, that's an apr_bucket_file which points to an apr_file_t). That's better than caching just the file handle, which incurs the calloc of the apr_bucket *and* a new apr_bucket_file to point to the cached apr_file_t. This is definitely better than the leak. =-) And nothing says that the cache can't apr_mmap_delete() the MMAP associated with the master file bucket as long as refcount==1 (ie, there are no requests using that file/mmap currently in progress) if it decides it has too many MMAPs laying around. It's possible that the file will be re-MMAPed by a later request, but the MMAPs could be just cycled through in an LRU fashion. This would leak one palloc'ed apr_mmap_t in the cache's pool, however. It's a trade-off. Which is better: having potentially as many MMAPs open as you have file handles cached, or growing the size of the cache pool by sizeof(apr_mmap_t) each time you delete and recreate an MMAP for a file? In either case, this is still much better than the current situation (the leak), which has both problems, only worse... ;-] --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Buckets destroy not cleaning up private structures?
On Mon, 30 Apr 2001, Cliff Woolley wrote: This is definitely better than the leak. =-) And nothing says that the cache can't apr_mmap_delete() the MMAP associated with the master file bucket as long as refcount==1 (ie, there are no requests using that file/mmap currently in progress) if it decides it has too many MMAPs laying around. It's possible that the file will be re-MMAPed by a later request, but the MMAPs could be just cycled through in an LRU fashion. This would leak one palloc'ed apr_mmap_t in the cache's pool, however. It's a trade-off. Which is better: having potentially as many MMAPs open as you have file handles cached, or growing the size of the cache pool by sizeof(apr_mmap_t) each time you delete and recreate an MMAP for a file? In either case, this is still much better than the current situation (the leak), which has both problems, only worse... ;-] I had an idea for fixing the leaking of the apr_mmap_t structure at lunch, and thought I'd throw it out here for discussion. One way around this is to have a variant of apr_mmap_create() that accepts an existing apr_mmap_t structure and fills it out, rather than preemptively palloc'ing a new apr_mmap_t and using that one. If we had that, then the apr_file_t could contain a pointer to the apr_mmap_t for that file. The first time the file is MMAP'ed, the apr_mmap_t that's allocated is hung off the apr_file_t's pointer. The apr_mmap_t has a flag in it that indicates active or not. When the mmap is apr_mmap_delete'd, the flag is set inactive. If the file gets re-MMAP'ed, the apr_mmap_t created before is reused and set active again. I don't know if this is useful or not, but it's one idea. Just a thought. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: [WISH] shared memory based apr_pool_t
On Sat, 5 May 2001, Ralf S. Engelschall wrote: A wish to our APR hackers: For mod_ssl and certainly also for other purposes we really need shared memory based apr_pool_t's, i.e., pools which allocate their memory not from the heap but from a shared memory segment. This would allow us to complete kick out the large 100KB third-party ssl_util_table.* stuff by using just a standard APR hash inside an APR pool which is located in a shared memory segment. For EAPI in Apache 1.3 there was a patch against alloc.c which provided such a functionality, although mod_ssl was never adjusted to use this feature directly. Sounds like the SMS (stackable memory system) stuff that the Samba-TNG guys have proposed for APR might be worth some investigation... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: apr_memory_system.c;apr_memory_system.h
On Sat, 5 May 2001 [EMAIL PROTECTED] wrote: This is the stackable memory that the Samba TNG guys put together. This is very impressive IMHO. I believe this is the way to move forward with APR's memory requirements. I would like to commit this to APR to get people hacking on this. To that end, unless somebody objects, I will commit this on Monday morning. Once it is committed, I expect people can use this to re-implement pools (same semantics, same API, just different under the covers), and shared memory. +1 -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/memory/unix apr_sms.c
On 19 May 2001 [EMAIL PROTECTED] wrote:
--- apr_sms.c 2001/05/19 13:53:06 1.3
+++ apr_sms.c 2001/05/19 15:35:45 1.4
@@ -193,7 +193,7 @@
mem_sys-accounting_mem_sys = mem_sys;
if (parent_mem_sys != NULL){
-if (mem_sys-sibling_mem_sys = parent_mem_sys-child_mem_sys){
+if ((mem_sys-sibling_mem_sys = parent_mem_sys-child_mem_sys)){
mem_sys-sibling_mem_sys-ref_mem_sys =
mem_sys-sibling_mem_sys;
}
mem_sys-ref_mem_sys = parent_mem_sys-child_mem_sys;
Just to verify (haven't looked at this section of the code itself yet),
assignment IS what's intended here, right? If so, a ((foo = bar) != NULL)
might make that more clear.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: apr/misc/unix errorcodes.c
On 19 May 2001 [EMAIL PROTECTED] wrote: --- apr_errno.h 2001/05/13 12:03:28 1.62 +++ apr_errno.h 2001/05/19 13:53:06 1.63 @@ -180,9 +180,11 @@ * APR_EGENERAL General failure (specific information not available) * APR_EBADIP The specified IP address is invalid * APR_EBADMASK The specified netmask is invalid - * APR_ENOCLEANUP There is no memory cleanup available - * APR_EMEMSYSAn invalid memory system was passed in to an - *apr_sms function + * APR_ENOCLEANUP There is no memory cleanup available + * APR_EMEMSYS An invalid memory system was passed in to an + * apr_sms function + * APR_EMEMFUNC A function was called that isn't available in the + * selected memory system I really dislike the proliferation of single use error return codes. I can't complain too loudly about what you've done here because you've followed precedent AFAICT... I just don't like the precedent. =-) For example, I don't see why these newest ones that've been added can't be handled within the definition of existing codes. Why can't APR_ENOTIMPL and APR_EINVAL do the job here? Then there's the problem of an error code sounding bigger in scope than it is. APR_ENOCLEANUP is a perfect example. If we're going to have a code named like that, then why should it be limited to being used only by the memory system? Why wouldn't that be used in more general cases? (I can't think of a good example at the moment, but I hope you see what I'm getting at anyway.) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: [PATCH] APR Stackable Memory System
On Sun, 20 May 2001, Sander Striker wrote: David is gone for a week, but I already wrote some stuff that might be worth reviewing/commiting. Attached are a few patches: I'll take a look at these later today and commit if nobody beats me to it. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
RE: [PATCH] APR Stackable Memory System
On Mon, 21 May 2001, Sander Striker wrote: dreid Why not stick to the standard apr format of dreid dreid apr_status_t abort_fn(char *sourcefile, int lineno); striker Ahh, there is such a function? I need to do a lot of digging into APR. Hmm... yeah, that's a point. Then again, if the abort_fn is basically just abort(), then it's pretty easy to do a debugger backtrace and get all the sourcefile/lineno information for the entire call stack that way. I really don't think we should be adding sourcefile/lineno arguments to ALL of the SMS functions... Well, whatever. Other opinions? --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr STATUS
On 31 May 2001 [EMAIL PROTECTED] wrote: @@ -65,6 +65,8 @@ * add apr_crypt() and APR_HAS_CRYPT for apps to determine whether the crypt() function is available, and a way to call it (whether it is located in libc, libcrypt, or libufc) + Justin says: Should apr_crypt() be in apr-util? + Status: Greg +1 (volunteers) From the description, it sounds to me like apr_crypt is a portability wrapper, not a function that is inherently portable like all the things in apr-util. Therefore it belongs in APR itself IMO... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr STATUS
On Wed, 30 May 2001, Justin Erenkrantz wrote: I think that wherever these other crypto functions go, so should crypt(). Yeah, but... crypt() is slightly different than the other crypto algorithms as my systems have it as a standard library call. But, not all systems will have it. Which is exactly why it should go in APR and not APR-util. APR-util only contains functions that are complete, portable implementations of their respective functionality (eg the SHA1 implementation). It was born when it was decided that APR's focus should be on creating a library that hides platform-specific details of functionality (eg locking, mmaps, DSOs, etc) that is non-portable. That's the aim of apr_crypt()... to make a portable way to access the system's crypt() if there is one. If apr_crypt() were to be a from-scratch implementation of crypt() that was written with inherently portable code, then yes, it would go in apr-util. But that's not the goal, so it goes in APR. My $0.02. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: Legal issues, DES
On Sun, 3 Jun 2001, Sander Striker wrote: Maybe a bit off topic, but are there still legal issues with DES implementations? I mean, if someone submitted a patch would it be possible to include des in apr-util/crypto, or would this be problematic. DES is still in use in a variety of things, so providing the functionality wouldn't be that strange I think. Legal issues aside, my only worry is that we seem to be duplicating more and more of the effort of the OpenSSL team... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/include apr_md4.h
On Mon, 4 Jun 2001, Greg Stein wrote: On this topic... note that the new SMS stuff has a bunch of argument checking, too. All that needs to be yanked, too. I just sent Jeff an email like five minutes ago asking what he thought about all that. I agree, it needs to go. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/misc/unix errorcodes.c
On Tue, 5 Jun 2001, David Reid wrote: * Remove the unnecessary parameter checks and the extra error codes that went along with them. The APR policy is to segfault on a NULL parameter rather than silently returning some error code that the caller might not check anyway. Can't say I agree 100% with this, but if you say so, it must be. BTW, where is this written/documented?? I don't know if it IS documented, but I've been told this a thousand times by various people since I came on board, and I've never once been told the opposite. shrug * Fix a misnamed APR_MEMORY_ASSERT - APR_ASSERT_MEMORY, which was causing apr_assert_memory() never to be compiled. Also fix a syntax error in s/apr_assert_memory/apr_sms_assert/ So sue me. that function that's been there since rev 1.1 of apr_sms.c, which no one's ever noticed because they never compiled it before. If you checked you'd see that the misnamed assert tag was added in revision 1.5 of the original file, and so the code that you claim was never built was building quite happily up till that point. As for the screw up on the naming, don't believe I missed that! Huh? I did check. The misnamed tag and the missing semicolon had both been there since rev 1.1 of apr_sms.c. Which file are you talking about? So anyway, apr_sms.c was using both APR_MEMORY_ASSERT and APR_ASSERT_MEMORY since rev 1.1, but only APR_ASSERT_MEMORY was ever actually defined by configure. shrug Oh well. No big deal. It's fixed now. Look, I wasn't trying to irritate anybody with this commit (or commit message, for that matter). The commit message was only meant to point out that I'd found a typo and tried to fix it so that somebody would know to look closely at that part of the commit. Sander did so, found that I'd in fact made a mistake, and I fixed it. That's what this is all about. As for the commit, I was just trying to implement what seemed to be the collective will of the group given what's been said by Greg, Jeff, Justin, myself, and others. That's all. If it came off as a personal thing, I apologize... that wasn't the intent. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/locks/win32 locks.c
On 5 Jun 2001 [EMAIL PROTECTED] wrote:
--- apr.h.in2001/05/10 18:09:26 1.80
+++ apr.h.in2001/06/05 08:15:05 1.81
@@ -174,6 +174,8 @@
typedef @off_t_value@ apr_off_t;
typedef @socklen_t_value@ apr_socklen_t;
+typedef struct apr_lock_tapr_lock_t;
+typedef struct apr_sms_t apr_sms_t;
...
typedef enum {APR_READER, APR_WRITER} apr_readerwriter_e;
-
-typedef struct apr_lock_t apr_lock_t;
...
-typedef struct apr_sms_t apr_sms_t;
-
Just out of curiousity, why was this necessary?
--Cliff
Re: cvs commit: apr-site patches.html
On 5 Jun 2001 [EMAIL PROTECTED] wrote: As discussed on [EMAIL PROTECTED] - add a comment about the assert/checking input debate. Our policy is don't check input values. Feel free to change the wording. It's something, but we've had two longish threads in the last day about this... Doh, you beat me to it. Gotta love that up-to-date check failed commit error. =-) Anyway, this looks fine to me. I was going to take more of a list-ified approach (I admit it, I'm a listaholic), something like this: P We also have very high expectations for code quality, and to us this means following these otherwise-undocumented coding policies: UL LIAvoid the use of excessive static buffers LIUse the memory pool mechanism to ensure proper cleanup LIWrite thread-safe code LIWe expect one or two levels of optimizations to be applied: Is a bitmask faster for this? Is a strchr() faster than an index()? Etc. LIParameter checking is generally unnecessary (we prefer for the code to segfault in these cases as it makes for easier debugging) LIAssertions are not needed in fatal conditions that would result in an immediate segfault anyway LI... /UL This list is not an exclusive one; it'd be nice if all of the group's policies were documented, but they're not yet. We'll add them here as they come up. /P But I'm perfectly fine with leaving the way you've done it. Thanks. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/locks/unix locks.c
On Tue, 5 Jun 2001, Ben Laurie wrote:
apr_status_t apr_os_lock_put(apr_lock_t **lock, apr_os_lock_t *thelock,
apr_pool_t *pool)
{
-if (cont == NULL) {
+if (pool == NULL) {
return APR_ENOPOOL;
}
if ((*lock) == NULL) {
Isn't this another example where we should just die instead of returning
an error?
I think so, yes. Many of the APR_ENOfoo codes are on my hit-list. =-)
For example:
* APR_ENOPOOL APR was not provided a pool with which to allocate memory
* APR_ENOFILE APR was not given a file structure
* APR_ENOPROC APR was not given a process structure
* APR_ENOTIME APR was not given a time structure
* APR_ENODIR APR was not given a directory structure
* APR_ENOLOCK APR was not given a lock structure
* APR_ENOPOLL APR was not given a poll structure
* APR_ENOSOCKETAPR was not given a socket
* APR_ENOTHREADAPR was not given a thread structure
* APR_ENOTHDKEYAPR was not given a thread key structure
I'm equally hostile toward five or six of the other codes in there as
well, but these are a good enough example.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: file/mmap buckets, subrequests, pools, 2.0.18
On Wed, 6 Jun 2001, Greg Stein wrote: Tough call. A POOL bucket is nominally safer than a HEAP bucket. But *IF* we are careful to ensure the HEAP bucket gets placed into a brigade, then we are guaranteed it will be tossed. That said, we've seen issues with malloc() efficiency in the bucket code. Using a POOL bucket has obvious performance implications. But on the gripping hand, we have the up-and-coming fast-malloc patches from FirstBill, et al. Those patches could mean that efficiency will not be the determination between HEAP and POOL. I've been somewhat involved in those discussions, and a patch is still a ways off, I think. The less we can allocate on the heap, the better. I seriously doubt that, regardless of what patch is finally applied, allocating on the heap will ever be faster than allocating into a pool. It might come close to as fast, but we're currently not even close, as we're all aware. :-/ On the other hand... As long as you ap_save_brigade into the right pool in the first place, the double copying wouldn't be an issue, but there are probably cases where you can't know which is the right pool. So you're probably right that using a POOL bucket opens us up to special cases where multiple copies might be made (even POOL-parentPOOL-...-HEAP in a worst-case scenario, I imagine), so it's probably best to go with a HEAP bucket. All the other bucket-morphings that copy into memory use HEAP buckets, so we might as well be consistent. sigh --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: httpd-2.0/server request.c
On 6 Jun 2001, Jeff Trawick wrote: Wow, so we have APR_NOFILE _and_ APR_ENOFILE? That's awfully confusing, don't you think? err umm no real opinion as to confusability... one is an error code (APR_E*) and one isn't Yeah, but if you're unfamiliar with the two and haven't actually gone out and looked at the header files, you might not even notice the 'E' clearly APR_ENOFILE is useless and should be removed, so any possible confusion should go away Clearly. =-) After that, I don't really have a problem with leaving APR_NOFILE as it is. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/threadproc/unix thread.c
On Wed, 6 Jun 2001, Justin Erenkrantz wrote: On Wed, Jun 06, 2001 at 06:12:17PM -, [EMAIL PROTECTED] wrote: - add an apr_pool_t to the sms structure -1 (non-veto, but awfully close). Uh, why are we doing this? I thought that a pool would be defined in terms of a sms (not now, but at some point). This would not allow that to happen. I'm still not entirely sold on the fact that sms needs locks. I think the locks can be handled at a higher level than sms (i.e. a pool). I was thinking the exact same thing, actually... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/memory/unix apr_sms.c
On 6 Jun 2001 [EMAIL PROTECTED] wrote: If we don't have a lock and unlock function in an sms module then it's not an error as this is allowed. Add a comment to this effect to make it clearer. if (!mem_sys-lock_fn) -return APR_ENOTIMPL; +return APR_SUCCESS; return mem_sys-lock_fn(mem_sys); Ahh, that makes sense. My bad. Thanks, David. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
buildconf
Can somebody remind me why it is that APR-util has 'buildconf.sh' and APR and Apache have just plain 'buildconf'? ISTR that we were going to migrate to 'buildconf.sh' and that it just only got done half-way. Is that right? Thanks, Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/threadproc/unix thread.c
On Wed, 6 Jun 2001, Justin Erenkrantz wrote: I think the thing is that I've seen the sms as slightly different than what it was originally posted as. So, I might be in the minority here. I think we are seeing two different views of what an sms should be. My -1 was non-veto, so it doesn't stop you. It just registers my dissent. -- justin The group has told me before that, since all code matters require a consensus, -1 always means veto. If you really, really, really don't like it but don't want to veto it, use -0.5 or -0.99 or something. =-) Only in non-consensus-requiring matters do -1's count as negative votes, but those matters seem to only exist in administrative matters, not code matters. shrug --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/threadproc/unix thread.c
On Wed, 6 Jun 2001, Justin Erenkrantz wrote: Ah. Okay. I thought I've seen people say -1 (non-veto) before. Fine, You have. We're inconsistent as usual. =-) Just thought I'd mention it since it's been mentioned to me in a very similar situation. consider my vote -0.9 (because I'm that close to vetoing it, but I'm not going to do that). Hopefully, my intention *not* to veto is and was clear. It was. My apologies. No sweat. =-) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
RE: cvs commit: apr/memory/unix apr_sms.c apr_sms_std.c apr_sms_tracking.c
On Fri, 8 Jun 2001, Sander Striker wrote: Oh, yes, I actually want to do s/mem_sys/sms/ aswell, but that can I'm hoping that includes the foo_mem_sys's that are in the apr_sms_t structure? sms-parent_mem_sys etc seems both redunant and too much typing. sms-parent would be ideal, but then again sms-ref wouldn't make much sense, so for consistency I could live with sms-parent_sms. =-) PS: If this post makes no sense, please tell me. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: file/mmap buckets, subrequests, pools, 2.0.18
On Fri, 8 Jun 2001, Roy T. Fielding wrote: Seriously, though, there is no reason for the setaside function to need a full memory allocation system (pool) passed just to do a bit of byte stuffing within a buffer. It's not just a bit of byte stuffing within a buffer at all. The pool has some lifetime associated with it. The reason for passing in a pool is to be able to say to the buckets code this resource should live at least THIS long. That also implies that some data structures (not data!) will need to be copied into that pool, ie an apr_file_t or an apr_mmap_t. It would only work to just pass an arbitrary storage pointer if we actually copied all of the data represented by the bucket into that buffer, and that's not the idea at all. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: file/mmap buckets, subrequests, pools, 2.0.18
On Mon, 11 Jun 2001, Roy T. Fielding wrote: But that's not thinking at a wide enough scope. There are lots of reasons why I chose the analogy of bucket brigades for the original design, but one is that we want to put the fire out. Web applications are very sensitive to latency. The only way to get both network-efficient packets and avoid suffering from unnecessary latency is to prevent more than a specific amount of data from getting stuck in the filter chain. Sometimes that isn't possible due to the nature of a filter (batch-like processing), but in general we do not want to setaside anything but raw data, and only then when it is less than 4KB. [Not 9000 bytes -- 4KB is more than sufficient to justify a network write.] Even a cached file handle is better copied to memory at that point if doing so avoids the need to mutex a bunch of code. Maybe what we need is two functions, I suppose that makes sense. one for moving data to a preallocated buffer Except for the preallocated part, plain old read() fits the bill. I would say that's good enough, but I'm currently on a mission to get rid of as many malloc's as possible. So let's just suppose for a moment that we decided to implemented something new: I guess what you're asking for is a read_into() function? I could live with that. I don't know what exactly that would do to the brigade, but I imagine there's some something that could be done. Let's say we have a pipe bucket that we have to handle. If read_into() is called on that bucket, the brigade is busted unless we morph the pipe bucket simultaneously and allow the brigade to include a pointer to that arbitrary data. But it's hard to morph the bucket in this case, because the bucket doesn't own the storage, and therefore it's hard to know what bucket type to use. I guess when the bucket doesn't own the storage that's technically the textbook case for an immortal bucket, but I haven't yet convinved myself that that will work in this case. I'll have to think about this some more... and another for setaside using a pool. The problem with a single function is that buffering is very filter-specific. Yep. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/include apr_sms_blocks.h
On 13 Jun 2001 [EMAIL PROTECTED] wrote:
Log:
Add a new sms module.
This one basically takes a slice of memory (defaults to 8k) and then
hands out pieces of it of a given size as fast as possible. When free
is called on an allocated block it's added to a free list and then
re-allocated.
Thanks! Buckets memory allocation, here I come.
A few suggestions:
static void *apr_sms_blocks_malloc(apr_sms_t *sms,
apr_size_t size)
{
void *mem;
if (size BLOCKS_T(sms)-block_sz)
return NULL;
if ((mem = BLOCKS_T(sms)-free_list) != NULL) {
BLOCKS_T(sms)-free_list = ((block_t*)mem)-nxt;
return mem;
}
mem = BLOCKS_T(sms)-ptr;
BLOCKS_T(sms)-ptr += BLOCKS_T(sms)-block_sz;
if (BLOCKS_T(sms)-ptr BLOCKS_T(sms)-endp)
return NULL;
return mem;
}
Instead of returning NULL if size block_sz, how about calling
apr_sms_malloc(sms-parent)? Same goes if the free list is empty and the
initial block is completely used. So if the parent is an apr_sms_std,
then we fall back on plain old malloc() at the cost of lower performance.
Say, for example, that you allocate 1K blocks out of your 8K hunk of
memory, and all 8 blocks are allocated. When we ask for a 9th one, it'd
be much nicer to get one a little slower than to not get one at all.
static void *apr_sms_blocks_calloc(apr_sms_t *sms,
apr_size_t size)
{
void *mem;
if (size BLOCKS_T(sms)-block_sz)
return NULL;
if ((mem = BLOCKS_T(sms)-free_list) != NULL) {
BLOCKS_T(sms)-free_list = ((block_t*)mem)-nxt;
return mem;
}
mem = BLOCKS_T(sms)-ptr;
BLOCKS_T(sms)-ptr += BLOCKS_T(sms)-block_sz;
if (BLOCKS_T(sms)-ptr BLOCKS_T(sms)-endp)
return NULL;
memset(mem, '\0', BLOCKS_T(sms)-block_sz);
return mem;
}
AFAICT this doesn't need to be implemented... apr_sms_default_calloc()
should be fine.
Speaking of which, the same could be done for apr_sms_std_calloc if
!HAVE_CALLOC, but then again, I'm of the mind that we don't even need to
check for calloc() at all and can just assume it exists. The buckets code
has used calloc() without checking for its existence first for ages, and
nobody's ever complained...
static apr_status_t apr_sms_blocks_reset(apr_sms_t *sms)
{
BLOCKS_T(sms)-ptr = (char *)sms + SIZEOF_BLOCKS_T;
BLOCKS_T(sms)-free_list = NULL;
return APR_SUCCESS;
}
I had been about to ask why you didn't just carve up the -ptr into as
many blocks as possible in the create function and put them all onto the
free_list ahead of time so that you can use only the free_list in
_malloc()... but now I see why. =-) Clever.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: apr/include apr_sms_blocks.h
On Thu, 14 Jun 2001, David Reid wrote: The bucket structures are done from bms, allocations for data can come from either ams (some other type of sms yet to be added) or from the pms using plain old malloc. 8192 gives us space for 127 bucket structures per thread. If we need more we can always add a method to get more, but given that it'll only be used for a single thread and be reset between each connection (if I've got that right) then this should be enough, shouldn't it? One would think, but no, not necessarily. It's possible for a filter to split a brigade up into a million little bitty pieces, possibly even 1 byte per bucket (granted, that'd be a pretty stupid filter). [side note: I just saw Ian's post, and he has some good examples of cases where you could end up with lots of buckets at a time. I'm not sure about the subrequests example, because each subrequest would get handled one at a time, its buckets being dumped out to the network and the buckets freed, generally. But it could happen, I guess.] It occurred to me that all you have to do to handle resets is keep a list of the extra blocks you allocate and then loop through them, freeing each one, when the reset happens. You end up with just the one pre-allocated block you started with. Anyway, grist for the mill to discuss on Friday :) Definitely. =-) --Cliff
Re: [PATCH] sms blocks
On Thu, 14 Jun 2001, Luke Kenneth Casson Leighton wrote: you are aware that strictly speaking you are _not_ supposed to treat memory addresses as contiguous? one of the reasons why void* _is_ void* is because potentially, areas of memory allocated by a system may be from different regions, and not contiguous. Uhh.. well, yeah, of course. I was speaking strictly about this particular implementation of apr_sms_blocks, which does its dirty deed by pre-allocating a single big block (8K I think) and then carving it up into little sub-blocks which it hands out. Only if it cannot fulfill the apr_sms_blocks_malloc() request with one of those sub-blocks will it call parent-malloc_fn(). So all you have to do in apr_sms_blocks_free() to find out if the address you were given is one of your sub-blocks or something else (ie, something you got from parent-malloc_fn()) is to check if the address is outside your single big block. If it is outside, then you MUST have gotten that memory block from your parent. If it is inside, then it MUST be one of your sub-blocks. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
file_setaside()
I have a few questions about file_setaside. I'm pasting the function in
here for easy reference.
--
static apr_status_t file_setaside(apr_bucket *data, apr_pool_t *pool)
{
apr_bucket_file *a = data-data;
apr_file_t *fd;
apr_file_t *f = a-fd;
apr_pool_t *p = apr_file_pool_get(f);
#if APR_HAS_MMAP
apr_off_t filelength = data-length; /* bytes remaining in file past
offset
*/
apr_off_t fileoffset = data-start;
#endif
if (apr_pool_is_ancestor(p, pool)) {
return APR_SUCCESS;
}
#if APR_HAS_MMAP
if (file_make_mmap(data, filelength, fileoffset, p)) {
return APR_SUCCESS;
}
#endif
apr_file_dup(fd, f, p);
a-fd = fd;
return APR_SUCCESS;
}
--
(1) Shouldn't the pool passed into file_make_mmap() and apr_file_dup() be
pool and not p? (It'd be easier to see that this is a problem if they
were called reqpool and curpool instead of pool and p
respectively, or something like that.)
(2) Why should file_setaside mmap the file? I'd think that we'd want to
keep it as a file as long as possible to make it easier to use
sendfile()... what am I missing?
(3) You don't really need to dup() the file, do you? You can palloc a new
apr_file_t in the requested pool and use apr_os_file_get() and
apr_os_file_put() to move the os file handle into it. mod_file_cache in
Apache does something like this. It should be cheaper to do this than to
do a full dup(), I think.
Thanks,
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
missing APU_DECLARE_DATA?
Is there a reason that the APU_DECLARE_DATA is missing on this line, or is
it just an oversight? It's there in the header file.
--Cliff
Index: apr_buckets_simple.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_simple.c,v
retrieving revision 1.28
diff -u -d -r1.28 apr_buckets_simple.c
--- apr_buckets_simple.c2001/06/13 19:16:15 1.28
+++ apr_buckets_simple.c2001/06/18 21:10:50
@@ -155,7 +155,7 @@
return apr_bucket_transient_make(b, buf, length);
}
-const apr_bucket_type_t apr_bucket_type_immortal = {
+APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_immortal = {
IMMORTAL, 5,
apr_bucket_destroy_noop,
simple_read,
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
[PATCH] bucket type capabilities flags
The attached patch adds a capabilities flag field to the
apr_bucket_type_t. Flags currently included indicate whether the
optional functions (setaside,copy,split) are implemented or not without
having to actually call them to find out, whether the bucket is a
metadata bucket, and whether the bucket has a file descriptor in it for
the use by apr_sendfile().
The main reason I need this right now is that I've implemented and am
about to commit a patch to Apache's mod_file_cache that uses a custom
bucket type (ap_bucket_cachedfile) to denote a file descriptor that's
shared among multiple threads. Such a file descriptor can be sendfile'd,
but cannot be read by other means without causing thread-safety problems.
Apache's core_output_filter currently is hardcoded to assume that a bucket
can be sendfile'd ONLY if it matches APR_BUCKET_IS_FILE(), though the
above is clearly a case where an APR_BUCKET_SUPPORTS_SENDFILE() test would
be much more useful and extensible. The flags field gives us that
ability.
I can easily see future situations arising where similar strategies would
be useful, so adding a flags field now would allow us to easily handle
those situations.
If nobody objects, I'll commit this tomorrow, followed closely by the
patch to mod_file_cache and the core_output_filter in Apache.
--Cliff
PS: I tried to find names for these flags that were both descriptive and
reasonably short, but if anybody has any better ideas, I'm all ears...
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Index: buckets/apr_buckets_eos.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_eos.c,v
retrieving revision 1.25
diff -u -d -r1.25 apr_buckets_eos.c
--- buckets/apr_buckets_eos.c 2001/06/13 19:16:07 1.25
+++ buckets/apr_buckets_eos.c 2001/06/19 01:05:33
@@ -89,6 +89,7 @@
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_eos = {
EOS, 5,
+APR_BUCKET_FLAGSETASIDE | APR_BUCKET_FLAGCOPY | APR_BUCKET_FLAGMETADATA,
apr_bucket_destroy_noop,
eos_read,
apr_bucket_setaside_noop,
Index: buckets/apr_buckets_file.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_file.c,v
retrieving revision 1.45
diff -u -d -r1.45 apr_buckets_file.c
--- buckets/apr_buckets_file.c 2001/06/18 21:57:10 1.45
+++ buckets/apr_buckets_file.c 2001/06/19 01:05:33
@@ -236,7 +236,7 @@
}
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_file = {
-FILE, 5,
+FILE, 5, APR_BUCKET_FLAGALLFNS | APR_BUCKET_FLAGSENDFILE,
file_destroy,
file_read,
file_setaside,
Index: buckets/apr_buckets_flush.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_flush.c,v
retrieving revision 1.17
diff -u -d -r1.17 apr_buckets_flush.c
--- buckets/apr_buckets_flush.c 2001/06/13 19:16:11 1.17
+++ buckets/apr_buckets_flush.c 2001/06/19 01:05:33
@@ -89,6 +89,7 @@
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_flush = {
FLUSH, 5,
+APR_BUCKET_FLAGSETASIDE | APR_BUCKET_FLAGCOPY | APR_BUCKET_FLAGMETADATA,
apr_bucket_destroy_noop,
flush_read,
apr_bucket_setaside_noop,
Index: buckets/apr_buckets_heap.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_heap.c,v
retrieving revision 1.33
diff -u -d -r1.33 apr_buckets_heap.c
--- buckets/apr_buckets_heap.c 2001/06/07 10:13:36 1.33
+++ buckets/apr_buckets_heap.c 2001/06/19 01:05:33
@@ -124,7 +124,7 @@
}
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_heap = {
-HEAP, 5,
+HEAP, 5, APR_BUCKET_FLAGALLFNS,
heap_destroy,
heap_read,
apr_bucket_setaside_noop,
Index: buckets/apr_buckets_mmap.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_mmap.c,v
retrieving revision 1.36
diff -u -d -r1.36 apr_buckets_mmap.c
--- buckets/apr_buckets_mmap.c 2001/06/14 22:56:13 1.36
+++ buckets/apr_buckets_mmap.c 2001/06/19 01:05:33
@@ -140,7 +140,7 @@
}
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_mmap = {
-MMAP, 5,
+MMAP, 5, APR_BUCKET_FLAGALLFNS,
mmap_destroy,
mmap_read,
mmap_setaside,
Index: buckets/apr_buckets_pipe.c
===
RCS file: /home/cvs/apr-util/buckets/apr_buckets_pipe.c,v
retrieving revision 1.35
diff -u -d -r1.35 apr_buckets_pipe.c
--- buckets/apr_buckets_pipe.c 2001/06/13 19:16:13 1.35
+++ buckets/apr_buckets_pipe.c 2001/06/19 01:05:34
@@ -151,7 +151,7 @@
}
APU_DECLARE_DATA const apr_bucket_type_t apr_bucket_type_pipe = {
-PIPE, 5,
+PIPE, 5, APR_BUCKET_FLAGNONE,
apr_bucket_destroy_noop,
pipe_read,
apr_bucket_setaside_notimpl,
Index: buckets
Re: [PATCH] bucket type capabilities flags
On 19 Jun 2001, Jeff Trawick wrote:
+#define APR_BUCKET_FLAGNONE 0x
+#define APR_BUCKET_FLAGSETASIDE 0x0001
+#define APR_BUCKET_FLAGSPLIT0x0002
+#define APR_BUCKET_FLAGCOPY 0x0004
+#define APR_BUCKET_FLAGSENDFILE 0x0010
+#define APR_BUCKET_FLAGMETADATA 0x0100
+#define APR_BUCKET_FLAGALLFNS
\
+(APR_BUCKET_FLAGSETASIDE | APR_BUCKET_FLAGSPLIT |
APR_BUCKET_FLAGCOPY)
how about a '_' after FLAG? as long as they are already we might as
well make them a little more readable :)
Yeah... sadly, I was just trying to make them fit on one line in places
where they're used. ;-) I'll add the extra _.
re APR_BUCKET_FLAGSENDFILE: all that matters is that the bucket
represents an apr_file_t... whether or not somebody is going to use
apr_sendfile() or something else shouldn't be important to the bucket
code, should it? perhaps it should be renamed to APR_BUCKET_FLAGFILE
Hmm... well, not necessarily. Just because it has a file descriptor in it
doesn't mean that it behaves like a file bucket in any OTHER way. All the
flag means is that there exists a file descriptor in the bucket data
structure at a particular offset that you can use sendfile on. Whether it
would be MMAPed upon read or whatever... well, that's a different flag, I
think. No?
a couple of really tiny nits about APR_BUCKET_FLAGALLFNS...
. ALLFNS doesn't mean all functions, it just means all of the cool
things
No, it means all functions. destroy() and read() are required, so if you
have the other three, you have all five of them.
. ALLFNS will surely change in the future, but what is the way to get
the bucket types updated appropriately? I suspect that if we do
away with ALLFNS and instead spell it out in the bucket type
definitions it will be most obvious what to update
Yeah, I guess... the lines just become prohibitively long. But you're
right that it could cause problems if we added to it later. sigh Well,
I'll see what I can do... I don't suppose anyone would agree to a
APR_BUCKET_FLAGTHREEOPTFNS or something like that?
would it be bad to have an accessor function for the file descriptor?
otherwise, the doc for APR_BUCKET_FLAGSENDFILE should mention that if
the bucket has this flag then the apr_file_t * has to be at the magic
offset
It does mention that (well, actually, the APR_BUCKET_SUPPORTS_SENDFILE()
documentation mentions it... I guess a @see is appropriate at least). An
accessor function is not needed; all you have to do is just pretend that
it's a FILE bucket. It goes something like this:
if (APR_BUCKET_SUPPORTS_SENDFILE(e)) {
apr_bucket_file *f = e-data;
apr_sendfile(sock, f-fd, ...);
}
Easy, huh?
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: [PATCH] bucket type capabilities flags
across multiple threads, then that information should be stored in the apr_file_t, and then the read/mmap functions should look at the apr_file_t and realize that they can't operate on that file. Once that work is done, I don't understand the need for the sendfile flag. But that would mean that an attempt to read from one of these regular file buckets with a special file handle in it would fail. That would be very bad. The right way to do it is that the bucket should just know how to handle the situation, which is to give the thread its own private file handle to the file (by reopening it) if and only if the particular request really calls for it. This is very different from the actions of regular file buckets, so a custom bucket type works quite nicely. All it has to do is make sure that it keeps its FD at the same offset as the regular file bucket does, and watch out for any calls to read(). Sure, this logic could be stuffed into the regular file bucket, but the regular file bucket logic is complicated enough as it is. Special type of data storage, special bucket type... it's a lot cleaner that way. Besides, who's to say that somebody else won't come along later and have yet ANOTHER bucket type that has a file descriptor in it that they want to sendfile()... Can you please post some other situations where these flags are useful. In general, I like the idea of having more information in the bucket structure, but I want to be sure that when we add this information, we do so with very good reasons. Well, like I said, the main reason I need this is for the sendfile thing. I'm sure that there are other cases that will arise later on where somebody (possibly a 3rd party module developer) will want to use a custom bucket type and will want to have his bucket type get handled by the core in some special way (eg this sendfile situation). In any of those cases, it's better to have a flag that says this bucket supports this special function than to have a hardcoded test for one of the canonical bucket types which would require hacking the core to get around. If I can't convince you of the need for flags that indicate which functions are implemented (which I hope I already have), then hopefully I can at least convince you of this. If nobody objects, I'll commit this tomorrow, followed closely by the patch to mod_file_cache and the core_output_filter in Apache. Please give more time than one day when posting such large changes. This really deserves two or three days so that people who aren't reading e-mail everyday can try to keep up. No problem. I'll wait until at least tomorrow, maybe Thursday. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: APR file_io/win32/readwrite.c
On Tue, 19 Jun 2001, William A. Rowe, Jr. wrote: I guess MSVC doesn't warn about such things?? Beats the hell out of me. Obviously we'd have seen it in a gcc warning if it were on the Unix side... It's a level 4 (think -wall) warning on my msvc5. I try building at least once a week against that standard, but as you might expect, there are a ton of trivial errors emitted, so it becomes somewhat illegible. Yeah... I figured as much. Thanks again for catching this quickly, Cliff! Actually, I just stumbled across it while looking at the XTHREAD stuff. And as it turns out, unfortunately, there was nothing quick about it... that line's been broken for 11.5 months (since the pre-a5 era). :-/ Oh well, it's fixed now. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: GCC 2.96 optimization bug
On Sun, 24 Jun 2001, Dale Ghent wrote: When Apache was compile sans -O2, everything worked well and there was no segfault. What about -O? You're right... because of a 2.96 snapshot was used in RH, the use of 2.96 (I'm assuming any snapshot. I see that your's was older than mine) should probably tell autoconf not to include any optimization. If you're gonna do that (which is probably unavoidable), there'd better be a BIG warning message issued, obviously... but other than that, I have no problem with this. http://gcc.gnu.org/gcc-2.96.html --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: file_setaside()
On Tue, 26 Jun 2001, Greg Stein wrote: The setaside() could be called on *really* large files. Calling mmap could be a very bad thing. Just dup the FILE bucket and leave it at that. The decision to do the mmap can/should come when it becomes necessary. Okay, I'll change file_setaside() so that it doesn't MMAP the file and mmap_setaside() so that it doesn't copy into a pool. That makes me feel better. Isn't it just a matter of killing the cleanup associated with one pool and registering the cleanup with the new pool? I'm with Ryan: dup the bugger. It'd be nice if there was an apr_file_t_dup() [if you catch my drift... I don't know what you'd really call it] that does everything apr_file_dup() does *except* calling dup(). Then I'd have no problem at all with this approach. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_mmap.c
On Tue, 26 Jun 2001, Cliff Woolley wrote: Your right, this can do that. However, we really can't keep that from happening. In reality, the mmap_setaside function should just map it back to a file opened out of the new pool. I see now that by map it back to a file you meant reopen the file into the new pool and make it a file bucket, rather than the way I interpreted it before which was MMAP a file opened out of the new pool... see my previous message where I became enlightened. =-) Anyway, does this mean that the apr_mmap_t is going to have to store the file path? Or the mmap bucket could do it, I guess. This will work, but it just doesn't feel right to me. It seems like an awful lot of work just to get around the fact that we might delete the mmap out from under ourselves elsewhere in the code... I still feel like there should be some way to tweak the cleanups to be smart about this sort of thing... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: module magic number
On Wed, 27 Jun 2001, William A. Rowe, Jr. wrote: It brings up a deeper issue. I'm thinking we actually need a composite magic number... the apr and apr-util changes we make also impact new-httpd, but there is no clear way to signal those to the httpd list when apr is mucking things up. Do you all suppose we ought to have an APR magic number, that assures the httpd people that the arguments to existing apr functions haven't been flipped about? This way the apr list can assume responsibility for their changes, while the httpd RM doesn't have to review that work? +1. I'd been wondering about that myself as I've been tweaking various API's. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/memory/unix apr_sms.c apr_sms_blocks.c apr_sms_std.c apr_sms_tracking.c
On 29 Jun 2001 [EMAIL PROTECTED] wrote: Log: We add another phase to the sms creation/initialisation that allows us to make calls that can/could use the sms and if we haven't finished initialising it, it'll segafult due to the lack of function pointers. This came up when trying to get pools running as sms :) I'll reiterate my desire to see us use a static const struct for the function pointers in sms (a la the apr_bucket_type_t). Not only would it make it slightly faster to create an sms, it could help avoid these sorts of problems. I know, I know... if I want it done, I should just patch the damn thing. I'll try to get to it this weekend. =-) --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
RE: cvs commit: apr/memory/unix apr_sms.c apr_sms_blocks.c apr_sms_std.c apr_sms_tracking.c
On Sat, 30 Jun 2001, Sander Striker wrote: I'll reiterate my desire to see us use a static const struct for the function pointers in sms (a la the apr_bucket_type_t). Not only would it make it slightly faster to create an sms, it could help avoid these sorts of problems. This is not possible since we want to be able to set defaults in the framework. With a const struct we are not able to do this. Why not? If the sms module doesn't feel like implementing its own edition of one of the functions, it just sets the function pointer in its const struct to point to the default function. Various bucket types do this sort of thing all the time. And, no, it will not prevent these sort of problems. The framework has to be able to do some allocating from the sms. In apr_sms_init() the sms hasn't been initialized yet. Hence the need for a apr_sms_post_init() function. I won't argue against apr_sms_post_init() too loudly. What I was imagining, though, was something along the lines of the apr_sms_foo_create() function passing a pointer to its static type structure into apr_sms_init(). So then apr_sms_init() could set the type pointer in the apr_sms_t and actually call apr_sms_malloc() if it needed, calling back into the module via the type structure. That might be a stretch, I don't know. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr/network_io/win32 sendrecv.c
On 1 Jul 2001 [EMAIL PROTECTED] wrote: stoddard01/07/01 08:13:35 Modified:network_io/win32 sendrecv.c Log: Back out this portion of Bill Rowe's large file support patch. We should not use the event handle in the apr_file_t to do overlapped i/o on a socket. We either need to wait for io completion on the socket or create a thread specific event handle. This fixes the seg fault we were seeing when serving a cached file on Windows. Wooh!!! Thanks, Bill! So are we working on keepalive requests now, too, dare I ask? PS: Is there any way to get ab to compare the results of each request it makes to verify that they're all the same and/or that they match a reference copy of the requested document? I realize this would slow down ab quite a bit and therefore possibly underestimate the speed of the server, but it'd be a useful option for testing things like this... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
warnings with APR_USE_PROC_PTHREAD_SERIALIZE
Just a heads up on some warnings I'm getting on Solaris 2.6 where
APR_USE_PROC_PTHREAD_SERIALIZE is selected:
locks.c: In function `apr_os_lock_get':
locks.c:344: warning: assignment makes pointer from integer without a cast
locks.c: In function `apr_os_lock_put':
locks.c:365: warning: assignment makes integer from pointer without a cast
The problem is that (after macro substitution) my apr_os_lock_t looks like
this:
struct apr_os_lock_t {
pthread_mutex_t *crossproc;
pthread_mutex_t *intraproc;
};
And apr_os_lock_get/put assume that crossproc will be an int (as it is
with the other serialize methods).
I'd work on this, but it'd probably be easier for somebody who's spent
more time in the locking code than I have...
--Cliff
Re: warnings with APR_USE_PROC_PTHREAD_SERIALIZE
On Sun, 1 Jul 2001 [EMAIL PROTECTED] wrote: Does this solve the warnings? If so, please apply. Two problems: (1) My system would never get into the elif because I have both SYSVSEM and FCNTL serialize: #define APR_USE_FLOCK_SERIALIZE 0 #define APR_USE_SYSVSEM_SERIALIZE 0 #define APR_USE_FCNTL_SERIALIZE 0 #define APR_USE_PROC_PTHREAD_SERIALIZE 1 #define APR_USE_PTHREAD_SERIALIZE 1 #define APR_HAS_FLOCK_SERIALIZE 0 #define APR_HAS_SYSVSEM_SERIALIZE 1 #define APR_HAS_FCNTL_SERIALIZE 1 #define APR_HAS_PROC_PTHREAD_SERIALIZE 1 #define APR_HAS_RWLOCK_SERIALIZE 0 #define APR_HAS_LOCK_CREATE_NP 1 (2) pthread_interproc is actually used in crossproc.c Thanks, --Cliff
Re: SMS lock on demand WAS: RE: Possible race condition...
On Mon, 2 Jul 2001, Sander Striker wrote:
apr_sms_thread_register(sms, os_thread)
{
[snip]
pms = sms-parent;
while (pms) {
apr_sms_thread_register(os_thread);
pms = pms-parent;
}
[snip]
}
You don't need a while loop here... the recursion ought to take care of it
for you.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: warnings with APR_USE_PROC_PTHREAD_SERIALIZE
On 2 Jul 2001, Jeff Trawick wrote:
struct apr_os_lock_t {
#if APR_HAS_PROC_PTHREAD_SERIALIZE
pthread_mutex_t *pthread_crossproc; /* NULL if not used */
#endif
#if APR_HAS_SYSVSEM_SERIALIZE || APR_HAS_FCNTL_SERIALIZE ||
APR_HAS_FLOCK_SERIALIZE
int int_crossproc; /* -1 if not used */
#endif
#if APR_USE_PTHREAD_SERIALIZE
pthread_mutex_t *pthread_intraproc; /* NULL if not used */
#endif
};
in apr_os_lock_get():
#if APR_HAS_PROC_PTHREAD_SERIALIZE
os-pthread_crossproc = lock-pthread_interproc;
#endif
#if APR_HAS_SYSVSEM_SERIALIZE || APR_HAS_FCNTL_SERIALIZE ||
APR_HAS_FLOCK_SERIALIZE
os-int_crossproc = lock-interproc;
#endif
#if APR_USE_PTHREAD_SERIALIZE
os-pthread_intraproc = lock-intraproc;
#endif
That's essentially what Ryan did last night...
In create_lock(), initialize as follows before filling in any lock handles:
#if APR_HAS_PROC_PTHREAD_SERIALIZE
new-pthread_interproc = NULL;
#endif
#if APR_HAS_SYSVSEM_SERIALIZE || APR_HAS_FCNTL_SERIALIZE ||
APR_HAS_FLOCK_SERIALIZE
new-interproc = -1;
#endif
#if APR_USE_PTHREAD_SERIALIZE
new-intraproc = NULL;
#endif
We didn't catch this part, though. The apr_lock_t is pcalloc'ed just
prior to calling create_lock(). Do we really need to do this? If so,
feel free to add it in...
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: pools code
On Wed, 4 Jul 2001 [EMAIL PROTECTED] wrote: +1 for moving it. To move it without losing history, log on to deadalus, and do the following: +1 for moving it. Definitely keep the history. It's much easier than trying to compare across files. And that's always been the group's consensus after many debates on the issue AFAIK. cd /home/cvs/apr cp lib/apr_pools.c memory/unix/apr_pools.c logout One more important thing... you must remove the tags from memory/unix/apr_pools.c. Otherwise checkouts of old tags will contain memory/unix/apr_pools.c in addition to lib/apr_pools.c which is wrong. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_file.c
On 5 Jul 2001 [EMAIL PROTECTED] wrote:
We need to ALWAYS do the seek if we are reading from the file. This is
unfortunate from a performance perspective, but right now, I can have an
offset of 0 in the bucket, but be referring to a file that has been read
from. If we don't seek before reading from the bucket, we get invalid
data.
/* Handle offset ... */
-if (fileoffset) {
rv = apr_file_seek(f, APR_SET, fileoffset);
if (rv != APR_SUCCESS) {
free(buf);
return rv;
-}
}
H... good catch. I don't know why I never thought of that before.
sigh You're right that it sucks for performance, but then again if we
get to this point we've given up on mmaping the file so we're in the
worst case performance scenario anyway.
--Cliff
--
Cliff Woolley
[EMAIL PROTECTED]
Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_file.c
On Thu, 5 Jul 2001, Bill Stoddard wrote: I am teetering on a -1 for this patch. You are hacking around a more fundamental problem. If we cannot fix problems like this w/o impacting the performance of all applications that need to read files, then APR is seriously broken. Well, that's probably true. But please don't -1 the patch. If you're going to -1 something, -1 APR's lack of an apr_file_read()-like function that takes an offset as a parameter. Until APR has such a beast (which will just have to do the lock and seek on its own), this is the correct patch to the buckets, which are broken without it. PS: The common case for this segment of code is that the offset will be non-zero, since it reads in from a file in 8KB hunks, incrementing the offset each time. So removing the conditional actually (very slightly) improves the performance of this section for the common case. Only the case where offset == 0 might be negatively impacted. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_file.c
On Thu, 5 Jul 2001, Bill Stoddard wrote: If you perform a read on a file and don't specifiy an offset, then you should assume you will be reading from the current file pointer maintained by the system (or by apr_file_t in the XTHREAD case). If you have an apr_file_t open and you are reading from the file someplace else using the fd, then you are screwed. You shouldn't be mixing apr_file_* calls with non apr_file_* calls on the same fd. If you insist on doing this, then you need to ensure that your non apr_file_* calls leaves the file pointer in the proper state when they are done. You definitely shouldn't be horking up apr_file* calls to defend against this case. [scratching head] I don't think we're talking about mixing apr_file_* and non apr_file_* calls. A file bucket *always* specifies an offset [although sometimes that offset might be 0]. Consider this: apr_file_t *f = ... apr_bucket *a, *b; a = apr_bucket_file_create(f, 0, len, pool); b = apr_bucket_split(a, 500); APR_BUCKET_INSERT_BEFORE(a, b); Now you have two file buckets referencing the same apr_file_t, one at offset 0 and one at offset 500. The one at offset 500 is BEFORE the one at offset 0 in the brigade. When you read from the buckets in the brigade (assume for a minute that !APR_HAS_MMAP), you get to the offset==500 bucket first, seek to offset 500, and read from there. Then you get to the offset==0 bucket, so you sure as hell better seek back to offset 0 before you read! Not doing so is a bug in the file buckets code, not in APR. If you want to combine the apr_file_seek/apr_file_read calls, that's fine, but you'll STILL need to have removed this offset-test conditional. So Ryan's patch is a correct fix either way, not just a hack. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_file.c
On Thu, 5 Jul 2001, Bill Stoddard wrote: Maybe there is something very fundamental that I a missing here. Each 8K chunk that is read causes the system to increment its file pointer for that open fd. You should not need to call seek() to do something the system is already doing for you under the covers. apr_file_t *f = ...; apr_bucket *a, *b, *c, *d; /* split the bucket into hunks of 100 bytes each */ a = apr_bucket_file_create(f, 0, len, pool); b = apr_bucket_split(a, 100); c = apr_bucket_split(b, 100); d = apr_bucket_split(c, 100); APR_BUCKET_INSERT_AFTER(a, c); apr_bucket_destroy(b); apr_bucket_destroy(d); You can't guarantee that consecutive reads from file buckets read from the next spot in the file. In fact, they very frequently jump around. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_file.c
On Thu, 5 Jul 2001, Bill Stoddard wrote: You can't guarantee that consecutive reads from file buckets read from the next spot in the file. In fact, they very frequently jump around. Give me a common example of reads from file buckets jumping around. The chunk filter. Or any filter for that matter. Any time apr_bucket_split() or apr_bucket_copy() [or even apr_bucket_delete()] are used. I've been trying to think of *any* possible way that the buckets code could know whether the file pointer is at the right spot or not. Possibly some kind of flag in the apr_bucket_file that indicates whether any of the above operations have been called. But no matter what approach I come up with, I always come up with cases where it just won't work. If you're really desperate to optimize the case where we read straight through the file [and can't use sendfile and can't use mmap], I think the best way to do it is to put another conditional in apr_file_seek that checks to see if the file handle is already at the right place and if so returns immediately, skipping the call to lseek() or whatever. But lseek() is probably doing the same thing anyhow... --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
Re: cvs commit: apr-util/buckets apr_buckets_file.c
On Thu, 5 Jul 2001, Cliff Woolley wrote: The chunk filter. s/chunk/byterange/ Duh. --Cliff -- Cliff Woolley [EMAIL PROTECTED] Charlottesville, VA
