Re: [PATCH 5/6] netconfig: Add l_netconfig_apply_rtnl

Re: [PATCH 5/6] netconfig: Add l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

On Tue, 8 Feb 2022 at 19:19, Andrew Zaborowski
<andrew.zaborowski(a)intel.com> wrote:
> On Tue, 8 Feb 2022 at 18:42, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > On 2/7/22 09:07, Andrew Zaborowski wrote:
> > > Add l_netconfig_apply_rtnl and the getters for current lists of routes
> > > and addresses for the netconfig user to be able to commit address and
> > > route changes.
> > > ---
> > >   ell/ell.sym     |  3 ++
> > >   ell/netconfig.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
> > >   ell/netconfig.h | 11 ++++++++
> > >   3 files changed, 87 insertions(+)
> > >
> >
> > <snip>
> >
> > >
> > > +void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
> > > +                             struct l_netlink *rtnl);
> > > +struct l_queue *l_netconfig_get_addresses(struct l_netconfig *netconfig,
> > > +                                     struct l_queue **out_added,
> > > +                                     struct l_queue **out_updated,
> > > +                                     struct l_queue **out_removed);
> > > +struct l_queue *l_netconfig_get_routes(struct l_netconfig *netconfig,
> > > +                                     struct l_queue **out_added,
> > > +                                     struct l_queue **out_updated,
> > > +                                     struct l_queue **out_removed);
> > > +
> >
> > I wonder if we can use const l_queue_entry * instead.  Or maybe some sort of
> > iterator concept?  Returning a pointer to the internal data structure is
> > something we should only do as a last resort.
>
> "const l_queue_entry *" would work.  We can also just transfer the
> ownership of those three lists to the caller and drop our references
> to those three lists when we return them.  They're actually temporary,
> I just used l_queue_clear instead of destroying and recreating them.

(This doesn't apply to the complete list of addresses/routes that we
also return, you're right that we'd need to return an iterator.
struct l_queue_entry is pretty much an iterator except that the
.next/.data members are public)

Best regards

Re: [PATCH 5/6] netconfig: Add l_netconfig_apply_rtnl

[Denis Kenzior]

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

Hi Andrew,

>>> I wonder if we can use const l_queue_entry * instead.  Or maybe some sort of
>>> iterator concept?  Returning a pointer to the internal data structure is
>>> something we should only do as a last resort.
>>
>> "const l_queue_entry *" would work.  We can also just transfer the
>> ownership of those three lists to the caller and drop our references
>> to those three lists when we return them.  They're actually temporary,
>> I just used l_queue_clear instead of destroying and recreating them.
> 
> (This doesn't apply to the complete list of addresses/routes that we
> also return, you're right that we'd need to return an iterator.
> struct l_queue_entry is pretty much an iterator except that the
> .next/.data members are public)
> 

Okay.  See what you can come up with.  If l_queue_entry works, that would likely 
be the easiest approach.

Regards,
-Denis

Re: [PATCH 5/6] netconfig: Add l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

On Tue, 8 Feb 2022 at 18:42, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 2/7/22 09:07, Andrew Zaborowski wrote:
> > Add l_netconfig_apply_rtnl and the getters for current lists of routes
> > and addresses for the netconfig user to be able to commit address and
> > route changes.
> > ---
> >   ell/ell.sym     |  3 ++
> >   ell/netconfig.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
> >   ell/netconfig.h | 11 ++++++++
> >   3 files changed, 87 insertions(+)
> >
>
> <snip>
>
> >
> > +void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
> > +                             struct l_netlink *rtnl);
> > +struct l_queue *l_netconfig_get_addresses(struct l_netconfig *netconfig,
> > +                                     struct l_queue **out_added,
> > +                                     struct l_queue **out_updated,
> > +                                     struct l_queue **out_removed);
> > +struct l_queue *l_netconfig_get_routes(struct l_netconfig *netconfig,
> > +                                     struct l_queue **out_added,
> > +                                     struct l_queue **out_updated,
> > +                                     struct l_queue **out_removed);
> > +
>
> I wonder if we can use const l_queue_entry * instead.  Or maybe some sort of
> iterator concept?  Returning a pointer to the internal data structure is
> something we should only do as a last resort.

"const l_queue_entry *" would work.  We can also just transfer the
ownership of those three lists to the caller and drop our references
to those three lists when we return them.  They're actually temporary,
I just used l_queue_clear instead of destroying and recreating them.

Best regards

Re: [PATCH 5/6] netconfig: Add l_netconfig_apply_rtnl

[Denis Kenzior]

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

Hi Andrew,

On 2/7/22 09:07, Andrew Zaborowski wrote:
> Add l_netconfig_apply_rtnl and the getters for current lists of routes
> and addresses for the netconfig user to be able to commit address and
> route changes.
> ---
>   ell/ell.sym     |  3 ++
>   ell/netconfig.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
>   ell/netconfig.h | 11 ++++++++
>   3 files changed, 87 insertions(+)
> 

<snip>

>   
> +void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
> +				struct l_netlink *rtnl);
> +struct l_queue *l_netconfig_get_addresses(struct l_netconfig *netconfig,
> +					struct l_queue **out_added,
> +					struct l_queue **out_updated,
> +					struct l_queue **out_removed);
> +struct l_queue *l_netconfig_get_routes(struct l_netconfig *netconfig,
> +					struct l_queue **out_added,
> +					struct l_queue **out_updated,
> +					struct l_queue **out_removed);
> +

I wonder if we can use const l_queue_entry * instead.  Or maybe some sort of 
iterator concept?  Returning a pointer to the internal data structure is 
something we should only do as a last resort.

Regards,
-Denis

[PATCH 5/6] netconfig: Add l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

Add l_netconfig_apply_rtnl and the getters for current lists of routes
and addresses for the netconfig user to be able to commit address and
route changes.
---
 ell/ell.sym     |  3 ++
 ell/netconfig.c | 73 +++++++++++++++++++++++++++++++++++++++++++++++++
 ell/netconfig.h | 11 ++++++++
 3 files changed, 87 insertions(+)

diff --git a/ell/ell.sym b/ell/ell.sym
index 6c27800..b256771 100644
--- a/ell/ell.sym
+++ b/ell/ell.sym
@@ -728,6 +728,9 @@ global:
 	l_netconfig_stop;
 	l_netconfig_get_dhcp_client;
 	l_netconfig_set_event_handler;
+	l_netconfig_apply_rtnl;
+	l_netconfig_get_addresses;
+	l_netconfig_get_routes;
 local:
 	*;
 };
diff --git a/ell/netconfig.c b/ell/netconfig.c
index e3ac751..7317a79 100644
--- a/ell/netconfig.c
+++ b/ell/netconfig.c
@@ -373,3 +373,76 @@ LIB_EXPORT void l_netconfig_set_event_handler(struct l_netconfig *netconfig,
 	netconfig->handler.user_data = user_data;
 	netconfig->handler.destroy = destroy;
 }
+
+LIB_EXPORT void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
+					struct l_netlink *rtnl)
+{
+	const struct l_queue_entry *entry;
+
+	for (entry = l_queue_get_entries(netconfig->addresses.removed); entry;
+			entry = entry->next)
+		l_rtnl_ifaddr_delete(rtnl, netconfig->ifindex, entry->data,
+					NULL, NULL, NULL);
+
+	for (entry = l_queue_get_entries(netconfig->addresses.added); entry;
+			entry = entry->next)
+		l_rtnl_ifaddr_add(rtnl, netconfig->ifindex, entry->data,
+					NULL, NULL, NULL);
+
+	/* We can use l_rtnl_ifaddr_add here since that uses NLM_F_REPLACE */
+	for (entry = l_queue_get_entries(netconfig->addresses.updated); entry;
+			entry = entry->next)
+		l_rtnl_ifaddr_add(rtnl, netconfig->ifindex, entry->data,
+					NULL, NULL, NULL);
+
+	for (entry = l_queue_get_entries(netconfig->routes.removed); entry;
+			entry = entry->next)
+		l_rtnl_route_delete(rtnl, netconfig->ifindex, entry->data,
+					NULL, NULL, NULL);
+
+	for (entry = l_queue_get_entries(netconfig->routes.added); entry;
+			entry = entry->next)
+		l_rtnl_route_add(rtnl, netconfig->ifindex, entry->data,
+					NULL, NULL, NULL);
+
+	/* We can use l_rtnl_route_add here since that uses NLM_F_REPLACE */
+	for (entry = l_queue_get_entries(netconfig->routes.updated); entry;
+			entry = entry->next)
+		l_rtnl_route_add(rtnl, netconfig->ifindex, entry->data,
+					NULL, NULL, NULL);
+}
+
+LIB_EXPORT struct l_queue *l_netconfig_get_addresses(
+						struct l_netconfig *netconfig,
+						struct l_queue **out_added,
+						struct l_queue **out_updated,
+						struct l_queue **out_removed)
+{
+	if (out_added)
+		*out_added = netconfig->addresses.added;
+
+	if (out_updated)
+		*out_updated = netconfig->addresses.updated;
+
+	if (out_removed)
+		*out_removed = netconfig->addresses.removed;
+
+	return netconfig->addresses.current;
+}
+
+LIB_EXPORT struct l_queue *l_netconfig_get_routes(struct l_netconfig *netconfig,
+						struct l_queue **out_added,
+						struct l_queue **out_updated,
+						struct l_queue **out_removed)
+{
+	if (out_added)
+		*out_added = netconfig->routes.added;
+
+	if (out_updated)
+		*out_updated = netconfig->routes.updated;
+
+	if (out_removed)
+		*out_removed = netconfig->routes.removed;
+
+	return netconfig->routes.current;
+}
diff --git a/ell/netconfig.h b/ell/netconfig.h
index eab48b0..bb5eff5 100644
--- a/ell/netconfig.h
+++ b/ell/netconfig.h
@@ -66,6 +66,17 @@ void l_netconfig_set_event_handler(struct l_netconfig *netconfig,
 					void *user_data,
 					l_netconfig_destroy_cb_t destroy);
 
+void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
+				struct l_netlink *rtnl);
+struct l_queue *l_netconfig_get_addresses(struct l_netconfig *netconfig,
+					struct l_queue **out_added,
+					struct l_queue **out_updated,
+					struct l_queue **out_removed);
+struct l_queue *l_netconfig_get_routes(struct l_netconfig *netconfig,
+					struct l_queue **out_added,
+					struct l_queue **out_updated,
+					struct l_queue **out_removed);
+
 #ifdef __cplusplus
 }
 #endif
-- 
2.32.0