On Tue, Dec 04, 2018 at 04:40:00PM +0900, Jeonghwan Yoon wrote:
> diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
> @@ -25,6 +25,9 @@
> #include "dbus_new_handlers_p2p.h"
> #include "p2p/p2p.h"
> #include "../p2p_supplicant.h"
> +#ifdef CONFIG_DPP
> +#include "../src/common/dpp.h"
> +#endif
Remove #ifdef CONFIG_DPP and use "common/dpp.h" as the path ("../src" is
on search patch).
> +void wpas_dbus_signal_dpp_gas_query_start(struct wpa_supplicant *wpa_s, const u8 *addr,
> + int dialog_token, unsigned int freq)
> +{
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_DPPDEVICE,
> + "DppGasQueryStart");
What would this DppGasQueryStart signal be used for? This is a good
example of those conformance testing functionality pieces I mentioned
earlier.
> +void wpas_dbus_signal_dpp_confobj_ssid(struct wpa_supplicant *wpa_s, const char *ssid)
SSID is not a string, i.e., it is a binary object that can contain
arbitrary octets, including '\0'. As such, it cannot be used as a C
string and assuming '\0' termination.
> +void wpas_dbus_signal_dpp_confobj_akm(struct wpa_supplicant *wpa_s, int akm)
> + if (akm == DPP_AKM_PSK || akm == DPP_AKM_PSK_SAE || akm == DPP_AKM_SAE) {
> + os_snprintf(akm_string, sizeof(akm_string), "%s", "psk");
> + } else {
> + os_snprintf(akm_string, sizeof(akm_string), "%s", "unknown");
> + }
That looks wrong. PSK, PSK+SAE, and SAE are three different AKMs and
they should not all be exposed as "psk". Exposing "unknown" here looks
quite wrong as well..
> +void wpas_dbus_signal_dpp_wpa_completed(struct wpa_supplicant *wpa_s, const char *ssid)
SSID should not be assumed to be a C string (see above).
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_DPPDEVICE,
> + "DppWpaCompleted");
"DppWpaCompleted" looks a bit confusing name for the signal and I'm not
convinced on this signal being triggered correctly from
wpa_supplicant_set_state() based on wpa_s->dpp_gas_client. There is no
requirement of connecting to the provisioned network (or networks in the
future) immediately after DPP, i.e., wpa_supplicant could decide not to
connect anywhere else (maintain existing association) or connect to a
different network.
> +void wpas_dbus_signal_dpp_conf_req_rx(struct wpa_supplicant *wpa_s, const u8 *addr)
> + msg = dbus_message_new_signal(wpa_s->dbus_new_path,
> + WPAS_DBUS_NEW_IFACE_DPPDEVICE,
> + "DppConfReqRx");
What would this DppConfReqRx signal be used for?
> diff --git a/wpa_supplicant/dpp_supplicant.c b/wpa_supplicant/dpp_supplicant.c
> @@ -1256,6 +1256,7 @@ static void wpas_dpp_process_config(struct wpa_supplicant *wpa_s,
> return;
>
> wpa_msg(wpa_s, MSG_INFO, DPP_EVENT_NETWORK_ID "%d", ssid->id);
> + wpas_notify_dpp_network_id(wpa_s, ssid->id);
It would probably make sense to move all the wpa_msg() calls to the new
wpas_notify_*() functions in this type of cases to clean up
implementation so that all external interfaces would be notified from
notify.c.
> diff --git a/wpa_supplicant/gas_query.c b/wpa_supplicant/gas_query.c
> @@ -159,6 +160,10 @@ static void gas_query_done(struct gas_query *gas,
> " dialog_token=%u freq=%d status_code=%u result=%s",
> MAC2STR(query->addr), query->dialog_token, query->freq,
> query->status_code, gas_result_txt(result));
> +#ifdef CONFIG_DPP
> + wpas_notify_dpp_gas_query_done(gas->wpa_s, query->addr,
> + query->dialog_token, query->freq, query->status_code, gas_result_txt(result));
> +#endif
> @@ -847,6 +852,9 @@ int gas_query_req(struct gas_query *gas, const u8 *dst, int freq,
> wpa_msg(gas->wpa_s, MSG_INFO, GAS_QUERY_START "addr=" MACSTR
> " dialog_token=%u freq=%d",
> MAC2STR(query->addr), query->dialog_token, query->freq);
> +#ifdef CONFIG_DPP
> + wpas_notify_dpp_gas_query_start(gas->wpa_s, query->addr, query->dialog_token, query->freq);
> +#endif
These do not sound like events that I would expose over D-Bus unless
there is a clearly identifiable use case for them. Please also note that
neither of these gas_query location is specific to DPP, so D-Bus should
not claim them to be DPP signals either.
> diff --git a/wpa_supplicant/wpa_supplicant.c b/wpa_supplicant/wpa_supplicant.c
> @@ -905,6 +905,10 @@ void wpa_supplicant_set_state(struct wpa_supplicant *wpa_s,
> ssid && ssid->id_str ? ssid->id_str : "",
> fils_hlp_sent ? " FILS_HLP_SENT" : "");
> #endif /* CONFIG_CTRL_IFACE || !CONFIG_NO_STDOUT_DEBUG */
> +#ifdef CONFIG_DPP
> + if(wpa_s->dpp_gas_client)
> + wpas_notify_dpp_wpa_completed(wpa_s, ssid->ssid);
> +#endif
I had a comment related to this above, i.e., I'm not convinced of this
being appropriate assumption about completion of DPP items. The
connection after completion of DPP may or may not be there and it may be
with the profile provisioned over DPP or something else.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
