[1/1] package/connman: enable wireguard
diff mbox series

Message ID 20200308070741.42542-1-james.hilliard1@gmail.com
State Accepted
Headers show
Series
  • [1/1] package/connman: enable wireguard
Related show

Commit Message

James Hilliard March 8, 2020, 7:07 a.m. UTC
We also need to select libmnl when building with wireguard support or
with nftables.

Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
---
 package/connman/Config.in  | 5 +++++
 package/connman/connman.mk | 9 ++++++++-
 2 files changed, 13 insertions(+), 1 deletion(-)

Comments

Thomas Petazzoni March 8, 2020, 1:47 p.m. UTC | #1
Hello,

On Sun,  8 Mar 2020 00:07:41 -0700
James Hilliard <james.hilliard1@gmail.com> wrote:

> We also need to select libmnl when building with wireguard support or
> with nftables.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  package/connman/Config.in  | 5 +++++
>  package/connman/connman.mk | 9 ++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/package/connman/Config.in b/package/connman/Config.in
> index ac012dda54..614b826f96 100644
> --- a/package/connman/Config.in
> +++ b/package/connman/Config.in
> @@ -33,6 +33,7 @@ config BR2_PACKAGE_CONNMAN_NFTABLES
>  	bool "nftables"
>  	depends on BR2_USE_WCHAR
>  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_12
> +	select BR2_PACKAGE_LIBMNL

How is this related to wireguard support ?

>  	select BR2_PACKAGE_NFTABLES
>  	help
>  	  Use nftables as firewall.
> @@ -51,6 +52,10 @@ config BR2_PACKAGE_CONNMAN_WIFI
>  	  setup). ConnMan detects the start of wpa_supplicant
>  	  automatically.
>  
> +config BR2_PACKAGE_CONNMAN_WIREGUARD
> +	bool "enable wireguard support"
> +	select BR2_PACKAGE_LIBMNL

We already had a patch from Petr at
http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.

Could you or Petr clarify this ?

Best regards,

Thomas
Petr Vorel March 8, 2020, 5:37 p.m. UTC | #2
Hi Thomas, James,

> On Sun,  8 Mar 2020 00:07:41 -0700
> James Hilliard <james.hilliard1@gmail.com> wrote:

> > We also need to select libmnl when building with wireguard support or
> > with nftables.

> > Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> > ---
> >  package/connman/Config.in  | 5 +++++
> >  package/connman/connman.mk | 9 ++++++++-
> >  2 files changed, 13 insertions(+), 1 deletion(-)

> > diff --git a/package/connman/Config.in b/package/connman/Config.in
> > index ac012dda54..614b826f96 100644
> > --- a/package/connman/Config.in
> > +++ b/package/connman/Config.in
> > @@ -33,6 +33,7 @@ config BR2_PACKAGE_CONNMAN_NFTABLES
> >  	bool "nftables"
> >  	depends on BR2_USE_WCHAR
> >  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_12
> > +	select BR2_PACKAGE_LIBMNL

> How is this related to wireguard support ?
Yes.

> >  	select BR2_PACKAGE_NFTABLES
> >  	help
> >  	  Use nftables as firewall.
> > @@ -51,6 +52,10 @@ config BR2_PACKAGE_CONNMAN_WIFI
> >  	  setup). ConnMan detects the start of wpa_supplicant
> >  	  automatically.

> > +config BR2_PACKAGE_CONNMAN_WIREGUARD
> > +	bool "enable wireguard support"
> > +	select BR2_PACKAGE_LIBMNL

> We already had a patch from Petr at
> http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
> but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.

> Could you or Petr clarify this ?
configure.ac
found_libmnl="no"
if (test "${firewall_type}" = "nftables" -o \
		"${enable_wireguard}" != "no"); then
	PKG_CHECK_MODULES(LIBMNL, [libmnl >= 1.0.0], [found_libmnl="yes"],
		AC_MSG_ERROR([libmnl >= 1.0.0 not found]))
	AC_SUBST(LIBMNL_CFLAGS)
	AC_SUBST(LIBMNL_LIBS)
fi
AM_CONDITIONAL(LIBMNL, test "${found_libmnl}" != "no")

IMHO this means that libmnl is required for nftables and wireguard.

Kind regards,
Petr
Peter Korsgaard March 8, 2020, 6:17 p.m. UTC | #3
>>>>> "Petr" == Petr Vorel <petr.vorel@gmail.com> writes:

Hi,

 >> > +config BR2_PACKAGE_CONNMAN_WIREGUARD
 >> > +	bool "enable wireguard support"
 >> > +	select BR2_PACKAGE_LIBMNL

 >> We already had a patch from Petr at
 >> http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
 >> but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.

 >> Could you or Petr clarify this ?
 > configure.ac
 > found_libmnl="no"
 > if (test "${firewall_type}" = "nftables" -o \
 > 		"${enable_wireguard}" != "no"); then
 > 	PKG_CHECK_MODULES(LIBMNL, [libmnl >= 1.0.0], [found_libmnl="yes"],
 > 		AC_MSG_ERROR([libmnl >= 1.0.0 not found]))
 > 	AC_SUBST(LIBMNL_CFLAGS)
 > 	AC_SUBST(LIBMNL_LIBS)
 > fi
 > AM_CONDITIONAL(LIBMNL, test "${found_libmnl}" != "no")

 > IMHO this means that libmnl is required for nftables and wireguard.

Yes. What about wireguard-tools? Are they used by connman or does it
directly talk the netlink protocol? I guess the latter given the libmnl
dependency?
James Hilliard March 8, 2020, 7:50 p.m. UTC | #4
On Sun, Mar 8, 2020 at 12:17 PM Peter Korsgaard <peter@korsgaard.com> wrote:
>
> >>>>> "Petr" == Petr Vorel <petr.vorel@gmail.com> writes:
>
> Hi,
>
>  >> > +config BR2_PACKAGE_CONNMAN_WIREGUARD
>  >> > + bool "enable wireguard support"
>  >> > + select BR2_PACKAGE_LIBMNL
>
>  >> We already had a patch from Petr at
>  >> http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
>  >> but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.
>
>  >> Could you or Petr clarify this ?
>  > configure.ac
>  > found_libmnl="no"
>  > if (test "${firewall_type}" = "nftables" -o \
>  >              "${enable_wireguard}" != "no"); then
>  >      PKG_CHECK_MODULES(LIBMNL, [libmnl >= 1.0.0], [found_libmnl="yes"],
>  >              AC_MSG_ERROR([libmnl >= 1.0.0 not found]))
>  >      AC_SUBST(LIBMNL_CFLAGS)
>  >      AC_SUBST(LIBMNL_LIBS)
>  > fi
>  > AM_CONDITIONAL(LIBMNL, test "${found_libmnl}" != "no")
>
>  > IMHO this means that libmnl is required for nftables and wireguard.
>
> Yes. What about wireguard-tools? Are they used by connman or does it
> directly talk the netlink protocol? I guess the latter given the libmnl
> dependency?
Yeah, connman seemed to build fine with wireguard support without
wireguard-tools.
>
> --
> Bye, Peter Korsgaard
Peter Korsgaard March 8, 2020, 8:17 p.m. UTC | #5
>>>>> "James" == James Hilliard <james.hilliard1@gmail.com> writes:

 > On Sun, Mar 8, 2020 at 12:17 PM Peter Korsgaard <peter@korsgaard.com> wrote:
 >> 
 >> >>>>> "Petr" == Petr Vorel <petr.vorel@gmail.com> writes:
 >> 
 >> Hi,
 >> 
 >> >> > +config BR2_PACKAGE_CONNMAN_WIREGUARD
 >> >> > + bool "enable wireguard support"
 >> >> > + select BR2_PACKAGE_LIBMNL
 >> 
 >> >> We already had a patch from Petr at
 >> >> http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
 >> >> but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.
 >> 
 >> >> Could you or Petr clarify this ?
 >> > configure.ac
 >> > found_libmnl="no"
 >> > if (test "${firewall_type}" = "nftables" -o \
 >> >              "${enable_wireguard}" != "no"); then
 >> >      PKG_CHECK_MODULES(LIBMNL, [libmnl >= 1.0.0], [found_libmnl="yes"],
 >> >              AC_MSG_ERROR([libmnl >= 1.0.0 not found]))
 >> >      AC_SUBST(LIBMNL_CFLAGS)
 >> >      AC_SUBST(LIBMNL_LIBS)
 >> > fi
 >> > AM_CONDITIONAL(LIBMNL, test "${found_libmnl}" != "no")
 >> 
 >> > IMHO this means that libmnl is required for nftables and wireguard.
 >> 
 >> Yes. What about wireguard-tools? Are they used by connman or does it
 >> directly talk the netlink protocol? I guess the latter given the libmnl
 >> dependency?
 > Yeah, connman seemed to build fine with wireguard support without
 > wireguard-tools.

Well, a wireguard-tools dependency issue would probably be visible at
runtime as it would then just call out to those binaries.

Looking at the code, I see that is not the case and connman directly
talks the netlink interface of the kernel driver:

https://git.kernel.org/pub/scm/network/connman/connman.git/tree/vpn/plugins/libwireguard.c
Thomas Petazzoni March 8, 2020, 9:30 p.m. UTC | #6
Hello,

On Sun, 8 Mar 2020 18:37:18 +0100
Petr Vorel <petr.vorel@gmail.com> wrote:

> > > diff --git a/package/connman/Config.in b/package/connman/Config.in
> > > index ac012dda54..614b826f96 100644
> > > --- a/package/connman/Config.in
> > > +++ b/package/connman/Config.in
> > > @@ -33,6 +33,7 @@ config BR2_PACKAGE_CONNMAN_NFTABLES
> > >  	bool "nftables"
> > >  	depends on BR2_USE_WCHAR
> > >  	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_12
> > > +	select BR2_PACKAGE_LIBMNL  
> 
> > How is this related to wireguard support ?  
> Yes.
> 
> > >  	select BR2_PACKAGE_NFTABLES
> > >  	help
> > >  	  Use nftables as firewall.
> > > @@ -51,6 +52,10 @@ config BR2_PACKAGE_CONNMAN_WIFI
> > >  	  setup). ConnMan detects the start of wpa_supplicant
> > >  	  automatically.  
> 
> > > +config BR2_PACKAGE_CONNMAN_WIREGUARD
> > > +	bool "enable wireguard support"
> > > +	select BR2_PACKAGE_LIBMNL  
> 
> > We already had a patch from Petr at
> > http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
> > but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.  
> 
> > Could you or Petr clarify this ?  
> configure.ac
> found_libmnl="no"
> if (test "${firewall_type}" = "nftables" -o \
> 		"${enable_wireguard}" != "no"); then
> 	PKG_CHECK_MODULES(LIBMNL, [libmnl >= 1.0.0], [found_libmnl="yes"],
> 		AC_MSG_ERROR([libmnl >= 1.0.0 not found]))
> 	AC_SUBST(LIBMNL_CFLAGS)
> 	AC_SUBST(LIBMNL_LIBS)
> fi
> AM_CONDITIONAL(LIBMNL, test "${found_libmnl}" != "no")

Then the "select BR2_PACKAGE_LIBMNL" added to
BR2_PACKAGE_CONNMAN_NFTABLES is completely unrelated to wireguard
support. connman needs libmnl if nftables is used *or* if wireguard is
used.

So the missing select BR2_PACKAGE_LIBMNL in
BR2_PACKAGE_CONNMAN_NFTABLES should be added as a separate patch.

Thanks!

Thomas
James Hilliard March 8, 2020, 9:31 p.m. UTC | #7
On Sun, Mar 8, 2020 at 3:30 PM Thomas Petazzoni
<thomas.petazzoni@bootlin.com> wrote:
>
> Hello,
>
> On Sun, 8 Mar 2020 18:37:18 +0100
> Petr Vorel <petr.vorel@gmail.com> wrote:
>
> > > > diff --git a/package/connman/Config.in b/package/connman/Config.in
> > > > index ac012dda54..614b826f96 100644
> > > > --- a/package/connman/Config.in
> > > > +++ b/package/connman/Config.in
> > > > @@ -33,6 +33,7 @@ config BR2_PACKAGE_CONNMAN_NFTABLES
> > > >   bool "nftables"
> > > >   depends on BR2_USE_WCHAR
> > > >   depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_12
> > > > + select BR2_PACKAGE_LIBMNL
> >
> > > How is this related to wireguard support ?
> > Yes.
> >
> > > >   select BR2_PACKAGE_NFTABLES
> > > >   help
> > > >     Use nftables as firewall.
> > > > @@ -51,6 +52,10 @@ config BR2_PACKAGE_CONNMAN_WIFI
> > > >     setup). ConnMan detects the start of wpa_supplicant
> > > >     automatically.
> >
> > > > +config BR2_PACKAGE_CONNMAN_WIREGUARD
> > > > + bool "enable wireguard support"
> > > > + select BR2_PACKAGE_LIBMNL
> >
> > > We already had a patch from Petr at
> > > http://patchwork.ozlabs.org/patch/1246181/ to add Wireguard support,
> > > but he was also selecting BR2_PACKAGE_WIREGUARD_TOOLS.
> >
> > > Could you or Petr clarify this ?
> > configure.ac
> > found_libmnl="no"
> > if (test "${firewall_type}" = "nftables" -o \
> >               "${enable_wireguard}" != "no"); then
> >       PKG_CHECK_MODULES(LIBMNL, [libmnl >= 1.0.0], [found_libmnl="yes"],
> >               AC_MSG_ERROR([libmnl >= 1.0.0 not found]))
> >       AC_SUBST(LIBMNL_CFLAGS)
> >       AC_SUBST(LIBMNL_LIBS)
> > fi
> > AM_CONDITIONAL(LIBMNL, test "${found_libmnl}" != "no")
>
> Then the "select BR2_PACKAGE_LIBMNL" added to
> BR2_PACKAGE_CONNMAN_NFTABLES is completely unrelated to wireguard
> support. connman needs libmnl if nftables is used *or* if wireguard is
> used.
>
> So the missing select BR2_PACKAGE_LIBMNL in
> BR2_PACKAGE_CONNMAN_NFTABLES should be added as a separate patch.
Sent here already: https://patchwork.ozlabs.org/patch/1251106/
>
> Thanks!
>
> Thomas
> --
> Thomas Petazzoni, CTO, Bootlin
> Embedded Linux and Kernel engineering
> https://bootlin.com
Petr Vorel March 8, 2020, 9:45 p.m. UTC | #8
Hi,

> > Then the "select BR2_PACKAGE_LIBMNL" added to
> > BR2_PACKAGE_CONNMAN_NFTABLES is completely unrelated to wireguard
> > support. connman needs libmnl if nftables is used *or* if wireguard is
> > used.

> > So the missing select BR2_PACKAGE_LIBMNL in
> > BR2_PACKAGE_CONNMAN_NFTABLES should be added as a separate patch.
> Sent here already: https://patchwork.ozlabs.org/patch/1251106/
James, thanks for your fix!
(add my Reviewed-by: tag to it).

Sorry for wrong patch.

Kind regards,
Petr
Thomas Petazzoni March 26, 2020, 8:46 p.m. UTC | #9
On Sun,  8 Mar 2020 00:07:41 -0700
James Hilliard <james.hilliard1@gmail.com> wrote:

> We also need to select libmnl when building with wireguard support or
> with nftables.
> 
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  package/connman/Config.in  | 5 +++++
>  package/connman/connman.mk | 9 ++++++++-
>  2 files changed, 13 insertions(+), 1 deletion(-)

I've applied to master, but only the parts related to wireguard
support, as the rest was applied separately.

Thanks,

Thomas

Patch
diff mbox series

diff --git a/package/connman/Config.in b/package/connman/Config.in
index ac012dda54..614b826f96 100644
--- a/package/connman/Config.in
+++ b/package/connman/Config.in
@@ -33,6 +33,7 @@  config BR2_PACKAGE_CONNMAN_NFTABLES
 	bool "nftables"
 	depends on BR2_USE_WCHAR
 	depends on BR2_TOOLCHAIN_HEADERS_AT_LEAST_3_12
+	select BR2_PACKAGE_LIBMNL
 	select BR2_PACKAGE_NFTABLES
 	help
 	  Use nftables as firewall.
@@ -51,6 +52,10 @@  config BR2_PACKAGE_CONNMAN_WIFI
 	  setup). ConnMan detects the start of wpa_supplicant
 	  automatically.
 
+config BR2_PACKAGE_CONNMAN_WIREGUARD
+	bool "enable wireguard support"
+	select BR2_PACKAGE_LIBMNL
+
 config BR2_PACKAGE_CONNMAN_WISPR
 	bool "enable WISPr support"
 	depends on !BR2_STATIC_LIBS # gnutls
diff --git a/package/connman/connman.mk b/package/connman/connman.mk
index 701be8b59d..23e7cb7797 100644
--- a/package/connman/connman.mk
+++ b/package/connman/connman.mk
@@ -41,7 +41,7 @@  CONNMAN_CONF_OPTS += --with-firewall=iptables
 CONNMAN_DEPENDENCIES += iptables
 else ifeq ($(BR2_PACKAGE_CONNMAN_NFTABLES),y)
 CONNMAN_CONF_OPTS += --with-firewall=nftables
-CONNMAN_DEPENDENCIES += nftables
+CONNMAN_DEPENDENCIES += libmnl nftables
 endif
 
 ifeq ($(BR2_PACKAGE_CONNMAN_LOOPBACK),y)
@@ -70,6 +70,13 @@  else
 CONNMAN_CONF_OPTS += --disable-wifi
 endif
 
+ifeq ($(BR2_PACKAGE_CONNMAN_WIREGUARD),y)
+CONNMAN_CONF_OPTS += --enable-wireguard
+CONNMAN_DEPENDENCIES += libmnl
+else
+CONNMAN_CONF_OPTS += --disable-wireguard
+endif
+
 ifeq ($(BR2_PACKAGE_CONNMAN_WISPR),y)
 CONNMAN_CONF_OPTS += --enable-wispr
 CONNMAN_DEPENDENCIES += gnutls