On Sat, 2 Sept 2023 at 06:09, Simon Glass <s...@google.com> wrote:
> Hi Maxim,
>
> On Fri, 1 Sept 2023 at 08:49, Maxim Uvarov <maxim.uva...@linaro.org>
> wrote:
> >
> >
> >
> > 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.
>
> 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.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));
>
> That can fail
>
>
yea, will add a check.
> >> > +
> >> > + 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?
>
>
I will add more checks. dns_gethostbyname() will return an error if dns
were set wrongly. But I will also add more checks here to be clear for
other people also.
> >
> >
> >>
> >> > +
> >> > + 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
>
Yes, that is exactly what happens here. Lwip ERR_INPROGRESS(-5) might be
U-boot:
#define EINPROGRESS 115 /* Operation now in progress */
I can do a conversion here with some comments to make it more clear.
