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

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 11.10.18 17:14, Sharan Santhanam wrote:
> 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.
Yes, sorry for this. I will update it.

> > + *   - (-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.
You are right. The idea was to have this as a reliable interface that 
never fails. We thought that the MTU information should always be there, 
otherwise we are doing something seriously wrong. I am changing the 
return type of uk_netdev_mtu_get() to uint16_t, too.
> > +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
Ooops,  you are right.

> > +    /* 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