Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

On Thu, 19 May 2022 at 22:28, Denis Kenzior <denkenz(a)gmail.com> wrote:
> > Makes sense.  How about using a global rtnl object for all of the
> > l_netconfig objects?  Do you still prefer that it be passed from
> > outside?
>
> I have no real preference on whether it is passed in from the outside vs not.
> But, there's another thing to consider here.  Today the call to l_foo_set_rtnl
> is used determine whether the dhcp/icmp/dhcp6 object should set everything up
> directly vs letting the outside layer do this.  With l_netconfig I don't think
> this is a problem per se, but you should consider the other classes when you
> design this.

Right but with l_netconfig you make this choice by either calling
l_netconfig_apply_rtnl or not, and you shouldn't call l_foo_set_rtnl
on the underlying dhcp/icmp objects.

>
> We can start by trying to come up with a nice ell API for obtaining the
> singleton rtnl object and go from there.

The global rtnl object I meant is to be used only by l_netconfig
instances so there's no need for a getter, or I'm misunderstanding
what you're saying.

Best regards

Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

On Thu, 19 May 2022 at 23:28, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 5/19/22 15:53, Andrew Zaborowski wrote:
> > On Thu, 19 May 2022 at 22:28, Denis Kenzior <denkenz(a)gmail.com> wrote:
> >>> Makes sense.  How about using a global rtnl object for all of the
> >>> l_netconfig objects?  Do you still prefer that it be passed from
> >>> outside?
> >>
> >> I have no real preference on whether it is passed in from the outside vs not.
> >> But, there's another thing to consider here.  Today the call to l_foo_set_rtnl
> >> is used determine whether the dhcp/icmp/dhcp6 object should set everything up
> >> directly vs letting the outside layer do this.  With l_netconfig I don't think
> >> this is a problem per se, but you should consider the other classes when you
> >> design this.
> >
> > Right but with l_netconfig you make this choice by either calling
> > l_netconfig_apply_rtnl or not, and you shouldn't call l_foo_set_rtnl
> > on the underlying dhcp/icmp objects.
> >
>
> Sure, but are you assuming we will not have any further classes using rtnl
> besides netconfig?
>
> >>
> >> We can start by trying to come up with a nice ell API for obtaining the
> >> singleton rtnl object and go from there.
> >
> > The global rtnl object I meant is to be used only by l_netconfig
> > instances so there's no need for a getter, or I'm misunderstanding
> > what you're saying.
> >
>
> We use rtnl all over the place in iwd, like netdev, p2p, ap, etc.  All these +
> netconfig should share the same one.

That's different than what I did in patch 11/17 but it would work.
I'll put it in rtnl.c then, with a _get() and a _put() since we have
no generic refcounting for l_netlink.

Best refards

Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Denis Kenzior]

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

Hi Andrew,

On 5/19/22 15:53, Andrew Zaborowski wrote:
> On Thu, 19 May 2022 at 22:28, Denis Kenzior <denkenz(a)gmail.com> wrote:
>>> Makes sense.  How about using a global rtnl object for all of the
>>> l_netconfig objects?  Do you still prefer that it be passed from
>>> outside?
>>
>> I have no real preference on whether it is passed in from the outside vs not.
>> But, there's another thing to consider here.  Today the call to l_foo_set_rtnl
>> is used determine whether the dhcp/icmp/dhcp6 object should set everything up
>> directly vs letting the outside layer do this.  With l_netconfig I don't think
>> this is a problem per se, but you should consider the other classes when you
>> design this.
> 
> Right but with l_netconfig you make this choice by either calling
> l_netconfig_apply_rtnl or not, and you shouldn't call l_foo_set_rtnl
> on the underlying dhcp/icmp objects.
> 

Sure, but are you assuming we will not have any further classes using rtnl 
besides netconfig?

>>
>> We can start by trying to come up with a nice ell API for obtaining the
>> singleton rtnl object and go from there.
> 
> The global rtnl object I meant is to be used only by l_netconfig
> instances so there's no need for a getter, or I'm misunderstanding
> what you're saying.
> 

We use rtnl all over the place in iwd, like netdev, p2p, ap, etc.  All these + 
netconfig should share the same one.

Regards,
-Denis

Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Denis Kenzior]

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

Hi Andrew,

> Makes sense.  How about using a global rtnl object for all of the
> l_netconfig objects?  Do you still prefer that it be passed from
> outside?

I have no real preference on whether it is passed in from the outside vs not. 
But, there's another thing to consider here.  Today the call to l_foo_set_rtnl 
is used determine whether the dhcp/icmp/dhcp6 object should set everything up 
directly vs letting the outside layer do this.  With l_netconfig I don't think 
this is a problem per se, but you should consider the other classes when you 
design this.

We can start by trying to come up with a nice ell API for obtaining the 
singleton rtnl object and go from there.

Regards,
-Denis

Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

Hi Denis,

On Wed, 18 May 2022 at 20:58, Denis Kenzior <denkenz(a)gmail.com> wrote:
> On 5/13/22 17:47, Andrew Zaborowski wrote:
> >   LIB_EXPORT struct l_netconfig *l_netconfig_new(uint32_t ifindex)
> >   {
> >       struct l_netconfig *nc;
> > +     struct l_netlink *rtnl = l_netlink_new(NETLINK_ROUTE);
> > +
> > +     if (!rtnl)
> > +             return NULL;
>
> I'm against this.  We will have multiple netconfig objects in play and each one
> would be utilizing an fd.  We should be keeping our fd use to a minimum,
> particularly since we're limited to ~128 right now.  Passing in the rtnl object
> from outside is preferred.

Makes sense.  How about using a global rtnl object for all of the
l_netconfig objects?  Do you still prefer that it be passed from
outside?

I did this later in patch 11 and maybe should have merged that change
directly here.  We could also create it only after
l_netconfig_start().

Best regards

Re: [PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Denis Kenzior]

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

Hi Andrew,

On 5/13/22 17:47, Andrew Zaborowski wrote:
> Since we'll need to keep an internal RTNL socket for DHCPv6
> functionality anyway, change l_netconfig_apply_rtnl's signature to not
> require an rtnl parameter.
> ---
>   ell/netconfig.c | 34 ++++++++++++++++++++--------------
>   ell/netconfig.h |  3 +--
>   2 files changed, 21 insertions(+), 16 deletions(-)
> 

<snip>

> @@ -730,8 +731,13 @@ static void netconfig_icmp6_event_handler(struct l_icmp6_client *client,
>   LIB_EXPORT struct l_netconfig *l_netconfig_new(uint32_t ifindex)
>   {
>   	struct l_netconfig *nc;
> +	struct l_netlink *rtnl = l_netlink_new(NETLINK_ROUTE);
> +
> +	if (!rtnl)
> +		return NULL;

I'm against this.  We will have multiple netconfig objects in play and each one 
would be utilizing an fd.  We should be keeping our fd use to a minimum, 
particularly since we're limited to ~128 right now.  Passing in the rtnl object 
from outside is preferred.

Regards,
-Denis

[PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

Since we'll need to keep an internal RTNL socket for DHCPv6
functionality anyway, change l_netconfig_apply_rtnl's signature to not
require an rtnl parameter.
---
 ell/netconfig.c | 34 ++++++++++++++++++++--------------
 ell/netconfig.h |  3 +--
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/ell/netconfig.c b/ell/netconfig.c
index 94d3ed1..55a7993 100644
--- a/ell/netconfig.c
+++ b/ell/netconfig.c
@@ -64,6 +64,7 @@ struct l_netconfig {
 	char **v6_dns_override;
 	char **v6_domain_names_override;
 
+	struct l_netlink *rtnl;
 	bool started;
 	struct l_idle *do_static_work;
 	bool v4_configured;
@@ -730,8 +731,13 @@ static void netconfig_icmp6_event_handler(struct l_icmp6_client *client,
 LIB_EXPORT struct l_netconfig *l_netconfig_new(uint32_t ifindex)
 {
 	struct l_netconfig *nc;
+	struct l_netlink *rtnl = l_netlink_new(NETLINK_ROUTE);
+
+	if (!rtnl)
+		return NULL;
 
 	nc = l_new(struct l_netconfig, 1);
+	nc->rtnl = rtnl;
 	nc->ifindex = ifindex;
 	nc->v4_enabled = true;
 
@@ -789,6 +795,7 @@ LIB_EXPORT void l_netconfig_destroy(struct l_netconfig *netconfig)
 	l_queue_destroy(netconfig->routes.added, NULL);
 	l_queue_destroy(netconfig->routes.updated, NULL);
 	l_queue_destroy(netconfig->routes.removed, NULL);
+	l_netlink_destroy(netconfig->rtnl);
 	l_free(netconfig);
 }
 
@@ -1193,42 +1200,41 @@ LIB_EXPORT void l_netconfig_set_event_handler(struct l_netconfig *netconfig,
 	netconfig->handler.destroy = destroy;
 }
 
-LIB_EXPORT void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
-					struct l_netlink *rtnl)
+LIB_EXPORT void l_netconfig_apply_rtnl(struct l_netconfig *netconfig)
 {
 	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);
+		l_rtnl_ifaddr_delete(netconfig->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);
+		l_rtnl_ifaddr_add(netconfig->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);
+		l_rtnl_ifaddr_add(netconfig->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);
+		l_rtnl_route_delete(netconfig->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);
+		l_rtnl_route_add(netconfig->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);
+		l_rtnl_route_add(netconfig->rtnl, netconfig->ifindex,
+					entry->data, NULL, NULL, NULL);
 }
 
 LIB_EXPORT const struct l_queue_entry *l_netconfig_get_addresses(
diff --git a/ell/netconfig.h b/ell/netconfig.h
index d58dda1..874491f 100644
--- a/ell/netconfig.h
+++ b/ell/netconfig.h
@@ -81,8 +81,7 @@ 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);
+void l_netconfig_apply_rtnl(struct l_netconfig *netconfig);
 const struct l_queue_entry *l_netconfig_get_addresses(
 				struct l_netconfig *netconfig,
 				const struct l_queue_entry **out_added,
-- 
2.32.0

[PATCH 07/17] netconfig: Use an internal rtnl socket for l_netconfig_apply_rtnl

[Andrew Zaborowski]

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

Since we'll need to keep an internal RTNL socket for DHCPv6
functionality anyway, change l_netconfig_apply_rtnl's signature to not
require an rtnl parameter.
---
 ell/netconfig.c | 34 ++++++++++++++++++++--------------
 ell/netconfig.h |  3 +--
 2 files changed, 21 insertions(+), 16 deletions(-)

diff --git a/ell/netconfig.c b/ell/netconfig.c
index aee56a2..724cb38 100644
--- a/ell/netconfig.c
+++ b/ell/netconfig.c
@@ -63,6 +63,7 @@ struct l_netconfig {
 	char **v6_dns_override;
 	char **v6_domain_names_override;
 
+	struct l_netlink *rtnl;
 	bool started;
 	struct l_idle *do_static_work;
 	bool v4_configured;
@@ -725,8 +726,13 @@ static void netconfig_icmp6_event_handler(struct l_icmp6_client *client,
 LIB_EXPORT struct l_netconfig *l_netconfig_new(uint32_t ifindex)
 {
 	struct l_netconfig *nc;
+	struct l_netlink *rtnl = l_netlink_new(NETLINK_ROUTE);
+
+	if (!rtnl)
+		return NULL;
 
 	nc = l_new(struct l_netconfig, 1);
+	nc->rtnl = rtnl;
 	nc->ifindex = ifindex;
 	nc->v4_enabled = true;
 
@@ -784,6 +790,7 @@ LIB_EXPORT void l_netconfig_destroy(struct l_netconfig *netconfig)
 	l_queue_destroy(netconfig->routes.added, NULL);
 	l_queue_destroy(netconfig->routes.updated, NULL);
 	l_queue_destroy(netconfig->routes.removed, NULL);
+	l_netlink_destroy(netconfig->rtnl);
 	l_free(netconfig);
 }
 
@@ -1188,42 +1195,41 @@ LIB_EXPORT void l_netconfig_set_event_handler(struct l_netconfig *netconfig,
 	netconfig->handler.destroy = destroy;
 }
 
-LIB_EXPORT void l_netconfig_apply_rtnl(struct l_netconfig *netconfig,
-					struct l_netlink *rtnl)
+LIB_EXPORT void l_netconfig_apply_rtnl(struct l_netconfig *netconfig)
 {
 	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);
+		l_rtnl_ifaddr_delete(netconfig->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);
+		l_rtnl_ifaddr_add(netconfig->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);
+		l_rtnl_ifaddr_add(netconfig->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);
+		l_rtnl_route_delete(netconfig->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);
+		l_rtnl_route_add(netconfig->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);
+		l_rtnl_route_add(netconfig->rtnl, netconfig->ifindex,
+					entry->data, NULL, NULL, NULL);
 }
 
 LIB_EXPORT const struct l_queue_entry *l_netconfig_get_addresses(
diff --git a/ell/netconfig.h b/ell/netconfig.h
index d58dda1..874491f 100644
--- a/ell/netconfig.h
+++ b/ell/netconfig.h
@@ -81,8 +81,7 @@ 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);
+void l_netconfig_apply_rtnl(struct l_netconfig *netconfig);
 const struct l_queue_entry *l_netconfig_get_addresses(
 				struct l_netconfig *netconfig,
 				const struct l_queue_entry **out_added,
-- 
2.32.0