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

[Simon Glass]

Hi Maxim,

On Fri, 1 Sept 2023 at 08:49, Maxim Uvarov <maxim.uvarov at linaro.org> wrote:
>
>
>
> On Wed, 23 Aug 2023 at 00:58, Simon Glass <sjg at google.com> wrote:
>>
>> 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
>> > + */
>
>
> here.

That seems to be a different function?

>
>>
>> > +int ulwip_dns(char *name, char *varname);

This one has no comment?

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

That can fail

>> > +
>> > +       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?
>>
>
> all of these functions will not crash, they have null check. You can set dnsip or dnsip2 or both. If both are not set then dns cmd will not look up anything.
> We can exit earlier here, but that is a common case and nothing bad if we make code simpler and exit a little bit later.

OK but I cannot see where the error is returned?

>
>
>>
>> > +
>> > +       dns_init();
>> > +       dns_setserver(0, &dns1);
>> > +       dns_setserver(1, &dns2);
>>
>> Can either of these fail?
>>
>> Please be very attentive to error-checking.
>
>
> All  above functions are void. But they have LWIP_ASSERT() for sanity checks in some places.
>
>>
>>
>> > +
>> > +       err = dns_gethostbyname(name, &ipaddr, dns_found_cb, varname);
>> > +       if (err == ERR_OK)
>>
>> Is that 0? If so, then if (err) is better
>>
>
> dns_gethostbyname() returns ERR_OK which is defined net/lwip/lwip-external/src/include/lwip/err.h.
> Yes it's defined to 0 and maybe always will be defined to 0. But  that may change. And I would keep
> the check against the same return macro that the function does.

I cannot imagine it changing.

>
>
>>
>> > +               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.
>>
> In the header file of this function is an explanation. It's above in this patch:
> + * 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
> + */

OK, so what I am getting at is trying to understand the error
conversion. It seems that lwip uses different numbering from
U-Boot/Linux, so we need to do a conversion somewhere?

Regards,
Simon