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

Baptiste Jonglez baptiste at bitsofnetworks.org
Mon May 3 10:30:16 BST 2021


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

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

More information about the openwrt-devel mailing list