httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Paul J. Reder" <rede...@raleigh.ibm.com>
Subject Re: [Patch]: mod_include.c port to 2.0 buckets (includes newmod_include.h)
Date Tue, 21 Nov 2000 15:28:01 GMT
Ryan,

Thank you for your comments. I am sorry for the long point by point response.
I am updating my patch to address all of your comments. Please read my
response for explainations.

Thank you.

rbb@covalent.net wrote:
> ...USE_SFIO...VOIDUSED...USE_STDIO?

These were historical. I didn't notice them there.

> 
> find_start_sequence and find_end_sequence are basically the same
> functions.  The biggest differences are the values that are set in the ctx
> structure, this could and should be handled with flags.  This code is too
> complex to duplicate.

I disagree. Yes the code is similar, but there are enough differences that the
code becomes ugly to the point of being unreadable if it is merged using
flags etc. I know because I started with a single merged function.

> 
> >        if ((ctx->combined_tag = malloc (ctx->tag_length + 1)) == NULL) {
> >            return (APR_ENOMEM);
> 
> Why are we malloc'ing here?  This data (if I understand the comments) is
> never sent to the client.  This should be a palloc, or even better a
> pcalloc.

In one of my design notes I commented that I wasn't sure of the best place to
(m|c|p)alloc from for this. My thinking was that I didn't want to grow the
pool size by pallocing from there for something that was strictly internal
to mod_include. My thinking was that by mallocing here I could free it when
I am done. I also don't think this is going to happen very frequently.
> 
> get_combined_directive deals with two distinct brigades.  Why aren't they
> concatenated?  This would simplify the code, and it isn't an expensive
> operation.

I thought it was expensive to setaside the rest of the brigade just so I
could throw it away. All I am doing is copying the bytes from the buckets
of the directive. As soon as I am done processing I throw all of those
buckets away. Why pay the price to set aside the buckets just to throw them
away. I don't think it adds that much complexity to handle two brigades.

> 
> get_combined_directive also uses AP_BUCKET_PREV(dptr) != bb_end.  From
> reading the code, this could be simplified to
> !AP_BRIGADE_SENTINEL(dptr).  This would again simplify the code and make
> it easier to understand and debug.

You are quite correct. I learned about AP_BRIGADE_SENTINEL after I wrote
this code.

> 
> get_tag_and_value returns the value using return, and returns the tag in
> an argument.  For consistancy it should return both as arguments.

Ok. Easily fixed.

> 
> get_directive should use a hash table to store the tags.  The key is the
> directive, the value is the function to call when that directive is
> encountered.  (This also allows modules to easily extend mod_include's
> capabilities to handle more directives, allowing us to remove
> USE_PERL_SSI, because that becomes part of mod_perl.)

I can see the added flexiblity visa-vie removing the USE_PERL_SSI stuff, but
the problem with this is that the main send_parsed_content function does
different wrapper code for each directive. For some it needs to check 
"printing" for others it needs to deal with "if_nesting_level", and for
others I need to send the preceding buckets before processing the subrequest.
Getting back a generic function pointer doesn't help this. It also doesn't
improve performance for checking such a small number of directives.


> You can't do this with any transient buckets.  The point behind transient
> buckets is that they are stack data, but in order for them to work, you
> have to call the set-aside function on those buckets.  If you don't, the
> data just disappears out from under the bucket.  Since your data is
> disappearing immediately, this should just be put in a heep or pool
> bucket.
> 
> Same problem here.

Ok. Chalk this up to ignorance on my part.

> 
> You have ap_r* functions in the main-line code.  This is not allowed.  #1,
> a handler that uses buckets cannot also use ap_r*.  #2 and more important,
> NO filter can use ap_r* functions, ever.

I assume you are referreing to ap_r(v)puts and not ap_run_sub_req or
ap_regex. I actually knew that those were problems and had it on my list
to clean up. I believe that all of those are in the parse_expr function
which I left to last to work on and then didn't get to. Those are just
leftover from before and I had already figured out how to get rid of them,
I just didn't actually do it.

> What exactly is parse_expr doing?  It is a huge function, and I don't even
> want to try to figure out what it is up to.

It is doing what it did before ;) (I didn't touch it yet). It is the 
expression parser for the "if" and "else" directives. I should be
able to clean it up and simplify it, but it was not broken at the moment
and so didn't need fixing (other than the ap_r* calls).

> 
> LOG_COND_STATUS is broken.  You can't use a transient bucket in this
> way.  Same as above.

Ok. Same as above.

> 
> The handle_* function abstraction is still wrong.  It shouldn't be up to
> send_parsed_content to split the brigade and pass it if
> necessary.  Sometimes, the include directive requires a sub_request,
> sometimes it doesn't (error condition).  Why do we always split and send
> the brigade?  The abstraction is simple, the handle_* function gets the
> whole brigade, and a pointer to the start of the tag.  The handle_*
> function can then decide if it wants to split and pass the brigade or not.

Easy change to move this from send_parsed_content in to the handle functions.

> 
> Think of it this way.  Right now, this code is not easily extensible, to
> make it extensible is easy however.  Instead of hard-coding the directives
> that are understood, take a modular approach.  Use a hash table to store
> the directives that are understood, and just pass the processing off to
> those functions.  This allows another module to register a new SSI
> directive easily.  In order for this to work, the handle_* function needs
> to have complete discretion over how the brigade is handled.  Again, this
> would simplify code, making this module easier to debug.

Ok. I see your hash table argument now. I was leaving the wrapper code outside
of the handle_* functions to avoid unnecessary function calls. You are
advocating moving all of the code into the handle_* functions so that they
are self contained for future flexibility. I can see and agree to this point.
This is also an easy thing to fix. By the way, how do you see other modules
to register handle_* functions with mod_include?

> 
> What is FREDDY_KRUGER_VEGAMATIC_MODE?

This was testing code to pathologically slice the brigade into a bunch of
brigades with a single bucket with a single byte in it. Hence the reference
to Freddy Kruger (sp) of Nightmare on elm street fame.

> 
> CREATE_ERROR_BUCKET is brokecn because of the transient problem.

Ok. Same as above.

> In general, instead of the re-write making mod_include easier to deal
> with, it has become a bigger monster, doubling in size and adding another
> 600 lines of code (not counting the include file).  I don't mind more
> lines of code, I do mind when those lines of code are almost impossible to
> wade through.

A fair number of the added lines of code are comments added to try to make the
change easier to understand. The code has grown by about 35% including the
comments. What do you feel makes them impossible to wade through. This strikes
me as sour grapes simply because you didn't write the code and are therefore
unfamiliar with it.

> I have started to try to debug this module, but I am giving up now.  I am
> unlikely to touch this version of the module again, because it is just too
> much.  I'm going back to the original version.  If somebody else wants to
> debug the new one, feel free.

I didn't ask you to debug the module, I am not asking you to fix the module, and
I sure as hell am not expecting you to rewrite the module. The comments that you
made are all easily fixed in this code. I don't see a single one of them that
warrants this sort of inane response. I will have a fixed patch soon addressing
all of your comments.


-- 
Paul J. Reder
-----------------------------------------------------------
"The strength of the Constitution lies entirely in the determination of each
citizen to defend it.  Only if every single citizen feels duty bound to do
his share in this defense are the constitutional rights secure."
-- Albert Einstein

Mime
View raw message