[MPlayer-dev-eng] [PATCH 3/7] Use authorization field to pass server authentication through proxy

[Reimar Döffinger]

On Sun, Nov 21, 2010 at 02:24:26PM +0100, Clément Bœsch wrote:
> diff --git a/stream/url.c b/stream/url.c
> index 0469de9..90f41f7 100644
> --- a/stream/url.c
> +++ b/stream/url.c
> @@ -58,6 +58,33 @@ URL_t *url_redirect(URL_t **url, const char *redir) {
>    return res;
>  }
>  
> +#ifdef __GNUC__
> +static void make_noauth_url(URL_t *url, const char *fmt, ...) __attribute__ ((format(printf, 2, 3)));
> +#endif
> +
> +static void make_noauth_url(URL_t *url, const char *fmt, ...)
> +{
> +    va_list va1, va2;
> +    int len;
> +
> +    va_start(va1, fmt);
> +    va_copy(va2, va1);
> +    len = vsnprintf(NULL, 0, fmt, va1);
> +    if (len < 0) {
> +        url->noauth_url = NULL;
> +        goto end;
> +    }
> +    len++;
> +    url->noauth_url = malloc(len);
> +    if (!url->noauth_url)
> +        goto end;
> +    vsnprintf(url->noauth_url, len, fmt, va2);
> +
> +end:
> +    va_end(va1);
> +    va_end(va2);
> +}
> +
>  URL_t*
>  url_new(const char* url) {
>  	int pos1, pos2,v6addr = 0;
> @@ -232,6 +259,17 @@ url_new(const char* url) {
>  		strcpy(Curl->file, "/");
>  	}
>  
> +	if (Curl->port)
> +		make_noauth_url(Curl, "%s://%s:%d%s", Curl->protocol,
> +				Curl->hostname, Curl->port, Curl->file);
> +	else
> +		make_noauth_url(Curl, "%s://%s%s", Curl->protocol,
> +				Curl->hostname, Curl->file);
> +	if (Curl->noauth_url == NULL) {
> +		mp_msg(MSGT_NETWORK, MSGL_FATAL, MSGTR_MemAllocFailed);
> +		goto err_out;
> +	}

This seems like a really bad way to split it.

static int make_noauth_url(URL_t *url, char *dst, int dst_size)
{
    if (url->port)
        return snprintf(dst, dst_size, "%s://%s:%d%s", url->protocol,
                        url->hostname, url->port, url->file);
    else
        return snprintf(dst, dst_size, "%s://%s%s", url->protocol,
                        url->hostname, url->file);
}


len = make_noauth_url(Curl, NULL, 0);
if (len > 0) {
    Curl->noauth_url = malloc(len);
    if (!Curl->noauth_url) {
    ...
    }
    make_noauth_url(Curl, Curl->noauth_url, len);
}

Is far simpler.
Or if you want to go with your approach it should be made generic
in the form of a kind of "sprintf_alloc", without the URL_t argument
but instead e.g. a char * return value - but I think that is overkill,
and difficult to review (and I am still not a fan of the va_* stuff even
though I guess that nowadays you can expect it to work correctly).