You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by rb...@covalent.net on 2000/05/26 22:52:50 UTC
Re: cvs commit: apache-2.0/src/modules/standard mod_access.c
mod_alias.c mod_asis.c mod_auth.c mod_auth_anon.c mod_auth_digest.c
mod_autoindex.c mod_cern_meta.c mod_cgi.c mod_cgid.c mod_dir.c mod_echo.c
mod_env.c mod_expires.c mod_file_cache.c mod_headers.c mod_imap.c mod_include.c
mod_info.c mod_log_config.c mod_mime.c mod_negotiation.c mod_rewrite.c
mod_setenvif.c mod_so.c mod_speling.c mod_status.c mod_unique_id.c
mod_usertrack.c mod_vhost_alias.c
I have a bunch of issues with this patch.
On 27 May 2000 wrowe@locus.apache.org wrote:
> This patch corrects the issues from the AP_EXPORT and linkage
> specification arguments to the ap_hooks.h declarations. As with
> the APR_ and AP_ patches, API_VAR_EXPORT becomes API_EXPORT_VAR,
> and MODULE_VAR_EXPORT becomes MODULE_EXPORT_VAR.
Why? This is a name change, and a gratuitous one at that. The name
change made sense in APR, because APR is a separate project. Here, it
just becomes a big commit and breaks a lot of knowledge people used to
have.
> I will be happy to revert the inclusion of ap_config.h from
> httpd.h if this bothers anyone. More individual modules need
> to be patched if we do so.
There was a reason that this was changed originally. I don't remember
what it was, but I suggest looking back at the archives.
> This patch also moves the following data from http_main to http_config:
>
> const char *ap_server_argv0;
> const char *ap_server_root;
> ap_array_header_t *ap_server_pre_read_config;
> ap_array_header_t *ap_server_post_read_config;
> ap_array_header_t *ap_server_config_defines;
Why?
> /* Hooks */
> -AP_DECLARE_HOOK(int,header_parser,(request_rec *))
> -AP_DECLARE_HOOK(void,post_config,
> +AP_DECLARE_HOOK(API_EXPORT,int,header_parser,(request_rec *))
> +AP_DECLARE_HOOK(API_EXPORT,void,post_config,
> (ap_pool_t *pconf,ap_pool_t *plog,ap_pool_t *ptemp,server_rec *s))
> -AP_DECLARE_HOOK(void,open_logs,
> +AP_DECLARE_HOOK(API_EXPORT,void,open_logs,
> (ap_pool_t *pconf,ap_pool_t *plog,ap_pool_t *ptemp,server_rec *s))
> -AP_DECLARE_HOOK(void,child_init,(ap_pool_t *pchild, server_rec *s))
> +AP_DECLARE_HOOK(API_EXPORT,void,child_init,(ap_pool_t *pchild, server_rec *s))
Every single hook is being declared API_EXPORT. This seems silly to
me. Can't we just use API_EXPORT in the macro definition?
> -#ifndef MODULE_VAR_EXPORT
> -#define MODULE_VAR_EXPORT
> +#ifndef MODULE_EXPORT_VAR
> +#define MODULE_EXPORT_VAR
> #endif
> -#ifndef API_VAR_EXPORT
> -#define API_VAR_EXPORT
> +#ifndef API_EXPORT_VAR
> +#define API_EXPORT_VAR
> #endif
These are name changes for the sake of name changes, blech.
I am basically -1 for this whole commit unless somebody can explain why
the name change was necessary. I would really like to see this whole
thing removed, and then possibly re-committed in small pieces that are
much easier to review.
Ryan
RE: cvs commit: apache-2.0/src/modules/standard mod_access.cmod_alias.c
mod_asis.c mod_auth.c mod_auth_anon.c mod_auth_digest.cmod_autoindex.c
mod_cern_meta.c mod_cgi.c mod_cgid.c mod_dir.c mod_echo.cmod_env.c mod_expires.c
mod_file_cache.c mod_headers.c
Posted by Greg Stein <gs...@lyra.org>.
I'm going to preface this with a big simplifying factor. One that should
have been done INSTEAD of all this AP_EXPORT vs API_EXPORT linkage crap.
CHANGE THE .DSP!!!
We've already said that ap/ "should not exist and should be part of
main/". Great. So make it that way!
On Sat, 27 May 2000, William A. Rowe, Jr. wrote:
> > On Fri, 26 May 2000 rbb@covalent.net wrote:
> > >...
> > > On 27 May 2000 wrowe@locus.apache.org wrote:
> > >...
> > > > /* Hooks */
> > > > -AP_DECLARE_HOOK(int,header_parser,(request_rec *))
> > > > -AP_DECLARE_HOOK(void,post_config,
> > > > +AP_DECLARE_HOOK(API_EXPORT,int,header_parser,(request_rec *))
> > > > +AP_DECLARE_HOOK(API_EXPORT,void,post_config,
> > > > (ap_pool_t *pconf,ap_pool_t
> > *plog,ap_pool_t *ptemp,server_rec *s))
> > > > -AP_DECLARE_HOOK(void,open_logs,
> > > > +AP_DECLARE_HOOK(API_EXPORT,void,open_logs,
> > > > (ap_pool_t *pconf,ap_pool_t
> > *plog,ap_pool_t *ptemp,server_rec *s))
> > > > -AP_DECLARE_HOOK(void,child_init,(ap_pool_t *pchild,
> > server_rec *s))
> > > > +AP_DECLARE_HOOK(API_EXPORT,void,child_init,(ap_pool_t
> > *pchild, server_rec *s))
> > >
> > > Every single hook is being declared API_EXPORT. This seems silly to
> > > me. Can't we just use API_EXPORT in the macro definition?
> >
> > That's what I said, too. Consider me a "-1" on this part of
> > the commit.
> > Until we have a *need* for a different linkage, this
> > parameter should not
> > be included. *When* that need arises, then we can add a second set of
> > macros (but leave these alone!).
>
> No... AP is a library that is not dependent on the apache core server.
YOU made the AP library. You don't have to go and screw with the rest of
the server to resolve that problem.
> API_EXPORT_bleh are defined in the core server, but the library exists
> before the core. I've had more problems than I care for trying to take
> these eggs away from the chicken. Since *today* it's a library function,
> it shouldn'd be defined in terms of the server.
"today" is because of the .dsp. Not because of anything else.
> If ap_hooks.c is moved to main and is compiled into the core, then I
> don't care, it becomes an argument of 'when' there is a need, and since
> I have enough nays, I'll reverse those args out.
Those extra args should be reversed out. There are a couple "-1" votes
against that change already.
You can move the ap_hooks.c simply be updating the .dsp. Or you could add
ap_hooks.c into src/main/ and delete it from src/ap/.
> > > > -#ifndef MODULE_VAR_EXPORT
> > > > -#define MODULE_VAR_EXPORT
> > > > +#ifndef MODULE_EXPORT_VAR
> > > > +#define MODULE_EXPORT_VAR
> > > > #endif
> > > > -#ifndef API_VAR_EXPORT
> > > > -#define API_VAR_EXPORT
> > > > +#ifndef API_EXPORT_VAR
> > > > +#define API_EXPORT_VAR
> > > > #endif
> > >
> > > These are name changes for the sake of name changes, blech.
> >
> > Agreed.
>
> As for EXPORT_VAR vs. VAR_EXPORT, it is in the schema of module_EXPORT,
> and happens to be data instead of a function. Easier to GREP. But if
> I get the nays I'll flip *_EXPORT_VAR back to *_VAR_EXPORT under all
> of APR, AP, and API. I don't care enough to argue this point.
There have been a couple "nays" already, stating that this was a
gratuitous change (done without warning).
I can see why you did it, but if the change is going to occur, then it
should have at least introduced some namespace protection.
> > If we're going to change the names, then it really should be
> > to something
> > like:
> >
> > AP_MODULE_EXPORT_VAR
> > AP_EXPART_VAR
> >
> > i.e. namespace protection
>
> API_ was 'because it's there', and was the path of least resistance
> (fewest changes). APR_ because that was intuitive. AP_ because
> that's intuitive as well, in that it is the ap library. What prefix
> would you propose for:
>
> The APR library package ________
APR_
> The AP library package ________
Never heard of that library :-)
> The server core binary ________
AP_
>...
> > > I am basically -1 for this whole commit unless somebody can
> > > explain why the name change was necessary.
> >
> > I certainly can't explain...
>
> Which name change? VAR_EXPORT <-> EXPORT_VAR? AP_ vs API_ vs APR_ ?
The VAR_EXPORT to EXPORT_VAR thing.
You explained why, but I think it should have been done with an AP_
prefix.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
RE: cvs commit: apache-2.0/src/modules/standard mod_access.cmod_alias.c mod_asis.c mod_auth.c mod_auth_anon.c mod_auth_digest.cmod_autoindex.c mod_cern_meta.c mod_cgi.c mod_cgid.c mod_dir.c mod_echo.cmod_env.c mod_expires.c mod_file_cache.c mod_headers.c
Posted by "William A. Rowe, Jr." <wr...@lnd.com>.
> On Fri, 26 May 2000 rbb@covalent.net wrote:
> >...
> > On 27 May 2000 wrowe@locus.apache.org wrote:
> >...
> > > /* Hooks */
> > > -AP_DECLARE_HOOK(int,header_parser,(request_rec *))
> > > -AP_DECLARE_HOOK(void,post_config,
> > > +AP_DECLARE_HOOK(API_EXPORT,int,header_parser,(request_rec *))
> > > +AP_DECLARE_HOOK(API_EXPORT,void,post_config,
> > > (ap_pool_t *pconf,ap_pool_t
> *plog,ap_pool_t *ptemp,server_rec *s))
> > > -AP_DECLARE_HOOK(void,open_logs,
> > > +AP_DECLARE_HOOK(API_EXPORT,void,open_logs,
> > > (ap_pool_t *pconf,ap_pool_t
> *plog,ap_pool_t *ptemp,server_rec *s))
> > > -AP_DECLARE_HOOK(void,child_init,(ap_pool_t *pchild,
> server_rec *s))
> > > +AP_DECLARE_HOOK(API_EXPORT,void,child_init,(ap_pool_t
> *pchild, server_rec *s))
> >
> > Every single hook is being declared API_EXPORT. This seems silly to
> > me. Can't we just use API_EXPORT in the macro definition?
>
> That's what I said, too. Consider me a "-1" on this part of
> the commit.
> Until we have a *need* for a different linkage, this
> parameter should not
> be included. *When* that need arises, then we can add a second set of
> macros (but leave these alone!).
No... AP is a library that is not dependent on the apache core server.
API_EXPORT_bleh are defined in the core server, but the library exists
before the core. I've had more problems than I care for trying to take
these eggs away from the chicken. Since *today* it's a library function,
it shouldn'd be defined in terms of the server.
If ap_hooks.c is moved to main and is compiled into the core, then I
don't care, it becomes an argument of 'when' there is a need, and since
I have enough nays, I'll reverse those args out.
> > > -#ifndef MODULE_VAR_EXPORT
> > > -#define MODULE_VAR_EXPORT
> > > +#ifndef MODULE_EXPORT_VAR
> > > +#define MODULE_EXPORT_VAR
> > > #endif
> > > -#ifndef API_VAR_EXPORT
> > > -#define API_VAR_EXPORT
> > > +#ifndef API_EXPORT_VAR
> > > +#define API_EXPORT_VAR
> > > #endif
> >
> > These are name changes for the sake of name changes, blech.
>
> Agreed.
As for EXPORT_VAR vs. VAR_EXPORT, it is in the schema of module_EXPORT,
and happens to be data instead of a function. Easier to GREP. But if
I get the nays I'll flip *_EXPORT_VAR back to *_VAR_EXPORT under all
of APR, AP, and API. I don't care enough to argue this point.
> If we're going to change the names, then it really should be
> to something
> like:
>
> AP_MODULE_EXPORT_VAR
> AP_EXPART_VAR
>
> i.e. namespace protection
API_ was 'because it's there', and was the path of least resistance
(fewest changes). APR_ because that was intuitive. AP_ because
that's intuitive as well, in that it is the ap library. What prefix
would you propose for:
The APR library package ________
The AP library package ________
The server core binary ________
I think we have been through the names a dozen some times.
As my comment in ap_config, I'm not at all happy with MODULE_squat,
and CORE_bleh is a noop as well (documentation convention?) so I
will be looking at something better. But not at this instant.
Now that the packages are isolated, Win32 won't suffer import/export
resolution faults, the compiler can resolve calls as jump indirects
vs. linker fixups, and no data GP faults when I bind http_main as a
seperate binary (the Win32 Apache.exe).
> > I am basically -1 for this whole commit unless somebody can
> > explain why the name change was necessary.
>
> I certainly can't explain...
Which name change? VAR_EXPORT <-> EXPORT_VAR? AP_ vs API_ vs APR_ ?
Re: cvs commit: apache-2.0/src/modules/standard mod_access.c
mod_alias.c mod_asis.c mod_auth.c mod_auth_anon.c mod_auth_digest.c
mod_autoindex.c mod_cern_meta.c mod_cgi.c mod_cgid.c mod_dir.c mod_echo.c
mod_env.c mod_expires.c mod_file_cache.c mod_headers.c mod_imap.c mod_include.c
mod_info.c mod_log_config.c mod_mime.c mod_negotiation.c mod_rewrite.c
mod_setenvif.c mod_so.c mod_speling.c mod_status.c mod_unique_id.c
mod_usertrack.c mod_vhost_alias.c
Posted by Greg Stein <gs...@lyra.org>.
On Fri, 26 May 2000 rbb@covalent.net wrote:
>...
> On 27 May 2000 wrowe@locus.apache.org wrote:
>...
> > /* Hooks */
> > -AP_DECLARE_HOOK(int,header_parser,(request_rec *))
> > -AP_DECLARE_HOOK(void,post_config,
> > +AP_DECLARE_HOOK(API_EXPORT,int,header_parser,(request_rec *))
> > +AP_DECLARE_HOOK(API_EXPORT,void,post_config,
> > (ap_pool_t *pconf,ap_pool_t *plog,ap_pool_t *ptemp,server_rec *s))
> > -AP_DECLARE_HOOK(void,open_logs,
> > +AP_DECLARE_HOOK(API_EXPORT,void,open_logs,
> > (ap_pool_t *pconf,ap_pool_t *plog,ap_pool_t *ptemp,server_rec *s))
> > -AP_DECLARE_HOOK(void,child_init,(ap_pool_t *pchild, server_rec *s))
> > +AP_DECLARE_HOOK(API_EXPORT,void,child_init,(ap_pool_t *pchild, server_rec *s))
>
> Every single hook is being declared API_EXPORT. This seems silly to
> me. Can't we just use API_EXPORT in the macro definition?
That's what I said, too. Consider me a "-1" on this part of the commit.
Until we have a *need* for a different linkage, this parameter should not
be included. *When* that need arises, then we can add a second set of
macros (but leave these alone!).
> > -#ifndef MODULE_VAR_EXPORT
> > -#define MODULE_VAR_EXPORT
> > +#ifndef MODULE_EXPORT_VAR
> > +#define MODULE_EXPORT_VAR
> > #endif
> > -#ifndef API_VAR_EXPORT
> > -#define API_VAR_EXPORT
> > +#ifndef API_EXPORT_VAR
> > +#define API_EXPORT_VAR
> > #endif
>
> These are name changes for the sake of name changes, blech.
Agreed.
If we're going to change the names, then it really should be to something
like:
AP_MODULE_EXPORT_VAR
AP_EXPART_VAR
i.e. namespace protection
> I am basically -1 for this whole commit unless somebody can explain why
> the name change was necessary.
I certainly can't explain...
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
moving globals (was: RE: cvs commit: ...)
Posted by Greg Stein <gs...@lyra.org>.
On Sat, 27 May 2000, William A. Rowe, Jr. wrote:
>...
> > > This patch also moves the following data from http_main
> > to http_config:
> > >
> > > const char *ap_server_argv0;
> > > const char *ap_server_root;
> > > ap_array_header_t *ap_server_pre_read_config;
> > > ap_array_header_t *ap_server_post_read_config;
> > > ap_array_header_t *ap_server_config_defines;
> >
> > Why?
>
> Unless someone has STRONG feelings, I am looking to isolate the code
> module http_main.c to allow it to link into a seperate binary. These
> args are the only thing in my way. In this, the Win32 implementation
> becomes Apache.exe (of http_main.c alone) dynamically linked to all
> the remaining core server code.
This is premised on http_main.c being able to link into a separate binary.
I think that you might run into problems over time because that is not
apparent in the source organization. People may end up introducing new
things to http_main.c that break this.
I don't have a particular solution to that, but it is a concern.
In any case, the move of the variables seems okay. Globals are bunk in the
first place, so whereever they go, they're still bogus :-)
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
RE: cvs commit: apache-2.0/src/modules/standard mod_access.cmod_alias.c mod_asis.c mod_auth.c mod_auth_anon.c mod_auth_digest.cmod_autoindex.c mod_cern_meta.c mod_cgi.c mod_cgid.c mod_dir.c mod_echo.cmod_env.c mod_expires.c mod_file_cache.c mod_headers.c
Posted by "William A. Rowe, Jr." <wr...@lnd.com>.
> From: rbb@covalent.net [mailto:rbb@covalent.net]
> Sent: Friday, May 26, 2000 3:53 PM
>
> I have a bunch of issues with this patch.
>
> On 27 May 2000 wrowe@locus.apache.org wrote:
>
> > This patch corrects the issues from the AP_EXPORT and linkage
> > specification arguments to the ap_hooks.h declarations. As with
> > the APR_ and AP_ patches, API_VAR_EXPORT becomes API_EXPORT_VAR,
> > and MODULE_VAR_EXPORT becomes MODULE_EXPORT_VAR.
>
> Why? This is a name change, and a gratuitous one at that. The name
> change made sense in APR, because APR is a separate project. Here, it
> just becomes a big commit and breaks a lot of knowledge people used to
> have.
AP_EXPORT? No, it's a seperate library, ergo seperate linkage spec.
VAR_EXPORT <-> EXPORT_VAR? Either way you want to have it, I'm hearing
three negatives here, so I'm reversing out *this aspect*, within the next
1/2 hour.
>
> > I will be happy to revert the inclusion of ap_config.h from
> > httpd.h if this bothers anyone. More individual modules need
> > to be patched if we do so.
>
> There was a reason that this was changed originally. I don't remember
> what it was, but I suggest looking back at the archives.
I'll search my CD for the comments. Any idea on the timetable of that
discussion (the year would be a good start).
> > This patch also moves the following data from http_main
> to http_config:
> >
> > const char *ap_server_argv0;
> > const char *ap_server_root;
> > ap_array_header_t *ap_server_pre_read_config;
> > ap_array_header_t *ap_server_post_read_config;
> > ap_array_header_t *ap_server_config_defines;
>
> Why?
Unless someone has STRONG feelings, I am looking to isolate the code
module http_main.c to allow it to link into a seperate binary. These
args are the only thing in my way. In this, the Win32 implementation
becomes Apache.exe (of http_main.c alone) dynamically linked to all
the remaining core server code.
See the Greg comments on the remaining name changes.