perl-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Philippe M. Chiasson" <go...@ectoplasm.org>
Subject Re: what appeared to be a small typo
Date Tue, 06 Apr 2010 15:40:46 GMT
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/


Mime
View raw message