[1/3] package/hostapd: enable Linux ioctls for VLANs
diff mbox series

Message ID 20190928173632.21119-2-geomatsi@gmail.com
State Accepted
Headers show
Series
  • Fix hostapd v2.9 build issues
Related show

Commit Message

Sergey Matyukevich Sept. 28, 2019, 5:36 p.m. UTC
Hostapd v2.9 follows the approach taken by bridge-utils and attempts
to use new bridge ioctls whenever possible. New bridge calls are
enabled in hostapd build using NEED_LINUX_IOCTL configuration option.
That switch is enabled for all the practical configurations excluding
wired driver. However it is required to support dynamic VLANs in
any configuration.

Enable NEED_LINUX_IOCTL hostapd configuration option whenever
dynamic VLANs support is requested in buildroot.

Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
---
 package/hostapd/hostapd.mk | 1 +
 1 file changed, 1 insertion(+)

Comments

Matthew Weber Sept. 30, 2019, 2:28 p.m. UTC | #1
Sergey,

On Sat, Sep 28, 2019 at 12:34 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
>
> Hostapd v2.9 follows the approach taken by bridge-utils and attempts
> to use new bridge ioctls whenever possible. New bridge calls are
> enabled in hostapd build using NEED_LINUX_IOCTL configuration option.
> That switch is enabled for all the practical configurations excluding
> wired driver. However it is required to support dynamic VLANs in
> any configuration.
>
> Enable NEED_LINUX_IOCTL hostapd configuration option whenever
> dynamic VLANs support is requested in buildroot.
>
> Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> ---
>  package/hostapd/hostapd.mk | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/package/hostapd/hostapd.mk b/package/hostapd/hostapd.mk
> index 4a493cb9b2..0e5e5b2962 100644
> --- a/package/hostapd/hostapd.mk
> +++ b/package/hostapd/hostapd.mk
> @@ -96,6 +96,7 @@ endif
>
>  ifeq ($(BR2_PACKAGE_HOSTAPD_VLAN_DYNAMIC),y)
>  HOSTAPD_CONFIG_ENABLE += CONFIG_FULL_DYNAMIC_VLAN
> +HOSTAPD_CONFIG_SET += NEED_LINUX_IOCTL

It seems interesting that CONFIG_FULL_DYNAMIC_VLAN doesn't set this
withing the Makefile of hostapd

>  endif
>
>  ifeq ($(BR2_PACKAGE_HOSTAPD_VLAN_NETLINK),y)
> --
> 2.23.0
>
Sergey Matyukevich Sept. 30, 2019, 3:20 p.m. UTC | #2
Hello Matt,

> Sergey,
>
> On Sat, Sep 28, 2019 at 12:34 PM Sergey Matyukevich <geomatsi@gmail.com> wrote:
> >
> > Hostapd v2.9 follows the approach taken by bridge-utils and attempts
> > to use new bridge ioctls whenever possible. New bridge calls are
> > enabled in hostapd build using NEED_LINUX_IOCTL configuration option.
> > That switch is enabled for all the practical configurations excluding
> > wired driver. However it is required to support dynamic VLANs in
> > any configuration.
> >
> > Enable NEED_LINUX_IOCTL hostapd configuration option whenever
> > dynamic VLANs support is requested in buildroot.
> >
> > Signed-off-by: Sergey Matyukevich <geomatsi@gmail.com>
> > ---
> >  package/hostapd/hostapd.mk | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/package/hostapd/hostapd.mk b/package/hostapd/hostapd.mk
> > index 4a493cb9b2..0e5e5b2962 100644
> > --- a/package/hostapd/hostapd.mk
> > +++ b/package/hostapd/hostapd.mk
> > @@ -96,6 +96,7 @@ endif
> >
> >  ifeq ($(BR2_PACKAGE_HOSTAPD_VLAN_DYNAMIC),y)
> >  HOSTAPD_CONFIG_ENABLE += CONFIG_FULL_DYNAMIC_VLAN
> > +HOSTAPD_CONFIG_SET += NEED_LINUX_IOCTL
>
> It seems interesting that CONFIG_FULL_DYNAMIC_VLAN doesn't set this
> withing the Makefile of hostapd

The NEED_LINUX_IOCTL option is set in driver make files, e.g. see
src/drivers/drivers.mk.
I assume it is done so in order to enable this option only for Linux
drivers. For instance, this
option is not set for wired hostapd driver which is platform agnostic.
In the case of Buildroot,
only Linux matters, so it is ok to set NEED_LINUX_IOCTL whenever we
need to support
dynamic VLANs.

But your remark is a good one anyway. Unless I am missing something,
support of dynamic
VLANs in hostapd is Linux only anyway. So it would make sense to
enable NEED_LINUX_IOCTL
in src/drivers/drivers.mk whenever CONFIG_FULL_DYNAMIC_VLAN is
selected in hostapd config.

Regards,
Sergey
Sergey Matyukevich Oct. 1, 2019, 7:44 a.m. UTC | #3
+ buildroot

> >>> Hostapd v2.9 follows the approach taken by bridge-utils and attempts
> >>> to use new bridge ioctls whenever possible. New bridge calls are
> >>> enabled in hostapd build using NEED_LINUX_IOCTL configuration option.
> >>> That switch is enabled for all the practical configurations excluding
> >>> wired driver. However it is required to support dynamic VLANs in
> >>> any configuration.
> >>>
> >>> Enable NEED_LINUX_IOCTL hostapd configuration option whenever
> >>> dynamic VLANs support is requested in buildroot.

...

> >> It seems interesting that CONFIG_FULL_DYNAMIC_VLAN doesn't set this
> >> withing the Makefile of hostapd
> > 
> > The NEED_LINUX_IOCTL option is set in driver make files, e.g. see
> > src/drivers/drivers.mk.
> > I assume it is done so in order to enable this option only for Linux
> > drivers. For instance, this
> > option is not set for wired hostapd driver which is platform agnostic.
> > In the case of Buildroot,
> > only Linux matters, so it is ok to set NEED_LINUX_IOCTL whenever we
> > need to support
> > dynamic VLANs.
> > 
> > But your remark is a good one anyway. Unless I am missing something,
> > support of dynamic
> > VLANs in hostapd is Linux only anyway. So it would make sense to
> > enable NEED_LINUX_IOCTL
> > in src/drivers/drivers.mk whenever CONFIG_FULL_DYNAMIC_VLAN is
> > selected in hostapd config.
> 
>  Indeed, since vlan_full.c includes linux_ioctl, it makes sense to enable that
> in drivers.mk. Could you send a patch doing that upstream?

Sure, will do. But hostapd upstreaming is going to take some time.
As for Buildroot, I have two options for this patch then:
- to keep this fix until next hostapd version update 
- drop this fix and add hostapd patch to packages/hostapd directory

Any preferences ?

Regards,
Sergey
Arnout Vandecappelle Oct. 11, 2019, 9:41 p.m. UTC | #4
On 01/10/2019 09:44, Sergey Matyukevich wrote:
> + buildroot
> 
>>>>> Hostapd v2.9 follows the approach taken by bridge-utils and attempts
>>>>> to use new bridge ioctls whenever possible. New bridge calls are
>>>>> enabled in hostapd build using NEED_LINUX_IOCTL configuration option.
>>>>> That switch is enabled for all the practical configurations excluding
>>>>> wired driver. However it is required to support dynamic VLANs in
>>>>> any configuration.
>>>>>
>>>>> Enable NEED_LINUX_IOCTL hostapd configuration option whenever
>>>>> dynamic VLANs support is requested in buildroot.
> 
> ...
> 
>>>> It seems interesting that CONFIG_FULL_DYNAMIC_VLAN doesn't set this
>>>> withing the Makefile of hostapd
>>>
>>> The NEED_LINUX_IOCTL option is set in driver make files, e.g. see
>>> src/drivers/drivers.mk.
>>> I assume it is done so in order to enable this option only for Linux
>>> drivers. For instance, this
>>> option is not set for wired hostapd driver which is platform agnostic.
>>> In the case of Buildroot,
>>> only Linux matters, so it is ok to set NEED_LINUX_IOCTL whenever we
>>> need to support
>>> dynamic VLANs.
>>>
>>> But your remark is a good one anyway. Unless I am missing something,
>>> support of dynamic
>>> VLANs in hostapd is Linux only anyway. So it would make sense to
>>> enable NEED_LINUX_IOCTL
>>> in src/drivers/drivers.mk whenever CONFIG_FULL_DYNAMIC_VLAN is
>>> selected in hostapd config.
>>
>>  Indeed, since vlan_full.c includes linux_ioctl, it makes sense to enable that
>> in drivers.mk. Could you send a patch doing that upstream?
> 
> Sure, will do. But hostapd upstreaming is going to take some time.
> As for Buildroot, I have two options for this patch then:
> - to keep this fix until next hostapd version update 
> - drop this fix and add hostapd patch to packages/hostapd directory

 I mulled over this for some time before finally applying the patch as is.

 Please do send a proper fix to upstream hostapd. When the version is bumped we
can remove the current fix again. And if we forget, it's not the end of the world.

 Regards,
 Arnout

> 
> Any preferences ?
> 
> Regards,
> Sergey
>
Sergey Matyukevich Oct. 12, 2019, 7:27 a.m. UTC | #5
....

> >>  Indeed, since vlan_full.c includes linux_ioctl, it makes sense to enable that
> >> in drivers.mk. Could you send a patch doing that upstream?
> >
> > Sure, will do. But hostapd upstreaming is going to take some time.
> > As for Buildroot, I have two options for this patch then:
> > - to keep this fix until next hostapd version update
> > - drop this fix and add hostapd patch to packages/hostapd directory
>
>  I mulled over this for some time before finally applying the patch as is.
>
>  Please do send a proper fix to upstream hostapd. When the version is bumped we
> can remove the current fix again. And if we forget, it's not the end of the world.

Patch has been already sent to hostapd mailing list. When it is accepted,
I will replace current solution in hostapd package makefile with a separate
patch for hostapd. In this caseuild failure will not let us to forget about it
when we bump hostapd version next time.

Regards,
Sergey

Patch
diff mbox series

diff --git a/package/hostapd/hostapd.mk b/package/hostapd/hostapd.mk
index 4a493cb9b2..0e5e5b2962 100644
--- a/package/hostapd/hostapd.mk
+++ b/package/hostapd/hostapd.mk
@@ -96,6 +96,7 @@  endif
 
 ifeq ($(BR2_PACKAGE_HOSTAPD_VLAN_DYNAMIC),y)
 HOSTAPD_CONFIG_ENABLE += CONFIG_FULL_DYNAMIC_VLAN
+HOSTAPD_CONFIG_SET += NEED_LINUX_IOCTL
 endif
 
 ifeq ($(BR2_PACKAGE_HOSTAPD_VLAN_NETLINK),y)