heh, revistited... it never was about current_callback. my bad :)
Stas Bekman wrote:
After enabling 'PerlSwitches -w' a few new "Use of uninitialized variable" warnings have popped up. One of them was due to this child_exit() handler:
sub ModPerl::Test::exit_handler { my($p, $s) = @_;
$s->log->info("Child process pid=$$ is exiting"); }
which didn't return a value. It actually did return a value via the last call to info(), which according to XS should return XSRETURN_EMPTY, which for some reason left some SV on the stack, which was recognized by Devel::Peek as Undef but it wasn't really &PL_sv_undef.
exactly - undef isn't PL_sv_undef for all definitions of undef, which is partially why I didn't want to explicity check against PL_sv_undef, but rather check first the IV slot, then the PV slot, _then_ fallback.
agreed, see below
I think the generated warning is a good solution, but the problem is that it doesn't show where does it happen, since we have left the scope of the callback by the time we retrieve the value off-the stack. Perl_sv_2iv calls:
void
Perl_report_uninit(pTHX)
{
if (PL_op)
Perl_warner(aTHX_ packWARN(WARN_UNINITIALIZED), PL_warn_uninit,
" in ", OP_DESC(PL_op));
else
Perl_warner(aTHX_ packWARN(WARN_UNINITIALIZED), PL_warn_uninit, "", "");
}
and since PL_op is not defined, it just prints a dummy warnings, with no pointers. If could fake PL_op to point to that handler that was just executed, it'll will tell the exact location of the warning. So "all" we have to figure out is how to fake that PL_op thingy...
Another solution would be to rip-off the logic from Perl_sv_2iv (there are 3 places where it calls report_uninit, and provide a custom warning message, but there is quite a lot of logic to duplicate, which is executed anyway.
Finally we could try to temprorary override the PL_warn_uninit format string to provide a custom error message. may be that will be the easiest thing to do.
uh... sure :)
Discussed this with Rafael, there is no easy way to trick perl into doing what we want. Therefore, I gave up on that idea.
or we could just check for IV (normal circumstances), PV (coerced ints), PL_sv_undef (real calls to Perl_croak), then throw an official error_log message for anything else.
that's exactly the final conclusiong I came to after messing with return statuses, the error should be fatal though if we agree that it's an error.
I'm not against putting things in the error_log if it's due to not following the API, and the API requires that handlers exit with a return code that is meaningful to Apache - various definitions of "undef" are not meaningful. so, save the special cases like die and MP::U::exit() where we fix things (trusting that Perl returns the real PL_sv_undef) just throw the error. something like "Perl handler did not return a valid value" or something. I don't see any reason to break that down into the line number - if we can get the name of the routine from the stash (or anon for a coderef) that ought to be sufficient.
So here is the version which seems to be cool, same as your original patch, plus returning 500 if it's not IV/PV/undef. I couldn't find anything better than returning 500, which is probably not good at all, since it's an HTTP code and modperl_callback handles so many other things. One problem is with hooks that ignore return values, the current ModPerl::Test::exit_handler, which returns no explicit value can't cause a sensible error, because httpd ignores child_exit status. I have tried adding Perl_croak, but it doesn't seem to do anything at all at the child_exit phase.
So here it is, sans comments, to make it easier to read here:
else {
SV *status_sv = POPs; if (SvIOK(status_sv)) {
status = SvIVX(status_sv);
MP_TRACE_h(MP_FUNC,
"callback %s returned integer %d\n",
handler->name, status);
}
else if (status_sv == &PL_sv_undef) {
status = OK;
MP_TRACE_h(MP_FUNC, "callback %s returned undef",
handler->name);
}
else if (SvPOK(status_sv)) {
status = SvIVx(status_sv);
MP_TRACE_h(MP_FUNC, "callback %s returned a string - "
"coerced to integer status %d",
handler->name, status);
}
else {
status = HTTP_INTERNAL_SERVER_ERROR;
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
"handler %s didn't return a valid return value!",
handler->name);
} /* assume OK for non-http status codes and for 200 (HTTP_OK) */
if (((status > 0) && (status < 100)) ||
(status == 200) || (status > 600)) {
status = OK;
}
}also I think we should drop the traces for all but the PV case, since they are proper and we already get another trace from modperl_callback_run_handlers which calls modperl_callback, so this duplication is just confusing. So something like this?
if (SvIOK(status_sv)) {
status = SvIVX(status_sv);
}
else if (status_sv == &PL_sv_undef) {
status = OK;
}
else if (SvPOK(status_sv)) {
status = SvIVx(status_sv);
MP_TRACE_h(MP_FUNC,
"coercing handler %s's return value '%s' into %d",
handler->name, SvPVX(status_sv), status);
}
else {
status = HTTP_INTERNAL_SERVER_ERROR;
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
"handler %s didn't return a valid return value!",
handler->name);
}__________________________________________________________________ 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]
