[PATCHv7 03/15] net/lwip: implement dns cmd

[Simon Glass]

Hi Maxim,

On Tue, 22 Aug 2023 at 03:39, Maxim Uvarov <maxim.uvarov at linaro.org> wrote:
>
> U-Boot recently got support for an alternative network stack using LWIP.
> Replace dns command with the LWIP variant while keeping the output and
> error messages identical.
>
> Signed-off-by: Maxim Uvarov <maxim.uvarov at linaro.org>
> ---
>  include/net/lwip.h           | 19 +++++++++++++++
>  net/lwip/Makefile            |  2 ++
>  net/lwip/apps/dns/lwip-dns.c | 46 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 67 insertions(+)
>  create mode 100644 include/net/lwip.h
>  create mode 100644 net/lwip/apps/dns/lwip-dns.c
>
> diff --git a/include/net/lwip.h b/include/net/lwip.h
> new file mode 100644
> index 0000000000..cda896d062
> --- /dev/null
> +++ b/include/net/lwip.h
> @@ -0,0 +1,19 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +
> +int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int argc,
> +               char *const argv[]);

Please make sure you comment all exported functions, including the return value.

> +
> +/**
> + * ulwip_dns() - creates the DNS request to resolve a domain host name
> + *
> + * This function creates the DNS request to resolve a domain host name. Function
> + * can return immediately if previous request was cached or it might require
> + * entering the polling loop for a request to a remote server.
> + *
> + * @name:    dns name to resolve
> + * @varname: (optional) U-Boot variable name to store the result
> + * Returns: ERR_OK(0) for fetching entry from the cache
> + *         ERR_INPROGRESS(-5) success, can go to the polling loop
> + *         Other value < 0, if error
> + */
> +int ulwip_dns(char *name, char *varname);
> diff --git a/net/lwip/Makefile b/net/lwip/Makefile
> index d1161bea78..6d2c00605b 100644
> --- a/net/lwip/Makefile
> +++ b/net/lwip/Makefile
> @@ -64,3 +64,5 @@ obj-$(CONFIG_NET) += $(LWIPDIR)/netif/ethernet.o
>
>  obj-$(CONFIG_NET) += port/if.o
>  obj-$(CONFIG_NET) += port/sys-arch.o
> +
> +obj-$(CONFIG_CMD_DNS) += apps/dns/lwip-dns.o
> diff --git a/net/lwip/apps/dns/lwip-dns.c b/net/lwip/apps/dns/lwip-dns.c
> new file mode 100644
> index 0000000000..6e205c1153
> --- /dev/null
> +++ b/net/lwip/apps/dns/lwip-dns.c
> @@ -0,0 +1,46 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +/*
> + * (C) Copyright 2023 Linaro Ltd. <maxim.uvarov at linaro.org>
> + */
> +
> +#include <common.h>
> +#include <command.h>
> +#include <console.h>
> +
> +#include <lwip/dns.h>
> +#include <lwip/ip_addr.h>
> +
> +#include <net/ulwip.h>
> +
> +static void dns_found_cb(const char *name, const ip_addr_t *ipaddr, void *callback_arg)
> +{
> +       char *varname = (char *)callback_arg;
> +
> +       if (varname)
> +               env_set(varname, ip4addr_ntoa(ipaddr));
> +
> +       log_info("resolved %s to %s\n",  name, ip4addr_ntoa(ipaddr));
> +       ulwip_exit(0);
> +}
> +
> +int ulwip_dns(char *name, char *varname)
> +{
> +       int err;
> +       ip_addr_t ipaddr; /* not used */
> +       ip_addr_t dns1;
> +       ip_addr_t dns2;
> +
> +       ipaddr_aton(env_get("dnsip"), &dns1);
> +       ipaddr_aton(env_get("dnsip2"), &dns2);

What if the env_get() fails? Won't that return NULL?

> +
> +       dns_init();
> +       dns_setserver(0, &dns1);
> +       dns_setserver(1, &dns2);

Can either of these fail?

Please be very attentive to error-checking.

> +
> +       err = dns_gethostbyname(name, &ipaddr, dns_found_cb, varname);
> +       if (err == ERR_OK)

Is that 0? If so, then if (err) is better

> +               dns_found_cb(name, &ipaddr, varname);
> +
> +       return err;

I am not sure what that is, but will review it when you add the header comments.

> +}
> --
> 2.30.2
>

Regards,
Simon