Re: [UNIKRAFT PATCH v2 2/2] lib/uknetdev: Get the ipaddr, gw_addr and subnet

[Hugo Lefeuvre <hugo.lefeuvre@xxxxxxxxx>]

Hi Sharan,

this patch looks good to me. Just a few minor comments inline.

regards,
Hugo

On Tue, 2020-06-30 at 12:51 +0200, Sharan Santhanam wrote:
> The user of uknetdev can pass the ipaddr, gw_addr and subnet as a
> library argument. The library argument can be retrieved from the
> network stack using uk_netdev_einfo_get function. The library
> arguments are retrieved only if the driver does not support the
> uk_netdev_einfo_get callback.
> 
> Signed-off-by: Sharan Santhanam <sharan.santhanam@xxxxxxxxx>
> ---
>  lib/uknetdev/netdev.c | 21 +++++++++++++++++++++
>  1 file changed, 21 insertions(+)
> 
> diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
> index 85429a8b..8fc7444e 100644
> --- a/lib/uknetdev/netdev.c
> +++ b/lib/uknetdev/netdev.c
> @@ -231,6 +231,25 @@ void uk_netdev_info_get(struct uk_netdev *dev,
>                                     dev_info->max_tx_queues);
>  }
>  
> +static const void *_netdev_einfo_get(struct uk_netdev *dev,
> +                             enum uk_netdev_einfo_type einfo)
> +{
> +     switch (einfo) {
> +     case UK_NETDEV_IPV4_ADDR_STR:
> +             uk_pr_debug("ip_addr: %s\n", dev->_config-
> >ipv4_addr);

AFAIK this is undefined behavior if `dev->_config->ipv4_addr` is a null
pointer. Sensible libc implementations will handle this correctly, but
that's implementation specific.

> +             return dev->_config->ipv4_addr;
> +     case UK_NETDEV_IPV4_MASK_STR:
> +             uk_pr_debug("netmask: %s\n", dev->_config-
> >ipv4_net_mask);

same here.

> +             return dev->_config->ipv4_net_mask;
> +     case UK_NETDEV_IPV4_GW_STR:
> +             uk_pr_debug("Gateway: %s\n", dev->_config-
> >ipv4_gw_addr);

same here.

> +             return dev->_config->ipv4_gw_addr;
> +     default:
> +             uk_pr_warn("Option %d not yet supported\n", einfo);
> +     }
> +     return NULL;
> +}
> +
>  const void *uk_netdev_einfo_get(struct uk_netdev *dev,
>                               enum uk_netdev_einfo_type einfo)
>  {
> @@ -239,6 +258,8 @@ const void *uk_netdev_einfo_get(struct uk_netdev
> *dev,
>  
>       if (!dev->ops->einfo_get) {
>               /* driver does not provide any extra configuration
> */

this comment only applies to `return NULL`, right? If so, I'd move it
after `return _netdev_einfo_get(dev, einfo)` to make this clear.

> +             if (dev->_config)
> +                     return _netdev_einfo_get(dev, einfo);
>               return NULL;
>       }
>       return dev->ops->einfo_get(dev, einfo);