On 10/17/25 4:32 PM, Eelco Chaudron wrote: > > > On 15 Oct 2025, at 16:59, Eli Britstein wrote: > >> Introduce generic API for netdev-class to apply global other_config >> configuration and status update as a pre-step towards generic library >> (dpdk for now) initialization and status updates. > > Hi Eli, > > Thanks for the series. I did a quick review of the first patch, and I think > it would be good to get some clarification on the OVSDB API being propagated > down to netdev (see comment below). As Ilya is already copied, I hope he can > shed some light on this also. > > Cheers, > > Eelco > >> Signed-off-by: Eli Britstein <[email protected]> >> --- >> lib/netdev-provider.h | 12 ++++++++++++ >> lib/netdev.c | 28 ++++++++++++++++++++++++++++ >> lib/netdev.h | 3 +++ >> 3 files changed, 43 insertions(+) >> >> diff --git a/lib/netdev-provider.h b/lib/netdev-provider.h >> index 5ae379469..afbb1912d 100644 >> --- a/lib/netdev-provider.h >> +++ b/lib/netdev-provider.h >> @@ -35,6 +35,8 @@ extern "C" { >> #endif >> >> struct netdev_tnl_build_header_params; >> +struct ovsrec_open_vswitch; >> + >> #define NETDEV_NUMA_UNSPEC OVS_NUMA_UNSPEC >> >> enum netdev_ol_flags { >> @@ -290,6 +292,16 @@ struct netdev_class { >> * function is used to implement different classes. */ >> void (*wait)(const struct netdev_class *netdev_class); >> >> + /* Applies global other_config settings. > > Maybe change this to: > Pass custom configuration options to the netdev class’s implementation. > >> + * >> + * 'netdev_class' points to the class. It is useful in case the same >> + * function is used to implement different classes. */ >> + int (*set_other_config)(const struct netdev_class *netdev_class, >> + const struct smap *other_config); > > We already have set_config, which does the same thing but at the netdev level. > Should we use a similar name, but with the distinction that it’s at the class > level? > > class_set_config()? > > >> + /* Set relevant status in 'cfg'. */ > > We definitely need some comments explaining what this function actually does. > >> + void (*class_status)(const struct ovsrec_open_vswitch *cfg); > > I think the name should be more meaningful. What about class_ovsdb_update() > or class_ovsdb_status_update()? > > In addition, this is also a bit of an odd API. The OVSDB is not exposed to > netdevs at all > (we use the smap), and now we’re adding this as a reverse dependency. > Shouldn’t we have > some kind of smap interface here as well? > > Ilya, thoughts?
Using smap is definitely preferable, it's not particualrly great to pass database row pointers around. Also, it feels a little off that we're creating a separate entity called "library", but its initialization and configuration is handled through the netdev... Maybe we should treat it as a separate entity and be initialized on it's own and then check from the netdev that needs a specific library if it is initialized or not? i.e. create a library class with it's own API, like library_set_config(name/type), library_initialized(name/type), library_status(name/type) ? "library" here may be a too generic term, as it sounds like we'll be configuring specific types of libraries, but I'm not sure how to call it... maybe "library" is good enough. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
