Re: [Minios-devel] [UNIKRAFT PATCH v4 09/11] lib/uknetdev: Promiscuous mode interfaces

[Simon Kuenzer <simon.kuenzer@xxxxxxxxx>]

On 19.10.18 00:56, Simon Kuenzer wrote:
> On 11.10.18 16:58, 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, enable, and disable promiscuous mode 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      | 26 ++++++++++++++++++++++++++
> > >   lib/uknetdev/include/uk/netdev_core.h |  9 +++++++++
> > >   lib/uknetdev/netdev.c                 | 35 
> > > +++++++++++++++++++++++++++++++++++
> > >   4 files changed, 72 insertions(+)
> > >
> > > diff --git a/lib/uknetdev/exportsyms.uk b/lib/uknetdev/exportsyms.uk
> > > index 8b13499..6e8b06b 100644
> > > --- a/lib/uknetdev/exportsyms.uk
> > > +++ b/lib/uknetdev/exportsyms.uk
> > > @@ -24,3 +24,5 @@ uk_netdev_txq_configure
> > >   uk_netdev_start
> > >   uk_netdev_hwaddr_set
> > >   uk_netdev_hwaddr_get
> > > +uk_netdev_promiscuous_get
> > > +uk_netdev_promiscuous_set
> > > diff --git a/lib/uknetdev/include/uk/netdev.h 
> > > b/lib/uknetdev/include/uk/netdev.h
> > > index 423e321..b06978a 100644
> > > --- a/lib/uknetdev/include/uk/netdev.h
> > > +++ b/lib/uknetdev/include/uk/netdev.h
> > > @@ -309,6 +309,32 @@ const struct uk_hwaddr 
> > > *uk_netdev_hwaddr_get(struct uk_netdev *dev);
> > >    */
> > >   int uk_netdev_hwaddr_set(struct uk_netdev *dev, const struct 
> > > uk_hwaddr *hwaddr);
> > > +/**
> > > + * Returns if promiscuous mode is enabled for an Unikraft network 
> > > device.
> > > + *
> > > + * @param dev
> > > + *   The Unikraft Network Device.
> > > + * @return
> > > + *   - (1): if promiscuous is enabled
> > > + *   - (0): if promiscuous is disabled.
> > We should rephrase it as < 0 for error, as it depend on the device driver
> Yes
Oh sorry, it is a bit late now ;-). Getting the promiscuous mode was 
intended as an interface that is "error-safe". So, I would change the 
return type to "unsigned".
> > > + *   - (-1): on error
> > > + */
> > > +int uk_netdev_promiscuous_get(struct uk_netdev *dev);
> > > +
> > > +/**
> > > + * Enables or disables promiscuous mode for an Unikraft network device.
> > > + *
> > > + * @param dev
> > > + *   The Unikraft Network Device.
> > > + * @param mode
> > > + *   - (0): disable promiscuous mode
> > > + *   - (1): enable promiscuous mode
> > > + * @return
> > > + *   - (0): if successful.
> > > + *   - (-ENOTSUP): if driver doesn't support promiscuous mode.
> > > + */
> > > +int uk_netdev_promiscuous_set(struct uk_netdev *dev, int mode);
> > > +
> > >   #ifdef __cplusplus
> > >   }
> > >   #endif
> > > diff --git a/lib/uknetdev/include/uk/netdev_core.h 
> > > b/lib/uknetdev/include/uk/netdev_core.h
> > > index cca6d13..0171127 100644
> > > --- a/lib/uknetdev/include/uk/netdev_core.h
> > > +++ b/lib/uknetdev/include/uk/netdev_core.h
> > > @@ -241,6 +241,11 @@ typedef const struct uk_hwaddr 
> > > *(*uk_netdev_hwaddr_get_t)(
> > >   typedef int (*uk_netdev_hwaddr_set_t)(struct uk_netdev *dev,
> > >                         const struct uk_hwaddr *hwaddr);
> > > +/** Driver callback type to get the current promiscuous mode */
> > > +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);
> > >   /**
> > >    * A structure containing the functions exported by a driver.
> > > @@ -250,6 +255,10 @@ struct uk_netdev_ops {
> > >       uk_netdev_hwaddr_get_t          hwaddr_get;       /* 
> > > recommended */
> > >       uk_netdev_hwaddr_set_t          hwaddr_set;       /* optional */
> > > +    /** Promiscuous mode. */
> > > +    uk_netdev_promiscuous_set_t     promiscuous_set;  /* optional */
> > > +    uk_netdev_promiscuous_get_t     promiscuous_get;
> > > +
> > >       /** Device/driver capabilities and info. */
> > >       uk_netdev_info_get_t            info_get;
> > >       uk_netdev_txq_info_get_t        txq_info_get;
> > > diff --git a/lib/uknetdev/netdev.c b/lib/uknetdev/netdev.c
> > > index 3fb521a..0af63ac 100644
> > > --- a/lib/uknetdev/netdev.c
> > > +++ b/lib/uknetdev/netdev.c
> > > @@ -78,6 +78,7 @@ int uk_netdev_drv_register(struct uk_netdev *dev, 
> > > struct uk_alloc *a,
> > >       UK_ASSERT(dev->ops->txq_info_get);
> > >       UK_ASSERT(dev->ops->txq_configure);
> > >       UK_ASSERT(dev->ops->start);
> > > +    UK_ASSERT(dev->ops->promiscuous_get);
> > >       dev->_data = _alloc_data(a, netdev_count,  drv_name);
> > >       if (!dev->_data)
> > > @@ -449,3 +450,37 @@ const struct uk_hwaddr 
> > > *uk_netdev_hwaddr_get(struct uk_netdev *dev)
> > >       return dev->ops->hwaddr_get(dev);
> > >   }
> > > +
> > > +int uk_netdev_promiscuous_get(struct uk_netdev *dev)
> > > +{
> > > +    UK_ASSERT(dev);
> > > +    UK_ASSERT(dev->_data);
> > > +    UK_ASSERT(dev->ops);
> > > +    UK_ASSERT(dev->ops->promiscuous_get);
> > > +
> > > +    /* We do support retrieving of promiscuous mode
> > > +     * only when device was configured
> > > +     */
> > > +    UK_ASSERT(dev->_data->state == UK_NETDEV_CONFIGURED ||
> > > +          dev->_data->state == UK_NETDEV_RUNNING);
> > > +
> > > +    return dev->ops->promiscuous_get(dev);
> > > +}
> > > +
> > > +int uk_netdev_promiscuous_set(struct uk_netdev *dev, int mode)
> > > +{
> > > +    UK_ASSERT(dev);
> > > +    UK_ASSERT(dev->_data);
> > > +    UK_ASSERT(dev->ops);
> > > +
> > > +    /* We do support setting of promiscuous mode
> > > +     * only when device was configured
> > > +     */
> > > +    UK_ASSERT(dev->_data->state == UK_NETDEV_CONFIGURED
> > > +          || dev->_data->state == UK_NETDEV_RUNNING);
> > > +
> > > +    if (unlikely(!dev->ops->promiscuous_set))
> > > +        return -ENOTSUP;
> > > +
> > > +    return dev->ops->promiscuous_set(dev, mode ? 1 : 0);
> > > +}
> > Thanks & Regards
> > Sharan
> _______________________________________________
> Minios-devel mailing list
> Minios-devel@xxxxxxxxxxxxxxxxxxxx
> https://lists.xenproject.org/mailman/listinfo/minios-devel
_______________________________________________
Minios-devel mailing list
Minios-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/minios-devel