Re: [UNIKRAFT PATCH v2 1/2] lib/uknetdev: Add ipaddr, netmask & gwaddr as arg

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

thanks a lot for this patch series!

This patch should be applied on top of "Poll based receive based on
uk_netdev features". It looks good to me, just a few small-ish issues
that should be fixed for the v2 (inline).

regards,
Hugo

On Tue, 2020-06-30 at 12:51 +0200, Sharan Santhanam wrote:
> The patch adds ip address, netmask and the gateway address as library
> argument. The parameters are list of argument delimited by a space.
> The library user can forward the argument using the following command
> line:
> net.ipv4_addr, net.ipv4_subnet_mask, net.ipv4_gw_addr
> 
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> ---
>  lib/uknetdev/Config.uk                |  1 +
>  lib/uknetdev/Makefile.uk              |  1 +
>  lib/uknetdev/include/uk/netdev_core.h |  8 +++
>  lib/uknetdev/netdev.c                 | 70
> +++++++++++++++++++++++++++
>  4 files changed, 80 insertions(+)
> 
> diff --git a/lib/uknetdev/Config.uk b/lib/uknetdev/Config.uk
> index 618e8e67..186dd462 100644
> --- a/lib/uknetdev/Config.uk
> +++ b/lib/uknetdev/Config.uk
> @@ -4,6 +4,7 @@ menuconfig LIBUKNETDEV
>       select LIBNOLIBC if !HAVE_LIBC
>       select LIBUKDEBUG
>       select LIBUKALLOC
> +     imply LIBUKLIBPARAM
>  
>  if LIBUKNETDEV
>       config LIBUKNETDEV_MAXNBQUEUES
> diff --git a/lib/uknetdev/Makefile.uk b/lib/uknetdev/Makefile.uk
> index ca08b254..abdcd4bc 100644
> --- a/lib/uknetdev/Makefile.uk
> +++ b/lib/uknetdev/Makefile.uk
> @@ -1,4 +1,5 @@
>  $(eval $(call addlib_s,libuknetdev,$(CONFIG_LIBUKNETDEV)))
> +$(eval $(call addlib_paramprefix,libuknetdev,net))
>  
>  CINCLUDES-$(CONFIG_LIBUKNETDEV)              +=
> -I$(LIBUKNETDEV_BASE)/include
>  CXXINCLUDES-$(CONFIG_LIBUKNETDEV)    +=
> -I$(LIBUKNETDEV_BASE)/include
> diff --git a/lib/uknetdev/include/uk/netdev_core.h
> b/lib/uknetdev/include/uk/netdev_core.h
> index 954aa8b0..7f157f94 100644
> --- a/lib/uknetdev/include/uk/netdev_core.h
> +++ b/lib/uknetdev/include/uk/netdev_core.h
> @@ -386,6 +386,12 @@ struct uk_netdev_data {
>       const char           *drv_name;
>  };
>  
> +struct uk_netdev_config {
> +     const char *ipv4_addr;
> +     const char *ipv4_net_mask;
> +     const char *ipv4_gw_addr;
> +};
> +
>  /**
>   * NETDEV
>   * A structure used to interact with a network device.
> @@ -413,6 +419,8 @@ struct uk_netdev {
>       struct
> uk_netdev_tx_queue   *_tx_queue[CONFIG_LIBUKNETDEV_MAXNBQUEUES];
>  
>       UK_TAILQ_ENTRY(struct uk_netdev) _list;
> +
> +     struct uk_netdev_config    *_config;
>  #if (CONFIG_UK_NETDEV_SCRATCH_SIZE > 0)
>       char scratch_pad[CONFIG_UK_NETDEV_SCRATCH_SIZE];
>  #endif /* CONFIG_UK_NETDEV_SCRATCH_SIZE */
> diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
> index 151e0897..85429a8b 100644
> --- a/lib/uknetdev/netdev.c
> +++ b/lib/uknetdev/netdev.c
> @@ -38,10 +38,59 @@
>  #include <string.h>
>  #include <uk/netdev.h>
>  #include <uk/print.h>
> +#include <uk/libparam.h>
>  
>  struct uk_netdev_list uk_netdev_list =
>       UK_TAILQ_HEAD_INITIALIZER(uk_netdev_list);
>  static uint16_t netdev_count;
> +/**
> + * TODO: Define Network argument format when multiple driver device
> need to
> + * coexist. For example like:
> + * Driver Name:IP Address:Net Mask
> + */
> +static char *ipv4_addr;
> +static char *ipv4_subnet_mask;
> +static char *ipv4_gw_addr;
> +
> +UK_LIB_PARAM_STR(ipv4_addr);
> +UK_LIB_PARAM_STR(ipv4_subnet_mask);
> +UK_LIB_PARAM_STR(ipv4_gw_addr);
> +
> +static const char *_parse_ipv4_addr(void)
> +{
> +     static char *ip_addr;

If I understand correctly, this is the internal state of strtok_r. I
would have appreciated a small comment here to make this clear.

> +
> +     if (ip_addr)
> +             return strtok_r(NULL, " ", &ip_addr);
> +     else if (ipv4_addr)
> +             return strtok_r(ipv4_addr, " ", &ip_addr);
> +
> +     return NULL;
> +}
> +
> +static const char *_parse_ipv4_net_mask(void)
> +{
> +     static char *net_mask;
> +
> +     if (net_mask)
> +             return strtok_r(NULL, " ", &net_mask);
> +     else if (ipv4_subnet_mask)
> +             return strtok_r(ipv4_subnet_mask, " ", &net_mask);
> +
> +     return NULL;
> +}
> +
> +static const char *_parse_ipv4_gw_addr(void)
> +{
> +     static char *gw;
> +
> +     if (gw)
> +             return strtok_r(NULL, " ", &gw);
> +     else if (ipv4_gw_addr)
> +             return strtok_r(ipv4_gw_addr, " ", &gw);
> +
> +     return NULL;
> +}
>  
>  static struct uk_netdev_data *_alloc_data(struct uk_alloc *a,
>                                         uint16_t netdev_id,
> @@ -64,6 +113,25 @@ static struct uk_netdev_data *_alloc_data(struct
> uk_alloc *a,
>       return data;
>  }
>  
> +static struct uk_netdev_config *_alloc_config(struct uk_alloc *a)
> +{
> +     struct uk_netdev_config *_config = NULL;
> +
> +     if (ipv4_addr) {
> +             _config = uk_zalloc(a, sizeof(*_config));
> +             if (!_config) {
> +                     uk_pr_warn("Failed to allocate memory for
> netdev config\n");
> +                     return NULL;
> +             }
> +
> +             _config->ipv4_addr = _parse_ipv4_addr();
> +             _config->ipv4_net_mask = _parse_ipv4_net_mask();
> +             _config->ipv4_gw_addr = _parse_ipv4_gw_addr();
> +     }
> +
> +     return _config;
> +}
> +
>  int uk_netdev_drv_register(struct uk_netdev *dev, struct uk_alloc
> *a,
>                          const char *drv_name)
>  {
> @@ -91,6 +159,8 @@ int uk_netdev_drv_register(struct uk_netdev *dev,
> struct uk_alloc *a,
>       if (!dev->_data)
>               return -ENOMEM;
>  
> +     dev->_config = _alloc_config(a);
> +

_alloc_config can return NULL if `uk_zalloc` fails. In this case, we
should probably abort with ENOMEM; otherwise we might crash later when
dereferencing `dev->_config`. e.g.,

if (!dev->_config)
        return -ENOMEM;

>       UK_TAILQ_INSERT_TAIL(&uk_netdev_list, dev, _list);
>       uk_pr_info("Registered netdev%"PRIu16": %p (%s)\n",
>                  netdev_count, dev, drv_name);