Re: [PATCH v4 01/14] net: wget: let wget_with_dns work with dns disabled

Heinrich Schuchardt
2024-11-19T09:27:44Z

On 18.11.24 22:08, Adriano Cordova wrote:
> This was marked as TODO in the code:
> -Enable use of wget_with_dns even if CMD_DNS is disabled if
> the given uri has the ip address for the http server.
> -Check for port in the uri when transforming to legacy wget
> syntax inside wget_with_dns.
> -Move the check for CMD_DNS inside wget_with_dns.
>
> Signed-off-by: Adriano Cordova <adrianox@gmail.com>
> ---

Changes in v4 are not described:

* drop console output
* avoid checking non-zero via '!= 0'.

>
> v3:
> Removed console output inside efi routine.
>
> (no changes since v2)
> net/wget.c | 36 +++++++++++++++++++++---------------
> 1 file changed, 21 insertions(+), 15 deletions(-)
>
> diff --git a/net/wget.c b/net/wget.c
> index 3bc2522cde..1166aa5aa6 100644
> --- a/net/wget.c
> +++ b/net/wget.c
> @@ -530,12 +530,10 @@ void wget_start(void)
> wget_send(TCP_SYN, 0, 0, 0);
> }
>
> -#if (IS_ENABLED(CONFIG_CMD_DNS))
> int wget_with_dns(ulong dst_addr, char *uri)
> {
> int ret;
> - char *s, *host_name, *file_name, *str_copy;
> -
> + char *s, *host_name, *file_name, *str_copy, *port;

nits:
Missing empty line.

> /*
> * Download file using wget.
> *
> @@ -550,24 +548,33 @@ int wget_with_dns(ulong dst_addr, char *uri)
> s = str_copy + strlen("http://");
> host_name = strsep(&s, "/");
> if (!s) {
> - log_err("Error: invalied uri, no file path\n");
> ret = -EINVAL;
> goto out;
> }
> file_name = s;
>
> - /* TODO: If the given uri has ip address for the http server, skip dns */
> - net_dns_resolve = host_name;
> - net_dns_env_var = "httpserverip";
> - if (net_loop(DNS) < 0) {
> - log_err("Error: dns lookup of %s failed, check setup\n", net_dns_resolve);
> - ret = -EINVAL;
> - goto out;
> - }
> - s = env_get("httpserverip");
> - if (!s) {
> + port = host_name;
> + host_name = strsep(&port, ":");

RFC 1738 has:

login = [ user [ ":" password ] "@" ] hostport
hostport = host [ ":" port ]

So here variable port may point to the password. And host_name to the
user name. We might check for '@' and bail out with an error code if it
occurs.

You are determining the host port as a string, but I cannot see where
you are ever using it.

In wget_send_stored() I see this line:

server_port = env_get_ulong("httpdstp", 10, SERVER_PORT) & 0xffff;

Maybe make server_port a static variable and set it here if variable
port does not contain a number.

Best regards

Heinrich

> +
> + if (string_to_ip(host_name).s_addr) {
> + s = host_name;
> + } else {
> +#if IS_ENABLED(CONFIG_CMD_DNS)
> + net_dns_resolve = host_name;
> + net_dns_env_var = "httpserverip";
> + if (net_loop(DNS) < 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> + s = env_get("httpserverip");
> + if (!s) {
> + ret = -EINVAL;
> + goto out;
> + }
> +#else
> ret = -EINVAL;
> goto out;
> +#endif
> }
>
> strlcpy(net_boot_file_name, s, sizeof(net_boot_file_name));
> @@ -581,7 +588,6 @@ out:
>
> return ret < 0 ? ret : 0;
> }
> -#endif
>
> /**
> * wget_validate_uri() - validate the uri for wget