httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Daniel Ruggeri <DRugg...@primary.net>
Subject Re: Question and request for comments on patch
Date Sun, 24 Jul 2011 23:35:31 GMT
On 7/24/2011 2:12 AM, Stefan Fritsch wrote:
> On Friday 22 July 2011, Daniel Ruggeri wrote:
>> Attached is the final cut of the patch including doco and MMN bump
>> as you brought up. I plan to commit this on Monday, time
>> permitting (and of course in the absence of objections). I'll
>> cobble something together afterwards for a 2.2 backport.
> +1 to the principle. This also allows to use LogLevel in .htaccess, 
> which I wanted to implement but never got around to.
>
> Some thoughts / nitpicks:
> style: Include a space between an if and the opening brace.
Done - looks like I only did this in a few spots some how.
> We need to support C90:
> core.c: In function 'set_override_list':
> core.c:1626:5: error: ISO C90 forbids mixed declarations and code [-
> Werror=declaration-after-statement]
Done also
>
> When parsing the list, look in ap_config_hash if a directive exists. 
> If not, either log a warning or error out (not sure which is better).
I may be missing something, but isn't ap_config_hash private to config.c
and unavailable to core.c where the configuration directive is
implemented? If I'm off, I'd definitely like to add this as well as
remove the need for the 'tolower' calls by looking up the expected
CamelCase of a directive as it shows up in config.c.
> I think the word1,word2,... syntax is unwieldly, especially if the 
> list gets long. Maybe use AP_INIT_TAKE_ARGV instead (see e.g. 
> AllowMethods).
This is far from a nitpick and makes a lot more sense. I have
implemented this as well.
> It should be possible to reset AllowOverrideList to an empty list in a 
> subdir, even if it is set in the parent dir. This is important in the 
> case that there is a permissive AllowOverrideList in main server scope 
> and an empty one for some virtual hosts.  In the case of an empty 
> list, d->override_list should be set to NULL instead of an empty table 
> for better performance.  Maybe one could allow "disabled" or "reset" 
> as alias for an empty list (like in UserDir/DirectoryIndex).
I realized I did not add this about five minutes after sending the patch
along to the list. I have added this also. Your followup email about
making the list NULL complicating things is also true - I had a lot of
trouble merging properly when NULL was considered "nothing in the list"
as opposed to "not configured".
> It may be possible that some modules react badly if a directive is 
> used in .htaccess that has previously not been allowed there. For 
> example if the module needs to do some global initialization when a 
> directive is used for the first time. I am not aware of such a case, 
> but it is something we may want to keep in mind if this is backported. 
> And it is definitely something that should go into the new_api_2.4 
> document.
Yes, very good point - I did not realize this would become a side
effect. I thought there were upstream context checks before a command is
invoked to prevent this.
I have taken the second suggestion and added a note in new_api_2.4.xml also.

Thank you for the review. Much appreciated. Attached is the patch
including the suggested changes except the directive look up.
-- 
--
Daniel Ruggeri

Mime
View raw message