Stas Bekman wrote:
Geoffrey Young wrote:
is it possible that that later on we will have callbacks that return a string, which is a valid thing? Aren't we guessing here again?
well, I suppose DIR_MAGIC_TYPE and DECLINE_CMD are both strings, but they managed to (somehow) work before.
I suppose, that's because we never return these?
right about DIR_MAGIC_TYPE. with DECLINE_CMD, modperl_module is using a custom callback routine instead of modperl_callback() (I did some more research :)
well, don't confuse the issue. this has nothing to do with default_port. the pv -> iv thing is a problem for any handler (as my tests show). I think the real culprit was the last change from POPi to POPs - maybe POPi was already doing sv_2iv conversions behind the scenes.
right, sorry. So it should be easy to revert in the previous code, once we figure out what gets onto the stack when exit() is called (I'd guess that it'll be undef).
according to perlapi, yes: for Perl_croak count is 1 and undef is on the stack with G_EVAL.
I haven't really looked at it, thinking that we can just check whether the int slot is valid. So I really introduced a potential bug then if someone was returning "0" and it used to work.
yeah, it looks like "return values as strings" used to work and now they don't. but that's ok, we'll fix it :)
Also for some reason I wasn't able to even reproduce the undef warning in the test suite, but easily reproduced outside of the test suite.
test suites are good for excercising functionality and some error conditions, but not always for reproducing real-world issues like that, unfortunately.
can you think of situation where something other than an integer would be returned?
not with the currently supported callbacks, but as you seem to plan to embrace all available hooks, I'm not familiar with all of them to tell ;)
me neither, but I'm learning as I go :)
everything I've seen (save DECLINE_CMD) is an integer, even the new auth constants in 2.1. so, I'd venture to say we're safe (for the time being, anyway) to just use integers. if we eventually need something that doesn't return an int, we can either hack modperl_callback, generate a new function for SVs, or split the logic up as needed.
with '9bar' yes. is there a C routine to check whether a string is a valid integer? if so, we could use it and throw 500.
Looks like Perl_grok_number is what you are after.
ah, cool.
Other than that isdigit(3) works on single chars.
But doing this in a general case would be expensive.
agreed.
I think a plan coercing into IV as before will do just as well.
well, I'd like to add the integer check if we coerce the PV into an int, just to be safe - that's probably the (very) rare case, but I'd like to be able to say _something_ about it in the logs.
I suppose that it already uses grok_number internally.
I dunno. I'll add an alphanumeric value to my tests and see what happens.
why 1? it can be any value from the last evaluation if there is no explicit return.
just because perl uses 1 for true, and things like print() and other functions return 1.
but yes, we can't protect against everything. I was just trying to make it a bit more obvious if users make an obvious mistake.
so we are guessing again?
sorta. with the patch I submitted, the 1 is allowed to continue on to Apache - only a warning is thrown.
I somehow feel that this will be a common mistake (it showed up in the test suite, in fact) that will be difficult to debug - when a return value is forgotten, the server merely sends a 500 with no errors logged at all.
but I'm not going to put up a fuss - I was just trying to anticipate user problems, which isn't always a good idea :)
at any rate, I'll come up with another patch tomorrow (if I have time - apachecon slides are looming). however, after this discussion, I don't think it will be too much different - just the grok_integer and special treatment of 1 issues, right?
--Geoff
--------------------------------------------------------------------- To unsubscribe, e-mail: [EMAIL PROTECTED] For additional commands, e-mail: [EMAIL PROTECTED]
