Patchwork wpa-supplicant: add options to enable the new DBus interface

login
register
mail settings
Submitter Sven Neumann
Date Sept. 13, 2012, 8:34 a.m.
Message ID <1347525267-4536-2-git-send-email-s.neumann@raumfeld.com>
Download mbox | patch
Permalink /patch/183549/
State Superseded
Headers show

Comments

Sven Neumann - Sept. 13, 2012, 8:34 a.m.
Allow to configure the DBus interfaces that the wpa_supplicant
binary should support (old or new or both). Also allow to
enable introspection support on the new DBus interface.

Signed-off-by: Sven Neumann <s.neumann@raumfeld.com>
---
 package/wpa_supplicant/Config.in         |   19 ++++++++++++++
 package/wpa_supplicant/wpa_supplicant.mk |   42 ++++++++++++++++++++++++++----
 2 files changed, 56 insertions(+), 5 deletions(-)
Arnout Vandecappelle - Sept. 18, 2012, 10:04 p.m.
On 09/13/12 10:34, Sven Neumann wrote:
> Allow to configure the DBus interfaces that the wpa_supplicant
> binary should support (old or new or both). Also allow to
> enable introspection support on the new DBus interface.
>
> Signed-off-by: Sven Neumann<s.neumann@raumfeld.com>

   Since nobody is reviewing it even after your resend, I'll do it.
However, I'm not familiar with wpa_supplicant and I don't have
internet access now, so I may miss some things.

  I have a few comments, but nothing major (the spaces are the
only thing that really have to be fixed). So

Acked-by: Arnout Vandecappelle (Essensium/Mind) <arnout@mind.be>

> ---
>   package/wpa_supplicant/Config.in         |   19 ++++++++++++++
>   package/wpa_supplicant/wpa_supplicant.mk |   42 ++++++++++++++++++++++++++----
>   2 files changed, 56 insertions(+), 5 deletions(-)
>
> diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
> index d7cefe3..d41913e 100644
> --- a/package/wpa_supplicant/Config.in
> +++ b/package/wpa_supplicant/Config.in
> @@ -30,6 +30,25 @@ config BR2_PACKAGE_WPA_SUPPLICANT_AP_SUPPORT
>   	  (optionally, with WPS); this links in parts of hostapd functionality
>   	  into wpa_supplicant.
>
> +config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD
> +	bool "Enable support for old DBus control interface"
> +        depends on BR2_PACKAGE_DBUS

   Use 1 tab instead of 8 spaces (below as well).

  Also, I think it's better to make this
     select BR2_PACKAGE_DBUS
     depends on BR2_TOOLCHAIN_HAS_THREADS

  Also, since this only installs a .service file, and it used to be
always enabled before, I would make at least one of the two
DBus interfaces enabled by default.

> +	help
> +	  Enable support for old DBus control interface
> +	  (fi.epitest.hostap.WPASupplicant).
> +
> +config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_NEW
> +	bool "Enable support for new DBus control interface"
> +        depends on BR2_PACKAGE_DBUS
> +	help
> +	  Enable support for new DBus control interface (fi.w1.wpa_supplicant1).
> +
> +config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_INTROSPECTION
> +	bool "Introspection support for new DBus control interface"

  Just write 'Introspection support'. This config will be indented
under the 'new DBus control interface', so that's clear enough.

> +        depends on BR2_PACKAGE_WPA_SUPPLICANT_DBUS_NEW
> +	help
> +	  Add introspection support for new DBus control interface.
> +
>   config BR2_PACKAGE_WPA_SUPPLICANT_WPS
>   	bool "Enable support for WPS"
>   	help
> diff --git a/package/wpa_supplicant/wpa_supplicant.mk b/package/wpa_supplicant/wpa_supplicant.mk
> index d8c916b..207eec1 100644
> --- a/package/wpa_supplicant/wpa_supplicant.mk
> +++ b/package/wpa_supplicant/wpa_supplicant.mk
> @@ -10,7 +10,8 @@ WPA_SUPPLICANT_LICENSE = GPLv2/BSD-3c
>   WPA_SUPPLICANT_LICENSE_FILES = README
>   WPA_SUPPLICANT_CONFIG = $(WPA_SUPPLICANT_DIR)/wpa_supplicant/.config
>   WPA_SUPPLICANT_SUBDIR = wpa_supplicant
> -WPA_SUPPLICANT_DBUS_SERVICE = fi.epitest.hostap.WPASupplicant
> +WPA_SUPPLICANT_DBUS_OLD_SERVICE = fi.epitest.hostap.WPASupplicant
> +WPA_SUPPLICANT_DBUS_NEW_SERVICE = fi.w1.wpa_supplicant1
>   WPA_SUPPLICANT_CFLAGS = $(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/libnl3/
>   WPA_SUPPLICANT_LDFLAGS = $(TARGET_LDFLAGS)
>
> @@ -84,9 +85,41 @@ ifeq ($(BR2_PACKAGE_DBUS),y)
>   	WPA_SUPPLICANT_MAKE_ENV = \
>   		PKG_CONFIG_SYSROOT_DIR="$(STAGING_DIR)"	\
>   		PKG_CONFIG_PATH="$(STAGING_DIR)/usr/lib/pkgconfig"
> -define WPA_SUPPLICANT_DBUS_CONFIG
> +
> +ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD),y)
> +define WPA_SUPPLICANT_DBUS_OLD_CONFIG
>   	$(SED) 's/\(#\)\(CONFIG_CTRL_IFACE_DBUS=\)/\2/' $(WPA_SUPPLICANT_CONFIG)

  The \(\) around the # are redundant.

  Since there are a few sed scripts here, it would be nicer to write:

ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD),y)
WPA_SUPPLICANT_CONFIG_EDIT += -e 's/#\(CONFIG_CTRL_IFACE_DBUS=\)/\1/'
define WPA_SUPPLICANT_INSTALL_DBUS_OLD
...
endef
endif

...

ifneq ($(WPA_SUPPLICANT_CONFIG_EDIT),)
define WPA_SUPPLICANT_DBUS_CONFIG
     $(SED) $(WPA_SUPPLICANT_CONFIG_EDIT) $(WPA_SUPPLICANT_CONFIG)
endef
endif



  Regards,
  Arnout

>   endef
> +define WPA_SUPPLICANT_INSTALL_DBUS_OLD
> +	$(INSTALL) -D \
> +	  $(@D)/wpa_supplicant/dbus/$(WPA_SUPPLICANT_DBUS_OLD_SERVICE).service \
> +	  $(TARGET_DIR)/usr/share/dbus-1/system-services/$(WPA_SUPPLICANT_DBUS_OLD_SERVICE).service
> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_NEW),y)
> +define WPA_SUPPLICANT_DBUS_NEW_CONFIG
> +	$(SED) 's/\(#\)\(CONFIG_CTRL_IFACE_DBUS_NEW=\)/\2/' $(WPA_SUPPLICANT_CONFIG)
> +endef
> +define WPA_SUPPLICANT_INSTALL_DBUS_NEW
> +	$(INSTALL) -D \
> +	  $(@D)/wpa_supplicant/dbus/$(WPA_SUPPLICANT_DBUS_NEW_SERVICE).service \
> +	  $(TARGET_DIR)/usr/share/dbus-1/system-services/$(WPA_SUPPLICANT_DBUS_NEW_SERVICE).service
> +endef
> +endif
> +
> +ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_INTROSPECTION),y)
> +define WPA_SUPPLICANT_DBUS_INTROSPECTION_CONFIG
> +	$(SED) 's/\(#\)\(CONFIG_CTRL_IFACE_DBUS_INTRO=\)/\2/' $(WPA_SUPPLICANT_CONFIG)
> +endef
> +endif
> +
> +define WPA_SUPPLICANT_DBUS_CONFIG
> +	$(WPA_SUPPLICANT_DBUS_OLD_CONFIG)
> +	$(WPA_SUPPLICANT_DBUS_NEW_CONFIG)
> +	$(WPA_SUPPLICANT_DBUS_INTROSPECTION_CONFIG)
> +endef
> +
>   endif
>
>   define WPA_SUPPLICANT_CONFIGURE_CMDS
> @@ -131,9 +164,8 @@ define WPA_SUPPLICANT_INSTALL_DBUS
>   	$(INSTALL) -D \
>   	  $(@D)/wpa_supplicant/dbus/dbus-wpa_supplicant.conf \
>   	  $(TARGET_DIR)/etc/dbus-1/system.d/wpa_supplicant.conf
> -	$(INSTALL) -D \
> -	  $(@D)/wpa_supplicant/dbus/$(WPA_SUPPLICANT_DBUS_SERVICE).service \
> -	  $(TARGET_DIR)/usr/share/dbus-1/system-services/$(WPA_SUPPLICANT_DBUS_SERVICE).service
> +	$(WPA_SUPPLICANT_INSTALL_DBUS_OLD)
> +	$(WPA_SUPPLICANT_INSTALL_DBUS_NEW)
>   endef
>   endif
>
Sven Neumann - Sept. 19, 2012, 9:02 a.m.
Hi,

thanks a lot for your review. I'll make the changes you suggested, but I
am not sure about the following:

On Tue, 2012-09-18 at 08:12 +0200, Arnout Vandecappelle wrote:

> > +config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD
> > +	bool "Enable support for old DBus control interface"
> > +        depends on BR2_PACKAGE_DBUS
> 
>    Use 1 tab instead of 8 spaces (below as well).
> 
>   Also, I think it's better to make this
>      select BR2_PACKAGE_DBUS
>      depends on BR2_TOOLCHAIN_HAS_THREADS
> 
>   Also, since this only installs a .service file, and it used to be
> always enabled before, I would make at least one of the two
> DBus interfaces enabled by default.

If I follow this suggestion and enable one of the DBus interfaces by
default, then selecting wpa-supplicant would select DBus, which it did
not do before. Is that really desired?


Regards,
Sven
Sven Neumann - Sept. 19, 2012, 10:11 a.m.
Hi,

here's a new patch series based on comments from Arnout. The first
patch is the old one with whitespace issues fixed. The second
patch removes some redundant escaping from the various sed commands
in wpa_supplicant.mk. The third patch tries to make the whole thing
more readable by introducing macros for editing the .config file.
Please review and comment.


Regards, Sven
Arnout Vandecappelle - Sept. 19, 2012, 8:03 p.m.
On 09/19/12 11:02, Sven Neumann wrote:
> Hi,
>
> thanks a lot for your review. I'll make the changes you suggested, but I
> am not sure about the following:
>
> On Tue, 2012-09-18 at 08:12 +0200, Arnout Vandecappelle wrote:
>
>>> +config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD
>>> +	bool "Enable support for old DBus control interface"
>>> +        depends on BR2_PACKAGE_DBUS
>>
>>     Use 1 tab instead of 8 spaces (below as well).
>>
>>    Also, I think it's better to make this
>>       select BR2_PACKAGE_DBUS
>>       depends on BR2_TOOLCHAIN_HAS_THREADS
>>
>>    Also, since this only installs a .service file, and it used to be
>> always enabled before, I would make at least one of the two
>> DBus interfaces enabled by default.
>
> If I follow this suggestion and enable one of the DBus interfaces by
> default, then selecting wpa-supplicant would select DBus, which it did
> not do before. Is that really desired?

  Actually, on second thought, it makes more sense to depend on dbus
after all: .service files are installed if dbus is enabled.  That's also
the old behaviour.

  Regards,
  Arnout

Patch

diff --git a/package/wpa_supplicant/Config.in b/package/wpa_supplicant/Config.in
index d7cefe3..d41913e 100644
--- a/package/wpa_supplicant/Config.in
+++ b/package/wpa_supplicant/Config.in
@@ -30,6 +30,25 @@  config BR2_PACKAGE_WPA_SUPPLICANT_AP_SUPPORT
 	  (optionally, with WPS); this links in parts of hostapd functionality
 	  into wpa_supplicant.
 
+config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD
+	bool "Enable support for old DBus control interface"
+        depends on BR2_PACKAGE_DBUS
+	help
+	  Enable support for old DBus control interface
+	  (fi.epitest.hostap.WPASupplicant).
+
+config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_NEW
+	bool "Enable support for new DBus control interface"
+        depends on BR2_PACKAGE_DBUS
+	help
+	  Enable support for new DBus control interface (fi.w1.wpa_supplicant1).
+
+config BR2_PACKAGE_WPA_SUPPLICANT_DBUS_INTROSPECTION
+	bool "Introspection support for new DBus control interface"
+        depends on BR2_PACKAGE_WPA_SUPPLICANT_DBUS_NEW
+	help
+	  Add introspection support for new DBus control interface.
+
 config BR2_PACKAGE_WPA_SUPPLICANT_WPS
 	bool "Enable support for WPS"
 	help
diff --git a/package/wpa_supplicant/wpa_supplicant.mk b/package/wpa_supplicant/wpa_supplicant.mk
index d8c916b..207eec1 100644
--- a/package/wpa_supplicant/wpa_supplicant.mk
+++ b/package/wpa_supplicant/wpa_supplicant.mk
@@ -10,7 +10,8 @@  WPA_SUPPLICANT_LICENSE = GPLv2/BSD-3c
 WPA_SUPPLICANT_LICENSE_FILES = README
 WPA_SUPPLICANT_CONFIG = $(WPA_SUPPLICANT_DIR)/wpa_supplicant/.config
 WPA_SUPPLICANT_SUBDIR = wpa_supplicant
-WPA_SUPPLICANT_DBUS_SERVICE = fi.epitest.hostap.WPASupplicant
+WPA_SUPPLICANT_DBUS_OLD_SERVICE = fi.epitest.hostap.WPASupplicant
+WPA_SUPPLICANT_DBUS_NEW_SERVICE = fi.w1.wpa_supplicant1
 WPA_SUPPLICANT_CFLAGS = $(TARGET_CFLAGS) -I$(STAGING_DIR)/usr/include/libnl3/
 WPA_SUPPLICANT_LDFLAGS = $(TARGET_LDFLAGS)
 
@@ -84,9 +85,41 @@  ifeq ($(BR2_PACKAGE_DBUS),y)
 	WPA_SUPPLICANT_MAKE_ENV = \
 		PKG_CONFIG_SYSROOT_DIR="$(STAGING_DIR)"	\
 		PKG_CONFIG_PATH="$(STAGING_DIR)/usr/lib/pkgconfig"
-define WPA_SUPPLICANT_DBUS_CONFIG
+
+ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_OLD),y)
+define WPA_SUPPLICANT_DBUS_OLD_CONFIG
 	$(SED) 's/\(#\)\(CONFIG_CTRL_IFACE_DBUS=\)/\2/' $(WPA_SUPPLICANT_CONFIG)
 endef
+define WPA_SUPPLICANT_INSTALL_DBUS_OLD
+	$(INSTALL) -D \
+	  $(@D)/wpa_supplicant/dbus/$(WPA_SUPPLICANT_DBUS_OLD_SERVICE).service \
+	  $(TARGET_DIR)/usr/share/dbus-1/system-services/$(WPA_SUPPLICANT_DBUS_OLD_SERVICE).service
+endef
+endif
+
+ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_NEW),y)
+define WPA_SUPPLICANT_DBUS_NEW_CONFIG
+	$(SED) 's/\(#\)\(CONFIG_CTRL_IFACE_DBUS_NEW=\)/\2/' $(WPA_SUPPLICANT_CONFIG)
+endef
+define WPA_SUPPLICANT_INSTALL_DBUS_NEW
+	$(INSTALL) -D \
+	  $(@D)/wpa_supplicant/dbus/$(WPA_SUPPLICANT_DBUS_NEW_SERVICE).service \
+	  $(TARGET_DIR)/usr/share/dbus-1/system-services/$(WPA_SUPPLICANT_DBUS_NEW_SERVICE).service
+endef
+endif
+
+ifeq ($(BR2_PACKAGE_WPA_SUPPLICANT_DBUS_INTROSPECTION),y)
+define WPA_SUPPLICANT_DBUS_INTROSPECTION_CONFIG
+	$(SED) 's/\(#\)\(CONFIG_CTRL_IFACE_DBUS_INTRO=\)/\2/' $(WPA_SUPPLICANT_CONFIG)
+endef
+endif
+
+define WPA_SUPPLICANT_DBUS_CONFIG
+	$(WPA_SUPPLICANT_DBUS_OLD_CONFIG)
+	$(WPA_SUPPLICANT_DBUS_NEW_CONFIG)
+	$(WPA_SUPPLICANT_DBUS_INTROSPECTION_CONFIG)
+endef
+
 endif
 
 define WPA_SUPPLICANT_CONFIGURE_CMDS
@@ -131,9 +164,8 @@  define WPA_SUPPLICANT_INSTALL_DBUS
 	$(INSTALL) -D \
 	  $(@D)/wpa_supplicant/dbus/dbus-wpa_supplicant.conf \
 	  $(TARGET_DIR)/etc/dbus-1/system.d/wpa_supplicant.conf
-	$(INSTALL) -D \
-	  $(@D)/wpa_supplicant/dbus/$(WPA_SUPPLICANT_DBUS_SERVICE).service \
-	  $(TARGET_DIR)/usr/share/dbus-1/system-services/$(WPA_SUPPLICANT_DBUS_SERVICE).service
+	$(WPA_SUPPLICANT_INSTALL_DBUS_OLD)
+	$(WPA_SUPPLICANT_INSTALL_DBUS_NEW)
 endef
 endif