You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@perl.apache.org by Torsten Förtsch <to...@gmx.net> on 2010/04/06 12:51:11 UTC
what appeared to be a small typo
Hi,
in modperl_filter.c my gcc gave a warning on this line:
if (!init_handler->attrs & MP_FILTER_INIT_HANDLER) {
Perl_croak(aTHX_ "handler %s doesn't have "
"the FilterInitHandler attribute set",
modperl_handler_name(init_handler));
}
Without parentheses this is interpreted as
if ((!init_handler->attrs) & MP_FILTER_INIT_HANDLER) {
MP_FILTER_INIT_HANDLER is a constant, 0x8. So, the expression can be reduced
to "if (0)" or "if (1)" depending upon the numeric value of 1==1. I think this
is not what is meant.
Now, if I change the if to what I think is meant:
if (!(init_handler->attrs & MP_FILTER_INIT_HANDLER)) {
the test server won't even start:
handler TestFilter::in_init_basic::suicide_init doesn't have the
FilterInitHandler attribute set.
This is correct, the attrs field is 0.
I think the culprit is the modperl_handler_new_from_sv function. It does not
set the attrs field.
Any objections against this patch?
Index: src/modules/perl/modperl_handler.c
===================================================================
--- src/modules/perl/modperl_handler.c (revision 930668)
+++ src/modules/perl/modperl_handler.c (working copy)
@@ -497,6 +497,7 @@
{
char *name = NULL;
GV *gv;
+ modperl_handler_t *handler;
if (SvROK(sv)) {
sv = SvRV(sv);
@@ -515,7 +516,10 @@
Perl_croak(aTHX_ "can't resolve the code reference");
}
name = apr_pstrcat(p, HvNAME(GvSTASH(gv)), "::", GvNAME(gv), NULL);
- return modperl_handler_new(p, apr_pstrdup(p, name));
+ handler = modperl_handler_new(p, name);
+ handler->attrs=*modperl_code_attrs(aTHX_ (CV*)sv);
+
+ return handler;
default:
break;
};
Index: src/modules/perl/modperl_filter.c
===================================================================
--- src/modules/perl/modperl_filter.c (revision 930668)
+++ src/modules/perl/modperl_filter.c (working copy)
@@ -407,7 +407,9 @@
MP_TRACE_h(MP_FUNC, "found init handler %s",
modperl_handler_name(init_handler));
- if (!init_handler->attrs & MP_FILTER_INIT_HANDLER) {
+ warn("init_attrs(%s)=%x\n", modperl_handler_name(init_handler),
+ init_handler->attrs);
+ if (!(init_handler->attrs & MP_FILTER_INIT_HANDLER)) {
Perl_croak(aTHX_ "handler %s doesn't have "
"the FilterInitHandler attribute set",
modperl_handler_name(init_handler));
@@ -656,6 +658,10 @@
(void)SvUPGRADE(buffer, SVt_PV);
SvPOK_only(buffer);
SvCUR(buffer) = 0;
+ SvGROW(buffer, 1); /* make PERL_ARGS_ASSERT_SV_CATPVN_FLAGS
+ * happy, see Perl_sv_catpvn_flags() in sv.c
+ * of perl 5.10.1
+ */
/* sometimes the EOS bucket arrives in the same brigade with other
* buckets, so that particular read() will not return 0 and will
The SvGROW thing was necessary due to perl 5.10.1 as the comment explains. It
is not related to the attributes problem. I can commit it in a separate step.
The patch also eliminates the useless apr_strdup in
name = apr_pstrcat(p, HvNAME(GvSTASH(gv)), "::", GvNAME(gv), NULL);
return modperl_handler_new(p, apr_pstrdup(p, name));
Torsten F�rtsch
--
Need professional modperl support? Hire me! (http://foertsch.name)
Like fantasy? http://kabatinte.net
Re: what appeared to be a small typo
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
On 10-04-06 06:51 , Torsten Förtsch wrote:
> Hi,
>
> in modperl_filter.c my gcc gave a warning on this line:
>
> if (!init_handler->attrs & MP_FILTER_INIT_HANDLER) {
> Perl_croak(aTHX_ "handler %s doesn't have "
> "the FilterInitHandler attribute set",
> modperl_handler_name(init_handler));
> }
>
> Without parentheses this is interpreted as
>
> if ((!init_handler->attrs) & MP_FILTER_INIT_HANDLER) {
>
> MP_FILTER_INIT_HANDLER is a constant, 0x8. So, the expression can be reduced
> to "if (0)" or "if (1)" depending upon the numeric value of 1==1. I think this
> is not what is meant.
>
> Now, if I change the if to what I think is meant:
>
> if (!(init_handler->attrs & MP_FILTER_INIT_HANDLER)) {
>
> the test server won't even start:
>
> handler TestFilter::in_init_basic::suicide_init doesn't have the
> FilterInitHandler attribute set.
>
> This is correct, the attrs field is 0.
Awesome, I ran into this just a few days ago when I started looking at
your io patch.
> I think the culprit is the modperl_handler_new_from_sv function. It does not
> set the attrs field.
Ah, nice, I didn't have the time to dig into the source of the problem.
> Any objections against this patch?
>
> Index: src/modules/perl/modperl_handler.c
> ===================================================================
> --- src/modules/perl/modperl_handler.c (revision 930668)
> +++ src/modules/perl/modperl_handler.c (working copy)
> @@ -497,6 +497,7 @@
> {
> char *name = NULL;
> GV *gv;
> + modperl_handler_t *handler;
>
> if (SvROK(sv)) {
> sv = SvRV(sv);
> @@ -515,7 +516,10 @@
> Perl_croak(aTHX_ "can't resolve the code reference");
> }
> name = apr_pstrcat(p, HvNAME(GvSTASH(gv)), "::", GvNAME(gv), NULL);
> - return modperl_handler_new(p, apr_pstrdup(p, name));
> + handler = modperl_handler_new(p, name);
> + handler->attrs=*modperl_code_attrs(aTHX_ (CV*)sv);
> +
> + return handler;
> default:
> break;
> };
> Index: src/modules/perl/modperl_filter.c
> ===================================================================
> --- src/modules/perl/modperl_filter.c (revision 930668)
> +++ src/modules/perl/modperl_filter.c (working copy)
> @@ -407,7 +407,9 @@
> MP_TRACE_h(MP_FUNC, "found init handler %s",
> modperl_handler_name(init_handler));
>
> - if (!init_handler->attrs & MP_FILTER_INIT_HANDLER) {
> + warn("init_attrs(%s)=%x\n", modperl_handler_name(init_handler),
> + init_handler->attrs);
Looks like a bit of leftover debugging code ? Or convert it to a TRACE
macro ?
> + if (!(init_handler->attrs & MP_FILTER_INIT_HANDLER)) {
> Perl_croak(aTHX_ "handler %s doesn't have "
> "the FilterInitHandler attribute set",
> modperl_handler_name(init_handler));
> @@ -656,6 +658,10 @@
> (void)SvUPGRADE(buffer, SVt_PV);
> SvPOK_only(buffer);
> SvCUR(buffer) = 0;
> + SvGROW(buffer, 1); /* make PERL_ARGS_ASSERT_SV_CATPVN_FLAGS
> + * happy, see Perl_sv_catpvn_flags() in sv.c
> + * of perl 5.10.1
> + */
>
> /* sometimes the EOS bucket arrives in the same brigade with other
> * buckets, so that particular read() will not return 0 and will
>
> The SvGROW thing was necessary due to perl 5.10.1 as the comment explains. It
> is not related to the attributes problem. I can commit it in a separate step.
Yes, I'd keep that separate as a distinct commit.
> The patch also eliminates the useless apr_strdup in
>
> name = apr_pstrcat(p, HvNAME(GvSTASH(gv)), "::", GvNAME(gv), NULL);
> return modperl_handler_new(p, apr_pstrdup(p, name));
Yeah, good catch!
--
Philippe M. Chiasson GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/ m/gozer\@(apache|cpan|ectoplasm)\.org/
Re: what appeared to be a small typo
Posted by "Philippe M. Chiasson" <go...@ectoplasm.org>.
On 10-04-06 11:44 , Torsten Förtsch wrote:
> On Tuesday 06 April 2010 12:51:11 Torsten Förtsch wrote:
>> Any objections against this patch?
>>
> I have separated the previous patch into 2
> 1) to address the problem. It calls now modperl_mgv_resolve instead of
> modperl_code_attrs.
>
> Index: src/modules/perl/modperl_filter.c
> ===================================================================
> --- src/modules/perl/modperl_filter.c (revision 930668)
> +++ src/modules/perl/modperl_filter.c (working copy)
> @@ -404,10 +404,12 @@
> FREETMPS;LEAVE;
>
> if (init_handler) {
> + modperl_mgv_resolve(aTHX_ init_handler, p, init_handler->name,
> 1);
> +
> MP_TRACE_h(MP_FUNC, "found init handler %s",
> modperl_handler_name(init_handler));
>
> - if (!init_handler->attrs & MP_FILTER_INIT_HANDLER) {
> + if (!(init_handler->attrs & MP_FILTER_INIT_HANDLER)) {
> Perl_croak(aTHX_ "handler %s doesn't have "
> "the FilterInitHandler attribute set",
> modperl_handler_name(init_handler));
This is a good, self-contained fix for a bug, +1
> @@ -656,6 +658,10 @@
> (void)SvUPGRADE(buffer, SVt_PV);
> SvPOK_only(buffer);
> SvCUR(buffer) = 0;
> + SvGROW(buffer, 1); /* make PERL_ARGS_ASSERT_SV_CATPVN_FLAGS
> + * happy, see Perl_sv_catpvn_flags() in sv.c
> + * of perl 5.10.1
> + */
>
> /* sometimes the EOS bucket arrives in the same brigade with other
> * buckets, so that particular read() will not return 0 and will
>
This needs a separate commit.
> 2) to remove the superfluous apr_strdup.
>
> Index: src/modules/perl/modperl_handler.c
> ===================================================================
> --- src/modules/perl/modperl_handler.c (revision 930668)
> +++ src/modules/perl/modperl_handler.c (working copy)
> @@ -36,7 +36,8 @@
> break;
> }
>
> - handler->cv = NULL;
> + /* not necessary due to apr_pcalloc */
> + /* handler->cv = NULL; */
This is an unrelated change that belongs in its own cleanup checkin.
> handler->name = name;
> MP_TRACE_h(MP_FUNC, "[%s] new handler %s",
> modperl_pid_tid(p), handler->name);
> @@ -515,7 +516,7 @@
> Perl_croak(aTHX_ "can't resolve the code reference");
> }
> name = apr_pstrcat(p, HvNAME(GvSTASH(gv)), "::", GvNAME(gv), NULL);
> - return modperl_handler_new(p, apr_pstrdup(p, name));
> + return modperl_handler_new(p, name);
> default:
> break;
> };
+1
--
Philippe M. Chiasson GPG: F9BFE0C2480E7680 1AE53631CB32A107 88C3A5A5
http://gozer.ectoplasm.org/ m/gozer\@(apache|cpan|ectoplasm)\.org/
Re: what appeared to be a small typo
Posted by Torsten Förtsch <to...@gmx.net>.
On Tuesday 06 April 2010 12:51:11 Torsten F�rtsch wrote:
> Any objections against this patch?
>
I have separated the previous patch into 2
1) to address the problem. It calls now modperl_mgv_resolve instead of
modperl_code_attrs.
Index: src/modules/perl/modperl_filter.c
===================================================================
--- src/modules/perl/modperl_filter.c (revision 930668)
+++ src/modules/perl/modperl_filter.c (working copy)
@@ -404,10 +404,12 @@
FREETMPS;LEAVE;
if (init_handler) {
+ modperl_mgv_resolve(aTHX_ init_handler, p, init_handler->name,
1);
+
MP_TRACE_h(MP_FUNC, "found init handler %s",
modperl_handler_name(init_handler));
- if (!init_handler->attrs & MP_FILTER_INIT_HANDLER) {
+ if (!(init_handler->attrs & MP_FILTER_INIT_HANDLER)) {
Perl_croak(aTHX_ "handler %s doesn't have "
"the FilterInitHandler attribute set",
modperl_handler_name(init_handler));
@@ -656,6 +658,10 @@
(void)SvUPGRADE(buffer, SVt_PV);
SvPOK_only(buffer);
SvCUR(buffer) = 0;
+ SvGROW(buffer, 1); /* make PERL_ARGS_ASSERT_SV_CATPVN_FLAGS
+ * happy, see Perl_sv_catpvn_flags() in sv.c
+ * of perl 5.10.1
+ */
/* sometimes the EOS bucket arrives in the same brigade with other
* buckets, so that particular read() will not return 0 and will
2) to remove the superfluous apr_strdup.
Index: src/modules/perl/modperl_handler.c
===================================================================
--- src/modules/perl/modperl_handler.c (revision 930668)
+++ src/modules/perl/modperl_handler.c (working copy)
@@ -36,7 +36,8 @@
break;
}
- handler->cv = NULL;
+ /* not necessary due to apr_pcalloc */
+ /* handler->cv = NULL; */
handler->name = name;
MP_TRACE_h(MP_FUNC, "[%s] new handler %s",
modperl_pid_tid(p), handler->name);
@@ -515,7 +516,7 @@
Perl_croak(aTHX_ "can't resolve the code reference");
}
name = apr_pstrcat(p, HvNAME(GvSTASH(gv)), "::", GvNAME(gv), NULL);
- return modperl_handler_new(p, apr_pstrdup(p, name));
+ return modperl_handler_new(p, name);
default:
break;
};
Torsten F�rtsch
--
Need professional modperl support? Hire me! (http://foertsch.name)
Like fantasy? http://kabatinte.net