Re: [PATCH 01/11] netdev: Add a wdev_id based frame watch API

[Denis Kenzior <denkenz@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 1835 bytes --]

Hi Andrew,

On 10/21/19 8:55 AM, Andrew Zaborowski wrote:
> From: Andrew Zaborowski <andrew.zaborowski@intel.com>
> 
> Allow watching for frames on interfaces that have no netdev in the
> kernel and can only be referenced through the wdev_id instead of the
> ifindex.
> ---
>   src/netdev.c | 86 +++++++++++++++++++++++++++++++++++++++++++++++-----
>   src/netdev.h |  6 ++++
>   2 files changed, 84 insertions(+), 8 deletions(-)
> 

<snip>

> diff --git a/src/netdev.h b/src/netdev.h
> index 114a6035..624811b2 100644
> --- a/src/netdev.h
> +++ b/src/netdev.h
> @@ -197,6 +197,12 @@ uint32_t netdev_frame_watch_add(struct netdev *netdev, uint16_t frame_type,
>   				void *user_data);
>   bool netdev_frame_watch_remove(struct netdev *netdev, uint32_t id);
>   
> +uint32_t netdev_wdev_frame_watch_add(uint64_t wdev_id, uint16_t frame_type,
> +				const uint8_t *prefix, size_t prefix_len,
> +				netdev_frame_watch_func_t handler,
> +				void *user_data);
> +bool netdev_wdev_frame_watch_remove(uint32_t id);
> +

So this really seems like it is jammed in there and it doesn't fit well.

I think that we should make frame watches into a separate module now 
that they no longer are only associated with a netdev.  This would allow 
us to also track iftype changes and wipe out those frame watches that 
are no longer registered with the kernel.  And also allow us not to try 
and register frame watches that are already live (and bounce with an 
-EAGAIN from the kernel).

We could also implement 'transient' frame watch sets by using a genl 
socket dedicated to each set.  At least until the kernel people fix 
their subsystem.

>   void netdev_handshake_failed(struct handshake_state *hs, uint16_t reason_code);
>   
>   struct netdev *netdev_find(int ifindex);
> 

Regards,
-Denis