| Message ID | 1347525267-4536-2-git-send-email-s.neumann@raumfeld.com |
|---|---|
| State | Superseded |
| Headers | show |
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 >
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
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
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
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
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(-)