apr-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Stas Bekman <s...@stason.org>
Subject Re: [patch] handling the missing scheme in uri unparse
Date Mon, 03 Mar 2003 22:49:36 GMT
William A. Rowe, Jr. wrote:
> At 01:57 AM 2/18/2003, Stas Bekman wrote:
> 
>>the uri unparse code wasn't properly ported from 1.0. The default scheme was dropped
on the floor, now causing segfaults. In order to stay back compatible with 1.0 code the following
patch should be applied.
>>
>>In order to avoid segfaults, and break backcompat, the code should at least do:
>>       /* Construct scheme://site string */
>>       if (uptr->hostname && uptr->scheme) {
>>
>>and not just if (uptr->hostname)
> 
> 
> ++1 to that.  My comments about your patch are a little lower, but
> I first decided to struggle through your suggestion above.  I went 
> one step further, in violation of (but also in the spirit of) rfc2396,
> and have committed the fix described here to avoid segfaults...
> 
> <scheme>: is not optional by rfc2396 (the violation I mentioned), but if 
> the user fails to provide one, who are we to argue?  Now the question 
> becomes what to include if scheme is omitted but hostname (and 
> optionally port) are provided?  My patch goes ahead and assembles
> them, but I was left with the question of 'how?'.
> 
> I looked to this defintion;
> 
> absoluteURI   = scheme ":" ( hier_part | opaque_part )
> 
> So the colon ties the scheme to the absolute URI, not "://".  So my segfault
> workaround simply drops "scheme:" if scheme is NULL.  This eliminates the
> segfault, and assembles the (heir_part) for a resources that include an
> authority (sec 2.3 of the rfc) by prepending the '//' if the authority (hostname)
> is present.
> 
> Comments about that patch that's now committed?

1. The patching of segfault was the most important.

2. I actually like the weird //localhost/foo/bar as it'll force the user to 
re-adjust their code, whereas /foo/bar may just work.

> ***
> 
> Now to my comments about your patch below.  As I was reviewing your 
> patch, I am convinced that the current apr_uri implementation is and was
> intended to be scheme-agnostic.  Your patch would break the agnostic
> behavior of assigning an arbitrary scheme.  I don't think that is in the
> interest of forcing the API's users to be explicit.  (I had the impression
> we were defaulting the port to 80 when we were chatting on IRC, I now
> see we don't make any such assumptions.)
> 
> A better solution to Apache::compat's implementation would be to test the
> scheme and (if empty) fill it in with http: since that's *Apache::compat*'s
> preference.  But it really shouldn't be a universal preference.  E.g. a pop
> or nntp parser wouldn't appreciate those defaults, while Apache::compat
> certainly likes that default.
> 
> If compat's job is to make everything look the same for the 1.3 module
> porter, then do so ;-)  But let's not twist this API to make it http-centric
> again.  Since it's simple for the user to fill in NULL members of the
> apr_uri_t with their preferred defaults, let them do so.
> 
> At least we've quit segfaulting, which was the hardest debugging job for 
> the casual user.  Now they get a string they should be unhappy with, 
> and can decide how to react and fix their code without gdb :-)

I'll adjust the compat code to do as you suggest.

Thank you, Bill!

__________________________________________________________________
Stas Bekman            JAm_pH ------> Just Another mod_perl Hacker
http://stason.org/     mod_perl Guide ---> http://perl.apache.org
mailto:stas@stason.org http://use.perl.org http://apacheweek.com
http://modperlbook.org http://apache.org   http://ticketmaster.com


Mime
View raw message