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

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

tested, reviewed, looks good to me. This was rebased on top of staging
so we can merge it right away.

regards,
Hugo

Reviewed-by: Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>

On Wed, 2020-07-08 at 17:05 +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 |  9 ++++
>  lib/uknetdev/netdev.c                 | 78
> +++++++++++++++++++++++++++
>  4 files changed, 89 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 dba719fc..d2e8da05 100644
> --- a/lib/uknetdev/include/uk/netdev_core.h
> +++ b/lib/uknetdev/include/uk/netdev_core.h
> @@ -373,6 +373,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.
> @@ -400,6 +406,9 @@ struct uk_netdev {
>       struct
> uk_netdev_tx_queue   *_tx_queue[CONFIG_LIBUKNETDEV_MAXNBQUEUES];
>  
>       UK_TAILQ_ENTRY(struct uk_netdev) _list;
> +
> +     /** Netdevice address configuration */
> +     struct uk_netdev_config    *_config;
>  };
>  
>  #ifdef __cplusplus
> diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
> index 151e0897..e0ee4427 100644
> --- a/lib/uknetdev/netdev.c
> +++ b/lib/uknetdev/netdev.c
> @@ -38,10 +38,62 @@
>  #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)
> +{
> +     /** Remember the reference to the ip address for successive
> calls*/
> +     static char *ip_addr;
> +
> +     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)
> +{
> +     /** Remember the reference to the netmask for successive
> calls*/
> +     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)
> +{
> +     /** Remember the reference to the gateway address for
> successive calls*/
> +     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 +116,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 || ipv4_subnet_mask || ipv4_gw_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 +162,13 @@ int uk_netdev_drv_register(struct uk_netdev *dev,
> struct uk_alloc *a,
>       if (!dev->_data)
>               return -ENOMEM;
>  
> +     /**
> +      * Since the _config is meant to be optional, the boot
> process should
> +      * still continue if the allocation of config fails instead
> of reporting
> +      * error.
> +      */
> +     dev->_config = _alloc_config(a);
> +
>       UK_TAILQ_INSERT_TAIL(&uk_netdev_list, dev, _list);
>       uk_pr_info("Registered netdev%"PRIu16": %p (%s)\n",
>                  netdev_count, dev, drv_name);