Re: [PATCHv10 06/15] net/lwip: implement dhcp cmd

Simon Glass
2023-10-02T01:38:40Z

Hi Maxim,

On Tue, 26 Sept 2023 at 03:44, Maxim Uvarov <maxim.uvarov@linaro.org> wrote:
>
> U-Boot recently got support for an alternative network stack using LWIP.
> Replace dhcp 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 | 12 +++++
> net/lwip/Makefile | 1 +
> net/lwip/apps/dhcp/lwip-dhcp.c | 85 ++++++++++++++++++++++++++++++++++
> 3 files changed, 98 insertions(+)
> create mode 100644 net/lwip/apps/dhcp/lwip-dhcp.c

Reviewed-by: Simon Glass <sjg@chromium.org>
with nits

>
> diff --git a/include/net/lwip.h b/include/net/lwip.h
> index ab3db1a214..6a8fcef392 100644
> --- a/include/net/lwip.h
> +++ b/include/net/lwip.h
> @@ -17,3 +17,15 @@ int do_lwip_dns(struct cmd_tbl *cmdtp, int flag, int argc,
> * Other value < 0, if error
> */
> int ulwip_dns(char *name, char *varname);
> +
> +/**
> + * ulwip_dhcp() - create the DHCP request to obtain IP address.
> + *
> + * This function creates the DHCP request to obtain IP address. If DHCP server
> + * returns file name, this file will be downloaded with tftp. After this
> + * function you need to invoke the polling loop to process network communication.
> + *
> + * Returns: 0 if success
> + * Other value < 0, if error
> +*/
> +int ulwip_dhcp(void);
> diff --git a/net/lwip/Makefile b/net/lwip/Makefile
> index 5d8d5527c6..a3a33b7f71 100644
> --- a/net/lwip/Makefile
> +++ b/net/lwip/Makefile
> @@ -63,4 +63,5 @@ obj-$(CONFIG_NET) += lwip-external/src/netif/ethernet.o
> obj-$(CONFIG_NET) += port/if.o
> obj-$(CONFIG_NET) += port/sys-arch.o
>
> +obj-y += apps/dhcp/lwip-dhcp.o
> obj-y += apps/dns/lwip-dns.o
> diff --git a/net/lwip/apps/dhcp/lwip-dhcp.c b/net/lwip/apps/dhcp/lwip-dhcp.c
> new file mode 100644
> index 0000000000..f0b0e26f6e
> --- /dev/null
> +++ b/net/lwip/apps/dhcp/lwip-dhcp.c
> @@ -0,0 +1,85 @@
> +// 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/dhcp.h>
> +#include <lwip/prot/dhcp.h>
> +#include "lwip/timeouts.h"
> +
> +#include <net/eth.h>
> +#include <net/ulwip.h>
> +
> +#define DHCP_TMO_TIME 500 /* poll for DHCP state change */
> +#define DHCP_TMO_NUM 10 /* number of tries */
> +
> +typedef struct dhcp_priv {

can you drop typedef?

> + int num_tries;
> + struct netif *netif;
> +} dhcp_priv;
> +
> +static void dhcp_tmo(void *arg)
> +{
> + struct dhcp_priv *dpriv = (struct dhcp_priv *)arg;
> + struct netif *netif = dpriv->netif;
> + struct dhcp *dhcp;
> +
> + dhcp = netif_get_client_data(netif, LWIP_NETIF_CLIENT_DATA_INDEX_DHCP);
> + if (!dhcp)
> + return;
> +
> + if (dhcp->state == DHCP_STATE_BOUND) {
> + int err = 0;
> +
> + err -= env_set("bootfile", dhcp->boot_file_name);
> + err -= env_set("ipaddr", ip4addr_ntoa(&dhcp->offered_ip_addr));
> + err -= env_set("netmask", ip4addr_ntoa(&dhcp->offered_sn_mask));
> + err -= env_set("serverip", ip4addr_ntoa(&dhcp->server_ip_addr));
> + if (err)
> + log_err("error update envs\n");

but don't you need to return the effort? How does a script know it failed?

I think this function need an error return...if that is not possible
then you need to stash the error somehow so you can report it when
needed.

> + log_info("DHCP client bound to address %s\n", ip4addr_ntoa(&dhcp->offered_ip_addr));
> + free(dpriv);
> + ulwip_exit(err);
> + return;
> + }
> +
> + dpriv->num_tries--;
> + if (dpriv->num_tries < 0) {
> + log_err("DHCP client timeout\n");
> + free(dpriv);
> + ulwip_exit(-1);
> + return;
> + }
> +
> + sys_timeout(DHCP_TMO_TIME, dhcp_tmo, dpriv);
> +}
> +
> +int ulwip_dhcp(void)
> +{
> + struct netif *netif;
> + int eth_idx;
> + struct dhcp_priv *dpriv;
> +
> + dpriv = malloc(sizeof(struct dhcp_priv));
> + if (!dpriv)
> + return -EPERM;

-ENOMEM

> +
> + eth_idx = eth_get_dev_index();
> + if (eth_idx < 0)
> + return -EPERM;
> +
> + netif = netif_get_by_index(eth_idx + 1);
> + if (!netif)
> + return -ENOENT;
> +
> + dpriv->num_tries = DHCP_TMO_NUM;
> + dpriv->netif = netif;
> + sys_timeout(DHCP_TMO_TIME, dhcp_tmo, dpriv);
> +
> + return dhcp_start(netif) ? 0 : -EPERM;

Why -EPERM? Is it a permission error? Can we not get a real error number?

> +}
> --
> 2.30.2
>

Regards,
SImon