[PATCH] Fix URL validation for more than one URLs.

[Baptiste Jonglez]

Hi,

On 02-05-21, fabian.baumanis at mailbox.org wrote:
> That's my first contribution for OpenWRT, if something's not right, please
> let me know.

The patch is a good start (comments below), thanks a lot for your first contribution!

There are a few formal issues with your patch.  See
https://openwrt.org/submitting-patches#submission_guidelines for
guidelines, but in short:

- your email subject should start with "[PATCH uclient]" or even "[PATCH
  uclient v2]" for a follow-up patch.  You can just edit the email
  manually before sending it with git-send-email.

- the commit message should start with "uclient-fetch: XXX"

- you're missing the Signed-off-by:

- don't forget to add "Fixes: FS#3559" when you have worked out the other
  issue (a single patch for both reported issues is fine, it's the same
  code that needs fixing)

> Until now, uclient-fetch only validates the first URL. If the second URL
> is invalid, it uses the first URL again.
> This patch catches the return value -1 of uclient_set_url and provides the
> user with an error message.

This should be part of the commit message description, and focus more on
the "why" than the "how".

> diff --git a/uclient-fetch.c b/uclient-fetch.c
> index 282092e..d046100 100644
> --- a/uclient-fetch.c
> +++ b/uclient-fetch.c
> @@ -384,14 +384,23 @@ static void request_done(struct uclient *cl)
>  	if (n_urls) {
>  		proxy_url = get_proxy_url(*urls);
>  		if (proxy_url) {
> -			uclient_set_url(cl, proxy_url, NULL);
> +			error_ret = uclient_set_url(cl, proxy_url, NULL);
>  			uclient_set_proxy_url(cl, *urls, auth_str);
>  		} else {
> -			uclient_set_url(cl, *urls, auth_str);
> +			error_ret = uclient_set_url(cl, *urls, auth_str);
>  		}

As far as I understand it, error_ret is meant as the global exit code of
the program, and it aims to be somewhat compatible with GNU wget (hence
the hard-coded values 4 and 8 throughout the code, see the end of "man
wget" for the codes used by GNU wget).

So, you should use a separate variable for error checking, and then set
error_ret to the appropriate value if the error needs to be communicated
through the exit code.  I agree that the same function already abuses
error_ret but it seems wrong.

There's the question of what the exit code should be when one of the URL
is incorrect but the other ones are OK.  I think we should again have the
same behaviour as wget for compatibility.  Also, if one URL is invalid, we
should probably continue with the next one (if wget does that).

Thanks,
Baptiste
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.openwrt.org/pipermail/openwrt-devel/attachments/20210503/2e09a48a/attachment.sig>