Re: [Minios-devel] [UNIKRAFT PATCH v4 10/11] lib/uknetdev: MTU interfaces

[Sharan Santhanam <sharan.santhanam@xxxxxxxxx>]

Hello Simon,

Please find the comments inline:

On 10/10/2018 02:00 PM, Simon Kuenzer wrote:
> From: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
>
> Add interfaces to query and set the maximum transmission unit (MTU)
> for a netdev.
>
> Signed-off-by: Simon Kuenzer <simon.kuenzer@xxxxxxxxx>
> Signed-off-by: Razvan Cojocaru <razvan.cojocaru93@xxxxxxxxx>
> ---
>   lib/uknetdev/exportsyms.uk            |  2 ++
>   lib/uknetdev/include/uk/netdev.h      | 27 +++++++++++++++++++++++++++
>   lib/uknetdev/include/uk/netdev_core.h | 10 ++++++++++
>   lib/uknetdev/netdev.c                 | 35 +++++++++++++++++++++++++++++++++++
>   4 files changed, 74 insertions(+)
>
> diff --git a/lib/uknetdev/exportsyms.uk b/lib/uknetdev/exportsyms.uk
> index 6e8b06b..9e264bd 100644
> --- a/lib/uknetdev/exportsyms.uk
> +++ b/lib/uknetdev/exportsyms.uk
> @@ -26,3 +26,5 @@ uk_netdev_hwaddr_set
>   uk_netdev_hwaddr_get
>   uk_netdev_promiscuous_get
>   uk_netdev_promiscuous_set
> +uk_netdev_mtu_get
> +uk_netdev_mtu_set
> diff --git a/lib/uknetdev/include/uk/netdev.h b/lib/uknetdev/include/uk/netdev.h
> index b06978a..e527534 100644
> --- a/lib/uknetdev/include/uk/netdev.h
> +++ b/lib/uknetdev/include/uk/netdev.h
> @@ -335,6 +335,33 @@ int uk_netdev_promiscuous_get(struct uk_netdev *dev);
>    */
>   int uk_netdev_promiscuous_set(struct uk_netdev *dev, int mode);
>   
> +/**
> + * Returns the MTU of an Unikraft network device.
> + *
> + * @param dev
> + *   The Unikraft Network Device.
> + * @return
> + *   - (>0): MTU of the uk_netdev
The description does not match the implementation.
> + *   - (-ENOTSUP): driver did not set a MTU.
> + */
> +int uk_netdev_mtu_get(struct uk_netdev *dev);
> +
> +/**
> + * Change the MTU of an Unikraft network device.
> + *
> + * @param dev
> + *   The Unikraft Network Device.
> + * @param mtu
> + *   A uint16_t for the MTU to be applied.
> + * @return
> + *   - (0): if successful.
The description of the API does not match with the implementation.
> + *   - (-ENOTSUP): if operation is not supported.
> + *   - (-EIO): if device is removed.
> + *   - (-EINVAL): if *mtu* invalid.
> + *   - (-EBUSY): if operation is not allowed when the device is running
> + */
> +int uk_netdev_mtu_set(struct uk_netdev *dev, uint16_t mtu);
> +
>   #ifdef __cplusplus
>   }
>   #endif
> diff --git a/lib/uknetdev/include/uk/netdev_core.h 
> b/lib/uknetdev/include/uk/netdev_core.h
> index 0171127..f78f992 100644
> --- a/lib/uknetdev/include/uk/netdev_core.h
> +++ b/lib/uknetdev/include/uk/netdev_core.h
> @@ -247,6 +247,12 @@ typedef int (*uk_netdev_promiscuous_get_t)(struct 
> uk_netdev *dev);
>   /** Driver callback type to enable or disable promiscuous mode */
>   typedef int (*uk_netdev_promiscuous_set_t)(struct uk_netdev *dev, int mode);
>   
> +/** Driver callback type to get the MTU. */
> +typedef uint16_t (*uk_netdev_mtu_get_t)(struct uk_netdev *dev);
> +
> +/** Driver callback type to set the MTU */
> +typedef int (*uk_netdev_mtu_set_t)(struct uk_netdev *dev, uint16_t mtu);
> +
>   /**
>    * A structure containing the functions exported by a driver.
>    */
> @@ -255,6 +261,10 @@ struct uk_netdev_ops {
>         uk_netdev_hwaddr_get_t          hwaddr_get;       /* recommended */
>         uk_netdev_hwaddr_set_t          hwaddr_set;       /* optional */
>   
> +	/** Set/Get MTU. */
> +       uk_netdev_mtu_get_t             mtu_get;
> +       uk_netdev_mtu_set_t             mtu_set;          /* optional */
> +
>         /** Promiscuous mode. */
>         uk_netdev_promiscuous_set_t     promiscuous_set;  /* optional */
>         uk_netdev_promiscuous_get_t     promiscuous_get;
> diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
> index 0af63ac..6277354 100644
> --- a/lib/uknetdev/netdev.c
> +++ b/lib/uknetdev/netdev.c
> @@ -79,6 +79,7 @@ int uk_netdev_drv_register(struct uk_netdev *dev, struct 
> uk_alloc *a,
>         UK_ASSERT(dev->ops->txq_configure);
>         UK_ASSERT(dev->ops->start);
>         UK_ASSERT(dev->ops->promiscuous_get);
> +       UK_ASSERT(dev->ops->mtu_get);
>   
>   	dev->_data = _alloc_data(a, netdev_count,  drv_name);
>         if (!dev->_data)
> @@ -484,3 +485,37 @@ int uk_netdev_promiscuous_set(struct uk_netdev *dev, int 
> mode)
>   
>   	return dev->ops->promiscuous_set(dev, mode ? 1 : 0);
>   }
> +
There is noway to report any error on the getting the mtu, as the driver 
returns an uint16_t data type.
> +int uk_netdev_mtu_get(struct uk_netdev *dev)
> +{
> +       UK_ASSERT(dev);
> +       UK_ASSERT(dev->_data);
> +       UK_ASSERT(dev->ops);
> +       UK_ASSERT(dev->ops->mtu_get);
> +
s/setting/getting
> +       /* We do support setting of MTU
> +        * only when device was configured
> +        */
> +       UK_ASSERT(dev->_data->state == UK_NETDEV_CONFIGURED
> +                 || dev->_data->state == UK_NETDEV_RUNNING);
> +
> +       return dev->ops->mtu_get(dev);
> +}
> +
> +int uk_netdev_mtu_set(struct uk_netdev *dev, uint16_t mtu)
> +{
> +       UK_ASSERT(dev);
> +       UK_ASSERT(dev->_data);
> +       UK_ASSERT(dev->ops);
s/retrieving/setting
> +
> +       /* We do support retrieving of MTU
> +        * only when device was configured
> +        */
> +       UK_ASSERT(dev->_data->state == UK_NETDEV_CONFIGURED
> +                 || dev->_data->state == UK_NETDEV_RUNNING);
> +
> +       if (dev->ops->mtu_set == NULL)
> +               return -ENOTSUP;
> +
> +       return dev->ops->mtu_set(dev, mtu);
> +}
Thanks & Regards
Sharan

_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel