Re: SSLSrvConfigRec shared
On Sat, Dec 23, 2017 at 3:53 PM, Stefan Eissing wrote: > > I think this really is a bug in our runtime. The flag was invented > just for precaution, in case a module would rely on the no-copy > behaviour. That is really close to saying: we will not fix > any more bugs in 2.4.x as someone could rely on it. I would be fine with duplicating/merging every server config by default, even when no module directive is specified, the risk is that some (third party) merger functions might not do the right thing when called in this situation, and could break. Too much caution possibly...
Re: SSLSrvConfigRec shared
> I think this really is a bug in our runtime. The flag was invented > just for precaution, in case a module would rely on the no-copy > behaviour. That is really close to saying: we will not fix > any more bugs in 2.4.x as someone could rely on it. I don't think this bit is fair. Every config section behaves the same -- modules get a new configuration when their directives are used in the section.
Re: SSLSrvConfigRec shared
> Am 23.12.2017 um 12:34 schrieb Yann Ylavic : > > On Sat, Dec 23, 2017 at 9:00 AM, Nick Kew wrote: >> On Sat, 2017-12-23 at 08:20 +0100, Stefan Eissing wrote: >> Ugh. Fine for trunk, but that's a lot of complexity to foist on a stable branch. A module would not only need to check MMN, but also implement fallback behaviour if there are no flags. So why not KISS and stick with that fallback for all 2.4? >>> >>> Not sure, I parse that. Any module that does nothing continues to >>> see the same behaviour than before. Where does the complexity >>> arrive? >> >> But a module cannot ever *use* it without checking MMN >> *AND* implementing fallback behaviour for being loaded >> into an httpd built with the old struct - and consequently >> old API and ABI. >> >> That's bad enough to work through once, let alone maintain longer-term! >> >> Whereas the fallback, by definition, works in all cases. > > Right, but hence nothing prevents the modules from implementing the > fallback only, just as if "flags" were not in 2.4.x. > It's not like depending on the MMN is mandatory. > > It seems that no module really needed it until mod_md, or they already > implemented the fallback. > If we make it to 2.4.next for mod_md needs (which anyway requires > mod_ssl 2.4.next), at least it simplifies the code in mod_ssl... Any module that writes to its config records in a post_config hook is in danger of having shared instances between vhosts. Only when directives of the module in the vhost are used, do they have a unique config record instance. There is workaround code in mod_md and I was writing that for mod_ssl as Yann came with this flag solution. So, current mod_ssl does not have a workaround and will not work in case someone uses the SSLEngine *:443 and uses no SSL* directives in vhosts. If "SSLEngine on" or other directives are used, that vhost has a unique instance of its mod_ssl config record. Very subtle. I think this really is a bug in our runtime. The flag was invented just for precaution, in case a module would rely on the no-copy behaviour. That is really close to saying: we will not fix any more bugs in 2.4.x as someone could rely on it. >> Agreed, post-config per-server stuff is clumsy: I have a distant >> memory from 2.0 days of hacking some ugly workaround, though the >> details elude me. But wouldn't it make more sense to review that >> in 2.5/trunk rather than the stable branch? > > It shouldn't hurt in 2.4, IMHO. I agree. I also think the bugfix without flag could have gone to 2.4.
Re: SSLSrvConfigRec shared
On Sat, Dec 23, 2017 at 9:00 AM, Nick Kew wrote: > On Sat, 2017-12-23 at 08:20 +0100, Stefan Eissing wrote: > >> > Ugh. Fine for trunk, but that's a lot of complexity to foist on >> > a stable branch. A module would not only need to check MMN, >> > but also implement fallback behaviour if there are no flags. >> > So why not KISS and stick with that fallback for all 2.4? >> >> Not sure, I parse that. Any module that does nothing continues to >> see the same behaviour than before. Where does the complexity >> arrive? > > But a module cannot ever *use* it without checking MMN > *AND* implementing fallback behaviour for being loaded > into an httpd built with the old struct - and consequently > old API and ABI. > > That's bad enough to work through once, let alone maintain longer-term! > > Whereas the fallback, by definition, works in all cases. Right, but hence nothing prevents the modules from implementing the fallback only, just as if "flags" were not in 2.4.x. It's not like depending on the MMN is mandatory. It seems that no module really needed it until mod_md, or they already implemented the fallback. If we make it to 2.4.next for mod_md needs (which anyway requires mod_ssl 2.4.next), at least it simplifies the code in mod_ssl... > > Agreed, post-config per-server stuff is clumsy: I have a distant > memory from 2.0 days of hacking some ugly workaround, though the > details elude me. But wouldn't it make more sense to review that > in 2.5/trunk rather than the stable branch? It shouldn't hurt in 2.4, IMHO. Regards, Yann.
Re: SSLSrvConfigRec shared
On Sat, 2017-12-23 at 08:20 +0100, Stefan Eissing wrote: > > Ugh. Fine for trunk, but that's a lot of complexity to foist on > > a stable branch. A module would not only need to check MMN, > > but also implement fallback behaviour if there are no flags. > > So why not KISS and stick with that fallback for all 2.4? > > Not sure, I parse that. Any module that does nothing continues to see the > same behaviour than before. Where does the complexity arrive? But a module cannot ever *use* it without checking MMN *AND* implementing fallback behaviour for being loaded into an httpd built with the old struct - and consequently old API and ABI. That's bad enough to work through once, let alone maintain longer-term! Whereas the fallback, by definition, works in all cases. Agreed, post-config per-server stuff is clumsy: I have a distant memory from 2.0 days of hacking some ugly workaround, though the details elude me. But wouldn't it make more sense to review that in 2.5/trunk rather than the stable branch? -- Nick Kew
Re: SSLSrvConfigRec shared
> Am 22.12.2017 um 23:31 schrieb Nick Kew : > > On Thu, 21 Sep 2017 08:11:17 -0400 > Eric Covener wrote: > >> IIUC it should be safe to extend module_struct with a minor bump to >> add 'int flags' to the bottom, but when you check the value you'd need >> to check the MMN first. In the module you'd then just have some flags >> or'ed together after register_hooks. > > Sorry to come to this late: arises from reviewing STATUS. > > Ugh. Fine for trunk, but that's a lot of complexity to foist on > a stable branch. A module would not only need to check MMN, > but also implement fallback behaviour if there are no flags. > So why not KISS and stick with that fallback for all 2.4? Not sure, I parse that. Any module that does nothing continues to see the same behaviour than before. Where does the complexity arrive? -Stefan > > -- > Nick Kew
Re: SSLSrvConfigRec shared
On Thu, 21 Sep 2017 08:11:17 -0400 Eric Covener wrote: > IIUC it should be safe to extend module_struct with a minor bump to > add 'int flags' to the bottom, but when you check the value you'd need > to check the MMN first. In the module you'd then just have some flags > or'ed together after register_hooks. Sorry to come to this late: arises from reviewing STATUS. Ugh. Fine for trunk, but that's a lot of complexity to foist on a stable branch. A module would not only need to check MMN, but also implement fallback behaviour if there are no flags. So why not KISS and stick with that fallback for all 2.4? -- Nick Kew
Re: SSLSrvConfigRec shared
I added the 'flags' getter in r1809311, much cleaner, thanks! On Fri, Sep 22, 2017 at 2:48 PM, Eric Covener wrote: > Whoops I see you already folllowed it up. > > On Fri, Sep 22, 2017 at 8:46 AM, Eric Covener wrote: >> On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic wrote: >>> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: > On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: >> >> IIUC it should be safe to extend module_struct with a minor bump to >> add 'int flags' to the bottom, but when you check the value you'd need >> to check the MMN first. In the module you'd then just have some flags >> or'ed together after register_hooks. > > Something like the attached patch might do it (untested, no MMN minor > bump). > >> >> (hopefully someone will check my work) > > Since modules (module_struct) are déclared globally, unspecified > fields at the end of the struct should be initialized to zero, so it > should be safe. I was thinking about modules compiled against the previous definition / out of tree. >>> >>> Hmm, I'm not sure my commits address this. >>> >>> The modules would be run by the latest core without being recompiled >>> against it, that's the case? >> >> Yes, I think it not yet handled because you're checking the cores MMN >> # at compile time. >> >> I think we need an accessor or macro to retrieve the flags that looks >> at the module_struct being evaluated which I think also has their >> compile-time MMN baked in. Probably best to have this be a simple >> function rather than a macro. >> >> >> -- >> Eric Covener >> cove...@gmail.com > > > > -- > Eric Covener > cove...@gmail.com
Re: SSLSrvConfigRec shared
posticipate - realizing, while one writes a reply, that Yann has probably already implemented it. X-) > Am 22.09.2017 um 14:48 schrieb Eric Covener : > > Whoops I see you already folllowed it up. > > On Fri, Sep 22, 2017 at 8:46 AM, Eric Covener wrote: >> On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic wrote: >>> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: > On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: >> >> IIUC it should be safe to extend module_struct with a minor bump to >> add 'int flags' to the bottom, but when you check the value you'd need >> to check the MMN first. In the module you'd then just have some flags >> or'ed together after register_hooks. > > Something like the attached patch might do it (untested, no MMN minor > bump). > >> >> (hopefully someone will check my work) > > Since modules (module_struct) are déclared globally, unspecified > fields at the end of the struct should be initialized to zero, so it > should be safe. I was thinking about modules compiled against the previous definition / out of tree. >>> >>> Hmm, I'm not sure my commits address this. >>> >>> The modules would be run by the latest core without being recompiled >>> against it, that's the case? >> >> Yes, I think it not yet handled because you're checking the cores MMN >> # at compile time. >> >> I think we need an accessor or macro to retrieve the flags that looks >> at the module_struct being evaluated which I think also has their >> compile-time MMN baked in. Probably best to have this be a simple >> function rather than a macro. >> >> >> -- >> Eric Covener >> cove...@gmail.com > > > > -- > Eric Covener > cove...@gmail.com
Re: SSLSrvConfigRec shared
Whoops I see you already folllowed it up. On Fri, Sep 22, 2017 at 8:46 AM, Eric Covener wrote: > On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic wrote: >> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: >>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: > > IIUC it should be safe to extend module_struct with a minor bump to > add 'int flags' to the bottom, but when you check the value you'd need > to check the MMN first. In the module you'd then just have some flags > or'ed together after register_hooks. Something like the attached patch might do it (untested, no MMN minor bump). > > (hopefully someone will check my work) Since modules (module_struct) are déclared globally, unspecified fields at the end of the struct should be initialized to zero, so it should be safe. >>> >>> I was thinking about modules compiled against the previous definition >>> / out of tree. >> >> Hmm, I'm not sure my commits address this. >> >> The modules would be run by the latest core without being recompiled >> against it, that's the case? > > Yes, I think it not yet handled because you're checking the cores MMN > # at compile time. > > I think we need an accessor or macro to retrieve the flags that looks > at the module_struct being evaluated which I think also has their > compile-time MMN baked in. Probably best to have this be a simple > function rather than a macro. > > > -- > Eric Covener > cove...@gmail.com -- Eric Covener cove...@gmail.com
Re: SSLSrvConfigRec shared
On Fri, Sep 22, 2017 at 8:11 AM, Yann Ylavic wrote: > On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: >> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: >>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: IIUC it should be safe to extend module_struct with a minor bump to add 'int flags' to the bottom, but when you check the value you'd need to check the MMN first. In the module you'd then just have some flags or'ed together after register_hooks. >>> >>> Something like the attached patch might do it (untested, no MMN minor bump). >>> (hopefully someone will check my work) >>> >>> Since modules (module_struct) are déclared globally, unspecified >>> fields at the end of the struct should be initialized to zero, so it >>> should be safe. >> >> I was thinking about modules compiled against the previous definition >> / out of tree. > > Hmm, I'm not sure my commits address this. > > The modules would be run by the latest core without being recompiled > against it, that's the case? Yes, I think it not yet handled because you're checking the cores MMN # at compile time. I think we need an accessor or macro to retrieve the flags that looks at the module_struct being evaluated which I think also has their compile-time MMN baked in. Probably best to have this be a simple function rather than a macro. -- Eric Covener cove...@gmail.com
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: > On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: >> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: >>> >>> IIUC it should be safe to extend module_struct with a minor bump to >>> add 'int flags' to the bottom, but when you check the value you'd need >>> to check the MMN first. In the module you'd then just have some flags >>> or'ed together after register_hooks. >> >> Something like the attached patch might do it (untested, no MMN minor bump). >> >>> >>> (hopefully someone will check my work) >> >> Since modules (module_struct) are déclared globally, unspecified >> fields at the end of the struct should be initialized to zero, so it >> should be safe. > > I was thinking about modules compiled against the previous definition > / out of tree. Hmm, I'm not sure my commits address this. The modules would be run by the latest core without being recompiled against it, that's the case?
Re: SSLSrvConfigRec shared
The patches look great! Will test on next occasion! Thanks! :) > Am 22.09.2017 um 14:02 schrieb Yann Ylavic : > > On Thu, Sep 21, 2017 at 2:54 PM, Yann Ylavic wrote: >> On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: >>> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: > > IIUC it should be safe to extend module_struct with a minor bump to > add 'int flags' to the bottom, but when you check the value you'd need > to check the MMN first. In the module you'd then just have some flags > or'ed together after register_hooks. Something like the attached patch might do it (untested, no MMN minor bump). > > (hopefully someone will check my work) Since modules (module_struct) are déclared globally, unspecified fields at the end of the struct should be initialized to zero, so it should be safe. >>> >>> I was thinking about modules compiled against the previous definition >>> / out of tree. >> >> Right, it's missing some #ifdefs MMN too :) > > r1809302 and r1809303, does it work for you Stefan? > > We have to change the AP_MODULE_HAS_FLAGS macro with the appropriate > MMN if we'd backport...
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 2:54 PM, Yann Ylavic wrote: > On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: >> On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: >>> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: IIUC it should be safe to extend module_struct with a minor bump to add 'int flags' to the bottom, but when you check the value you'd need to check the MMN first. In the module you'd then just have some flags or'ed together after register_hooks. >>> >>> Something like the attached patch might do it (untested, no MMN minor bump). >>> (hopefully someone will check my work) >>> >>> Since modules (module_struct) are déclared globally, unspecified >>> fields at the end of the struct should be initialized to zero, so it >>> should be safe. >> >> I was thinking about modules compiled against the previous definition >> / out of tree. > > Right, it's missing some #ifdefs MMN too :) r1809302 and r1809303, does it work for you Stefan? We have to change the AP_MODULE_HAS_FLAGS macro with the appropriate MMN if we'd backport...
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 2:51 PM, Eric Covener wrote: > On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: >> On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: >>> >>> IIUC it should be safe to extend module_struct with a minor bump to >>> add 'int flags' to the bottom, but when you check the value you'd need >>> to check the MMN first. In the module you'd then just have some flags >>> or'ed together after register_hooks. >> >> Something like the attached patch might do it (untested, no MMN minor bump). >> >>> >>> (hopefully someone will check my work) >> >> Since modules (module_struct) are déclared globally, unspecified >> fields at the end of the struct should be initialized to zero, so it >> should be safe. > > I was thinking about modules compiled against the previous definition > / out of tree. Right, it's missing some #ifdefs MMN too :)
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 8:44 AM, Yann Ylavic wrote: > On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: >> >> IIUC it should be safe to extend module_struct with a minor bump to >> add 'int flags' to the bottom, but when you check the value you'd need >> to check the MMN first. In the module you'd then just have some flags >> or'ed together after register_hooks. > > Something like the attached patch might do it (untested, no MMN minor bump). > >> >> (hopefully someone will check my work) > > Since modules (module_struct) are déclared globally, unspecified > fields at the end of the struct should be initialized to zero, so it > should be safe. I was thinking about modules compiled against the previous definition / out of tree. -- Eric Covener cove...@gmail.com
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 2:11 PM, Eric Covener wrote: > > IIUC it should be safe to extend module_struct with a minor bump to > add 'int flags' to the bottom, but when you check the value you'd need > to check the MMN first. In the module you'd then just have some flags > or'ed together after register_hooks. Something like the attached patch might do it (untested, no MMN minor bump). > > (hopefully someone will check my work) Since modules (module_struct) are déclared globally, unspecified fields at the end of the struct should be initialized to zero, so it should be safe. Index: include/http_config.h === --- include/http_config.h (revision 1809129) +++ include/http_config.h (working copy) @@ -329,6 +329,12 @@ struct cmd_parms_struct { }; /** + * Flags associated with a module. + */ +#define AP_MODULE_FLAG_NONE (0) +#define AP_MODULE_FLAG_ALWAYS_MERGE (1 << 0) + +/** * Module structures. Just about everything is dispatched through * these, directly or indirectly (through the command and handler * tables). @@ -407,6 +413,9 @@ struct module_struct { * @param p the pool to use for all allocations */ void (*register_hooks) (apr_pool_t *p); + +/** A bitmask of AP_MODULE_FLAG_* */ +int flags; }; /** Index: server/config.c === --- server/config.c (revision 1809129) +++ server/config.c (working copy) @@ -323,7 +323,7 @@ static ap_conf_vector_t *create_server_config(apr_ } static void merge_server_configs(apr_pool_t *p, ap_conf_vector_t *base, - ap_conf_vector_t *virt) + server_rec *virt) { /* Can reuse the 'virt' vector for the spine of it, since we don't * have to deal with the moral equivalent of .htaccess files here... @@ -330,7 +330,7 @@ static void merge_server_configs(apr_pool_t *p, ap */ void **base_vector = (void **)base; -void **virt_vector = (void **)virt; +void **virt_vector = (void **)virt->module_config; module *modp; for (modp = ap_top_module; modp; modp = modp->next) { @@ -337,10 +337,19 @@ static void merge_server_configs(apr_pool_t *p, ap merger_func df = modp->merge_server_config; int i = modp->module_index; -if (!virt_vector[i]) -virt_vector[i] = base_vector[i]; -else if (df) +if (!virt_vector[i]) { +if (df && modp->create_server_config + && modp->flags & AP_MODULE_FLAG_ALWAYS_MERGE) { +virt_vector[i] = (*modp->create_server_config)(p, virt); +} +else { +virt_vector[i] = base_vector[i]; +df = NULL; +} +} +if (df) { virt_vector[i] = (*df)(p, base_vector[i], virt_vector[i]); +} } } @@ -2322,8 +2331,7 @@ AP_DECLARE(void) ap_fixup_virtual_hosts(apr_pool_t dconf->log = &main_server->log; for (virt = main_server->next; virt; virt = virt->next) { -merge_server_configs(p, main_server->module_config, - virt->module_config); +merge_server_configs(p, main_server->module_config, virt); virt->lookup_defaults = ap_merge_per_dir_configs(p, main_server->lookup_defaults, Index: modules/ssl/mod_ssl.c === --- modules/ssl/mod_ssl.c (revision 1809129) +++ modules/ssl/mod_ssl.c (working copy) @@ -712,5 +712,6 @@ module AP_MODULE_DECLARE_DATA ssl_module = { ssl_config_server_create, /* create per-server config structures */ ssl_config_server_merge,/* merge per-server config structures */ ssl_config_cmds,/* table of configuration directives */ -ssl_register_hooks /* register hooks */ +ssl_register_hooks, /* register hooks */ +AP_MODULE_FLAG_ALWAYS_MERGE /* flags */ };
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 7:42 AM, Stefan Eissing wrote: > >> Am 21.09.2017 um 13:35 schrieb Eric Covener : >> >> On Thu, Sep 21, 2017 at 7:00 AM, Yann Ylavic wrote: >>> On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing >>> wrote: > Am 21.09.2017 um 11:37 schrieb Yann Ylavic : > > If the module defines its own server_config_create() which allocates > one, each vhost will have its own, and the module's > server_config_merge() can do whatever needs to for the members of the > config (pointer copy, shallow/deep copy, ...). Yes, but only *iff* there is every a directive of that module used in a VirtualHost. >>> >>> OK, I see know, thanks. >>> >>> I'd call that a premature optimization though, even if it matured for >>> decades :) >>> Only the module knows what to do when merging, so I think the core >>> config should still call config_create() and config_merge() for those, >>> precisely because post_config() is always called. >>> Modules also know how to merge configs that do nothing (usually), with >>> all those _set members all over the place, so it should work (untested >>> ;) even if it may consume more (not that much I think) initial memory >>> for large confs. >> >> Maybe less intrusive for a module to make a copy of its config if it >> detects base/vh are the same and needs them to differ? > > That is why I "fixed" this for mod_ssl only. But mod_md has similar code now. > > Do we have some sort of bitfield of flags for each module? That would be great > as we could make an opt-in change that way. > IIUC it should be safe to extend module_struct with a minor bump to add 'int flags' to the bottom, but when you check the value you'd need to check the MMN first. In the module you'd then just have some flags or'ed together after register_hooks. (hopefully someone will check my work) -- Eric Covener cove...@gmail.com
Re: SSLSrvConfigRec shared
> Am 21.09.2017 um 13:35 schrieb Eric Covener : > > On Thu, Sep 21, 2017 at 7:00 AM, Yann Ylavic wrote: >> On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing >> wrote: >>> Am 21.09.2017 um 11:37 schrieb Yann Ylavic : If the module defines its own server_config_create() which allocates one, each vhost will have its own, and the module's server_config_merge() can do whatever needs to for the members of the config (pointer copy, shallow/deep copy, ...). >>> >>> Yes, but only *iff* there is every a directive of that module used in >>> a VirtualHost. >> >> OK, I see know, thanks. >> >> I'd call that a premature optimization though, even if it matured for >> decades :) >> Only the module knows what to do when merging, so I think the core >> config should still call config_create() and config_merge() for those, >> precisely because post_config() is always called. >> Modules also know how to merge configs that do nothing (usually), with >> all those _set members all over the place, so it should work (untested >> ;) even if it may consume more (not that much I think) initial memory >> for large confs. > > Maybe less intrusive for a module to make a copy of its config if it > detects base/vh are the same and needs them to differ? That is why I "fixed" this for mod_ssl only. But mod_md has similar code now. Do we have some sort of bitfield of flags for each module? That would be great as we could make an opt-in change that way. -Stefan
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 7:00 AM, Yann Ylavic wrote: > On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing > wrote: >> >>> Am 21.09.2017 um 11:37 schrieb Yann Ylavic : >>> >>> If the module defines its own server_config_create() which allocates >>> one, each vhost will have its own, and the module's >>> server_config_merge() can do whatever needs to for the members of the >>> config (pointer copy, shallow/deep copy, ...). >> >> Yes, but only *iff* there is every a directive of that module used in >> a VirtualHost. > > OK, I see know, thanks. > > I'd call that a premature optimization though, even if it matured for decades > :) > Only the module knows what to do when merging, so I think the core > config should still call config_create() and config_merge() for those, > precisely because post_config() is always called. > Modules also know how to merge configs that do nothing (usually), with > all those _set members all over the place, so it should work (untested > ;) even if it may consume more (not that much I think) initial memory > for large confs. Maybe less intrusive for a module to make a copy of its config if it detects base/vh are the same and needs them to differ?
Re: SSLSrvConfigRec shared
On Thu, Sep 21, 2017 at 11:48 AM, Stefan Eissing wrote: > >> Am 21.09.2017 um 11:37 schrieb Yann Ylavic : >> >> If the module defines its own server_config_create() which allocates >> one, each vhost will have its own, and the module's >> server_config_merge() can do whatever needs to for the members of the >> config (pointer copy, shallow/deep copy, ...). > > Yes, but only *iff* there is every a directive of that module used in > a VirtualHost. OK, I see know, thanks. I'd call that a premature optimization though, even if it matured for decades :) Only the module knows what to do when merging, so I think the core config should still call config_create() and config_merge() for those, precisely because post_config() is always called. Modules also know how to merge configs that do nothing (usually), with all those _set members all over the place, so it should work (untested ;) even if it may consume more (not that much I think) initial memory for large confs. > > The assumption here is that module configs at that point-in-time are > read-only. Which is not really true since several modules make changes > in the post_config phase that happens afterwards. Agreed. > > My fix for this in mod_ssl is part of > http://svn.apache.org/viewvc?view=revision&revision=1809037 > which calls ssl_config_server_uniq() once at the post_init phase > for each server. Or maybe do the right thing (IMHO) in core. >>> >>> Now, are you speaking of changing that for all modules? Add a flag to >>> "struct module" >>> or solve it in mod_ssl post_config? >> >> Of course not, create/merge process per module can already do that, up >> to the module to do what it needs (correctly). Finally, maybe :) It could be "suprising" for any module. Let's see what others think...
Re: SSLSrvConfigRec shared
> Am 21.09.2017 um 11:37 schrieb Yann Ylavic : > > Hi Stefan, > > On Wed, Sep 20, 2017 at 2:06 PM, Stefan Eissing > wrote: >> >>> Am 20.09.2017 um 12:33 schrieb Yann Ylavic : >>> >>> On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing >>> wrote: Is there some better way? >>> >>> I would go with the usual/unconditional per server config (and hence >>> merging), trade simplicity vs a few memory space... >> >> Not sure I get your dift here. > > I think you lost me too :) > > Let's try to get in sync... Sure! :D >> >> server/config.c calls merge_server_configs() for each non-base server_rec >> and that one copies the config pointer from base if the vhost has none. >> if there is one, it merges. > > If the module defines its own server_config_create() which allocates > one, each vhost will have its own, and the module's > server_config_merge() can do whatever needs to for the members of the > config (pointer copy, shallow/deep copy, ...). Yes, but only *iff* there is every a directive of that module used in a VirtualHost. > I didn't re-looked at the mod_ssl config code lately, I remember some > special treatment for main server config (re process->pool), but it > seems to me that each vhost has its own config, and that the merge > process issues copies (shallow?). mod_ssl does nothing special here, just normal module stuff. The special thing happens in server/config.c: merge_server_configs() that goes over all modules and all virtual servers and either merges the modules config *or* just assigns the pointer from the base_server's one. The assumption here is that module configs at that point-in-time are read-only. Which is not really true since several modules make changes in the post_config phase that happens afterwards. My fix for this in mod_ssl is part of http://svn.apache.org/viewvc?view=revision&revision=1809037 which calls ssl_config_server_uniq() once at the post_init phase for each server. Cheers, Stefan > What I don't remember is some modifications of the server configs in > place in post_config, if that's the case it should indeed be preceded > by a deep copy sowehow... > >> >> Now, are you speaking of changing that for all modules? Add a flag to >> "struct module" >> or solve it in mod_ssl post_config? > > Of course not, create/merge process per module can already do that, up > to the module to do what it needs (correctly). > > > Regards, > Yann.
Re: SSLSrvConfigRec shared
Hi Stefan, On Wed, Sep 20, 2017 at 2:06 PM, Stefan Eissing wrote: > >> Am 20.09.2017 um 12:33 schrieb Yann Ylavic : >> >> On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing >> wrote: >>> >>> Is there some better way? >> >> I would go with the usual/unconditional per server config (and hence >> merging), trade simplicity vs a few memory space... > > Not sure I get your dift here. I think you lost me too :) Let's try to get in sync... > > server/config.c calls merge_server_configs() for each non-base server_rec > and that one copies the config pointer from base if the vhost has none. > if there is one, it merges. If the module defines its own server_config_create() which allocates one, each vhost will have its own, and the module's server_config_merge() can do whatever needs to for the members of the config (pointer copy, shallow/deep copy, ...). I didn't re-looked at the mod_ssl config code lately, I remember some special treatment for main server config (re process->pool), but it seems to me that each vhost has its own config, and that the merge process issues copies (shallow?). What I don't remember is some modifications of the server configs in place in post_config, if that's the case it should indeed be preceded by a deep copy sowehow... > > Now, are you speaking of changing that for all modules? Add a flag to "struct > module" > or solve it in mod_ssl post_config? Of course not, create/merge process per module can already do that, up to the module to do what it needs (correctly). Regards, Yann.
Re: SSLSrvConfigRec shared
> Am 20.09.2017 um 12:33 schrieb Yann Ylavic : > > On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing > wrote: >> >> Is there some better way? > > I would go with the usual/unconditional per server config (and hence > merging), trade simplicity vs a few memory space... Not sure I get your dift here. server/config.c calls merge_server_configs() for each non-base server_rec and that one copies the config pointer from base if the vhost has none. if there is one, it merges. Now, are you speaking of changing that for all modules? Add a flag to "struct module" or solve it in mod_ssl post_config? Cheers, Stefan > Regards, > Yann.
Re: SSLSrvConfigRec shared
On Wed, Sep 20, 2017 at 12:09 PM, Stefan Eissing wrote: > > Is there some better way? I would go with the usual/unconditional per server config (and hence merging), trade simplicity vs a few memory space... Regards, Yann.
SSLSrvConfigRec shared
mod_ssl's server_rec configurations (SSLSrvConfigRec) are shared between vhost and base server *iff* there are no SSL* directives used inside a VirtualHost. This is not really a good idea since mod_ssl modifies these recs in its post_config hook. This looks currently harmless, e.g. setting sc->vhost_id n times (but the vhost_id is wrong for all but the last). With adding certificate/keys in post-config (mod_md) this sharing can no longer happen. To be precise: this is a side effect of a global "SSLEngine" config. The old-skool "SSLEngine on" in each vhost will cause every server_rec to have its own SSLSrvConfigRec instance and things work. Now, I would like both cases to work. Does anyone have a recommendation? My current thoughts go like (pseudo code): if (server != base_server && sslconf(server) == sslconf(base_server)) { newconf = conf_merge(new_server_conf(), sslconf(base_server)); ap_set_module_config(server, newconf); } Is there some better way? Cheers, Stefan