You are viewing a plain text version of this content. The canonical link for it is here.
Posted to apreq-dev@httpd.apache.org by Stas Bekman <st...@stason.org> on 2004/11/27 00:20:36 UTC
Re: cvs commit: httpd-apreq-2/glue/perl/xsbuilder/Apache/Upload Apache__Upload.h
joes@apache.org wrote:
> Tag all the *2sv macros with a "parent" argument to ensure the parent object
> isn't destroyed until the child object is done with it. Without this patch
> the following will dump core on the command line, because the underlying pool
> would be destroyed after the first statement is executed- thus the segfault
> in param(). This patch fixes that.
>
> % perl -MApache2 -MApache::Request -MAPR::Pool
> -wle '$r=Apache::Request->new(APR::Pool->new); print $r->param("foo")'
I'm looking at this change to adopt for modperl (per the current
discussion on the modperl dev list). I've some comments below:
> Revision Changes Path
> 1.32 +14 -13 httpd-apreq-2/glue/perl/xsbuilder/apreq_xs_postperl.h
>
> Index: apreq_xs_postperl.h
> ===================================================================
> RCS file: /home/cvs/httpd-apreq-2/glue/perl/xsbuilder/apreq_xs_postperl.h,v
> retrieving revision 1.31
> retrieving revision 1.32
> diff -u -r1.31 -r1.32
> --- apreq_xs_postperl.h 1 Jul 2004 18:26:33 -0000 1.31
> +++ apreq_xs_postperl.h 2 Jul 2004 04:40:08 -0000 1.32
> @@ -107,7 +107,7 @@
> * @return Reference to the new Perl object in class.
> */
> APR_INLINE
> -static SV *apreq_xs_c2perl(pTHX_ void *obj, void *env, const char *class)
> +static SV *apreq_xs_c2perl(pTHX_ void *obj, void *env, const char *class, SV *parent)
> {
> SV *rv = sv_setref_pv(newSV(0), class, obj);
> if (env) {
> @@ -120,7 +120,7 @@
> * 5.8.x is OK with the old way as well, but in the future
> * we may have to use "#if PERL_VERSION < 8" ...
> */
> - sv_magic(SvRV(rv), Nullsv, PERL_MAGIC_ext, Nullch, -1);
> + sv_magic(SvRV(rv), parent, PERL_MAGIC_ext, Nullch, -1);
> SvMAGIC(SvRV(rv))->mg_ptr = env;
> }
> return rv;
1) the new param is undocumented (not sure how to explain it, see below)
2) what happens if env == NULL? The parent dependancy will not be created.
(see the suggested fix below) If my correction is right, should the
comment be moved up too?
Index: glue/perl/xsbuilder/apreq_xs_postperl.h
===================================================================
--- glue/perl/xsbuilder/apreq_xs_postperl.h (revision 106667)
+++ glue/perl/xsbuilder/apreq_xs_postperl.h (working copy)
@@ -111,12 +111,14 @@
* @param obj C object.
* @param env C environment.
* @param class Class perl object will be blessed into.
+ * @param parent XXX
* @return Reference to the new Perl object in class.
*/
APR_INLINE
static SV *apreq_xs_c2perl(pTHX_ void *obj, void *env, const char
*class, SV *parent)
{
SV *rv = sv_setref_pv(newSV(0), class, obj);
+ sv_magic(SvRV(rv), parent, PERL_MAGIC_ext, Nullch, -1);
if (env) {
/* We use the old idiom for sv_magic() below,
* because perl 5.6 mangles the env pointer on
@@ -127,7 +129,6 @@
* 5.8.x is OK with the old way as well, but in the future
* we may have to use "#if PERL_VERSION < 8" ...
*/
- sv_magic(SvRV(rv), parent, PERL_MAGIC_ext, Nullch, -1);
SvMAGIC(SvRV(rv))->mg_ptr = env;
}
return rv;
--
__________________________________________________________________
Stas Bekman JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/ mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org http://ticketmaster.com
Re: cvs commit: httpd-apreq-2/glue/perl/xsbuilder/Apache/Upload
Apache__Upload.h
Posted by Joe Schaefer <jo...@sunstarsys.com>.
Stas Bekman <st...@stason.org> writes:
[...]
>> {
>> SV *rv = sv_setref_pv(newSV(0), class, obj);
>> if (env) {
>> @@ -120,7 +120,7 @@
>> * 5.8.x is OK with the old way as well, but in the future
>> * we may have to use "#if PERL_VERSION < 8" ...
>> */
>> - sv_magic(SvRV(rv), Nullsv, PERL_MAGIC_ext, Nullch, -1);
>> + sv_magic(SvRV(rv), parent, PERL_MAGIC_ext, Nullch, -1);
>> SvMAGIC(SvRV(rv))->mg_ptr = env;
>> }
>> return rv;
>
> 1) the new param is undocumented (not sure how to explain it, see below)
>
> 2) what happens if env == NULL? The parent dependancy will not be
> created. (see the suggested fix below) If my correction is right,
> should the comment be moved up too?
It might be worthwhile to include an assert(env != NULL) in that
function and see what happens. We may not need the conditional
at all. But if we really do need it, I think the sv_magic() call
should stay where it is.
--
Joe Schaefer