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

[Simon Glass]

Hi Maxim,

On Tue, 26 Sept 2023 at 03:44, Maxim Uvarov <maxim.uvarov at 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 at 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 at 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 at 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