Re: [Minios-devel] [UNIKRAFT/LWIP PATCH 1/3] Try to get IP address from device

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

Hey Costin,

thanks for this patch. Please see my comments inline. I think we can 
earlier integrate this patch of the series to lwip.
Thanks,

Simon

On 01.04.19 15:41, Costin Lupu wrote:
> IP addresses for netfront devices on Xen may be configured using
> Xenstore. Therefore we should first try to get IP address from device
> before taking it from elsewhere.
>
> Signed-off-by: Costin Lupu <costin.lupu@xxxxxxxxx>
> ---
>   init.c | 47 +++++++++++++++++++++++++++--------------------
>   1 file changed, 27 insertions(+), 20 deletions(-)
>
> diff --git a/init.c b/init.c
> index a5854b8..58fb173 100644
> --- a/init.c
> +++ b/init.c
> @@ -38,11 +38,13 @@
>   #include "lwip/tcpip.h"
>   #include "lwip/init.h"
>   #include "lwip/dhcp.h"
> +#include "lwip/inet.h"
>   #if CONFIG_LWIP_NOTHREADS
>   #include "lwip/timeouts.h"
>   #else /* CONFIG_LWIP_NOTHREADS */
>   #include <uk/semaphore.h>
>   #endif /* CONFIG_LWIP_NOTHREADS */
> +#include <uk/netdev_core.h>
>   #include "netif/uknetdev.h"
>   
>   #if LWIP_NETIF_EXT_STATUS_CALLBACK && CONFIG_LWIP_NETIF_STATUS_PRINT
> @@ -138,11 +140,11 @@ int liblwip_init(void)
>         uint16_t  __maybe_unused int16cfg;
>         int is_first_nf;
>   #if LWIP_IPV4
> -       ip4_addr_t __maybe_unused ip4;
> +       ip4_addr_t ip4;
>         ip4_addr_t *ip4_arg;
> -       ip4_addr_t __maybe_unused mask4;
> +       ip4_addr_t mask4;
>         ip4_addr_t *mask4_arg;
> -       ip4_addr_t __maybe_unused gw4;
> +       ip4_addr_t gw4;
>         ip4_addr_t *gw4_arg;
>   #endif /* LWIP_IPV4 */
>   #endif /* CONFIG_LWIP_UKNETDEV && CONFIG_LWIP_AUTOIFACE */
> @@ -186,23 +188,28 @@ int liblwip_init(void)
>                 mask4_arg = NULL;
>                 gw4_arg   = NULL;
>   
> -		/*
> -                * TODO: Try to get device configuration from
> -                * netdev's econf interface:
> -                *
> -                * UK_NETDEV_IPV4_ADDR_NINT16;
> -                * UK_NETDEV_IPV4_ADDR_STR;
> -                * UK_NETDEV_IPV4_MASK_NINT16;
> -                * UK_NETDEV_IPV4_MASK_STR;
> -                * UK_NETDEV_IPV4_GW_NINT16;
> -                * UK_NETDEV_IPV4_GW_STR;
> -                *
> -                * When successfully done, set
> -                *  ip_arg = &ip;
> -                *  mask_arg = &mask;
> -                *  gw_arg = &gw;
> -                */
> -
> +               strcfg = uk_netdev_einfo_get(dev, UK_NETDEV_IPV4_ADDR_STR);
> +               if (!strcfg)
> +                       goto no_conf;
> +               if (ip4addr_aton(strcfg, &ip4) != 1)
> +                       goto no_conf;
> +
> +               strcfg = uk_netdev_einfo_get(dev, UK_NETDEV_IPV4_MASK_STR);
> +               if (!strcfg)
> +                       goto no_conf;
> +               if (ip4addr_aton(strcfg, &mask4) != 1)
> +                       goto no_conf;
I would go for a default mask (Class C for instance), if we don't got 
one but have an IP. Of course we should fail if we could not parse the 
mask but the absence of one I would go for a default one.
> +
> +               strcfg = uk_netdev_einfo_get(dev, UK_NETDEV_IPV4_GW_STR);
> +               if (!strcfg)
> +                       goto no_conf;
> +               if (ip4addr_aton(strcfg, &gw4) != 1)
> +                       goto no_conf;
> +
Not having a gateway is a reasonable configuration. We just set it to 
NULL and lwip should handle it properly. We should still setup the given 
IP and gateway instead of considering this as no_conf.
> +               ip4_arg = &ip4;
> +               mask4_arg = &mask4;
> +               gw4_arg = &gw4;
> +no_conf:
>                 nf = uknetdev_addif(dev, ip4_arg, mask4_arg, gw4_arg);
>   #else /* LWIP_IPV4 */
>                 /*
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel