On Wed, 23 Aug 2023 at 00:58, Simon Glass <s...@google.com> wrote:
> Hi Maxim,
>
> On Tue, 22 Aug 2023 at 03:39, Maxim Uvarov <maxim.uva...@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.uva...@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.
> > +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.uva...@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?
>
>
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.
> > +
> > + 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.
> > + 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
+ */
> > +}
> > --
> > 2.30.2
> >
>
> Regards,
> Simon
>
