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/
signature.asc
Description: OpenPGP digital signature
