Re: [MERGE] Further cleanup of urlAbsolute and friends.

> On Wed, 2008-09-03 at 11:04 +1000, Benno Rice wrote:
>> Ok, hopefully this meets with everybody's approval. Changes in this
>> version:
>>
>> - Split urlAbsolute into urlIsRelative and urlMakeAbsolute.
>> - Make urlIsRelative compliant with the RFC as to what defines a
>> relative URL.
>> - Use a malloc'ed buffer instead of a stack-allocated array and xstrdup
>> in
>> urlMakeAbsolute.
>> - Rework purgeEntriesByHeader to be a little easier on the eyes.
>
> Thank you for fixing and polishing the code! I only found one serious
> problem related to "const char *" return type of urlMakeAbsolute. Please
> feel free to ignore other comments.
>
>> int
>> urlIsRelative(const char *url)
>
> The above should return bool and use true/false constants as appropriate.
>
>
>> * It is assumed that you have already ensured that the URL is relative.
>> * If NULL is returned, you should use the original URL unchanged.
>
> This does not document what a NULL return value means (only what we
> should do with it, which is calling context dependent). The code seems
> to imply that a NULL return value is a special CONNECT method case.
> Perhaps that case should be handled by urlIsRelative (you will need to
> add the req argument to it)? After all, the CONNECT Request URI is
> "absolute" by the CONNECT method definition.
>
> Also, it is not documented whether/how the returned value should be
> freed.

Um, thats a big one.

>
>
>> const char *
>> +urlMakeAbsolute(const HttpRequest * req, const char *relUrl)
>
> If this is returning "const char*", we should not free (or modify) the
> result. Did you mean to use "char *" as the return value?
>
> BTW, safe_free (i.e., xxfree) prototype misleads that you can free a
> constant. That was probably inherited from Squid2/C, with an ugly
> const-removing cast inside the xxfree implementation.
>
>> + }
>> +
>> + if (req->port != urlDefaultPort(req->protocol)) {
>
> It could be my mailer, but the indentation above seems wrong.
>
>
>> + char *urlbuf;
>> + const char *path, *last_slash;
>> + size_t urllen, pathlen;
>
> Declaring variables as needed rather than in advance makes code
> maintenance easier and might help the compiler to optimize. Not a big
> deal though.

But it does needlessly break the oldest-possible-compiler policy.

Amos
Received on Wed Sep 03 2008 - 04:30:53 MDT