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.

> 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.

HTH,

Alex.
P.S. I have not verified the correctness of urlMakeAbsolute internals.
Received on Wed Sep 03 2008 - 04:20:53 MDT