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
>> > +
>> > + 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
