On Sat, 2 Sept 2023 at 06:09, Simon Glass sjg@google.com wrote:
Hi Maxim,
On Fri, 1 Sept 2023 at 08:49, Maxim Uvarov maxim.uvarov@linaro.org wrote:
On Wed, 23 Aug 2023 at 00:58, Simon Glass sjg@google.com wrote:
Hi Maxim,
On Tue, 22 Aug 2023 at 03:39, Maxim Uvarov maxim.uvarov@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@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@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.