Stas Bekman <[EMAIL PROTECTED]> writes:
[...]
Looking at the most recent post on the topic from Sep 30th, you were
still discussing some nuances, like MP_APR_POOL_SV_DROPS_OWNERSHIP,
and I was expecting to see the final patch, before reviewing it.
Err, ok- personally I thought the discussion ended, and it was now a matter of incorporating the suggested tweaks
and committing it.
Philippe was in charge of that thread, so I wasn't closely following it.
I want to commit the bucket changes today also,
sure, I've already +1'ed that patch ;)
and then move on to fixing the t/directive/cmdparams.t
segfaults on amd64 (also reported+patched earlier this week). Knocking the pool patch out first seems like a good idea.
AFAIK, Philippe will show up only tomorrow, but if you think it's fine proceed with it that's fine, and if anything pops up it can be rectified later.
Mind to post the latest version (sorry if you did already and I've missed it).
Sure- with Philippe's t/apr/pool_lifetime.t test included.
I have a few minor comments below:
+#define MP_APR_POOL_SV_DROPS_OWNERSHIP(acct) do { \
s/do/STMT_START/
+ dTHXa(acct->perl); \
+ mg_free(acct->sv); \
+ SvIVX(acct->sv) = 0; \
+ if (modperl_opt_interp_unselect && acct->interp) { \
+ /* this will decrement the interp refcnt until \
+ * there are no more references, in which case \
+ * the interpreter will be putback into the mip \
+ */ \
+ (void)modperl_opt_interp_unselect(acct->interp); \
+ } \
+} while (0)
s/while (0)/STMT_END/
+#define MP_APR_POOL_SV_TAKES_OWNERSHIP(SV, P) do { \
- s/do/STMT_START/, well, here and everywhere below
- why upcased SV, P? I find that confusing.
/* XXX: should we make it a new global tracing category * MOD_PERL_TRACE=p for tracing pool management? */ #define MP_POOL_TRACE_DO 0 @@ -50,26 +121,8 @@ static MP_INLINE apr_status_t mpxs_apr_pool_cleanup(void *cleanup_data) { - mpxs_pool_account_t *data; - apr_pool_userdata_get((void **)&data, MP_APR_POOL_NEW, - (apr_pool_t *)cleanup_data); - if (!(data && data->sv)) { - /* if there is no data, there is nothing to unset */ - MP_POOL_TRACE(MP_FUNC, "this pool seems to be destroyed already"); - } - else { - MP_POOL_TRACE(MP_FUNC, - "pool 0x%lx contains a valid sv 0x%lx, invalidating it", - (unsigned long)data->sv, (unsigned long)cleanup_data); - - /* invalidate all Perl objects referencing this sv */ - SvIVX(data->sv) = 0; - - /* invalidate the reference stored in the pool */ - data->sv = NULL; - /* data->sv will go away by itself when all objects will go away */ - } - + mpxs_pool_account_t *acct = cleanup_data; + MP_APR_POOL_SV_DROPS_OWNERSHIP(acct); return APR_SUCCESS; }
@@ -116,9 +169,6 @@ * mess, trying to destroy an already destroyed pool or even worse * a pool allocate in the place of the old one. */ - apr_pool_cleanup_register(child_pool, (void *)child_pool, - mpxs_apr_pool_cleanup, - apr_pool_cleanup_null);
As you have removed that code, is the comment above still relevant? should it be moved elsewhere? It seems to be hanging in the air now.
-- __________________________________________________________________ Stas Bekman JAm_pH ------> Just Another mod_perl Hacker http://stason.org/ mod_perl Guide ---> http://perl.apache.org mailto:[EMAIL PROTECTED] http://use.perl.org http://apacheweek.com http://modperlbook.org http://apache.org http://ticketmaster.com
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
