On Tue, Dec 04, 2018 at 04:39:58PM +0900, Jeonghwan Yoon wrote:
> diff --git a/wpa_supplicant/dbus/dbus_new.c b/wpa_supplicant/dbus/dbus_new.c
> @@ -3278,6 +3278,67 @@ static const struct wpa_dbus_method_desc wpas_dbus_interface_methods[] = {
> +#ifdef CONFIG_DPP
> + { "ConfiguratorParams", WPAS_DBUS_NEW_IFACE_DPPDEVICE,
> + (WPADBusMethodHandler)wpas_dbus_handler_dpp_configurator_params,
> + {
> + { "cmd","s",ARG_IN},
Please try to follow the style with a space after the commas:
{ "cmd", "s", ARG_ING }
> + { "BootstrapGen", WPAS_DBUS_NEW_IFACE_DPPDEVICE,
> + (WPADBusMethodHandler)wpas_dbus_handler_dpp_bootstrap_gen,
> + {
> + { "type", "s", ARG_IN },
Like here..
> + { "own_id","i",ARG_OUT},
But consistently for everything..
> diff --git a/wpa_supplicant/dbus/dbus_new.h b/wpa_supplicant/dbus/dbus_new.h
> @@ -65,6 +65,10 @@ enum wpas_dbus_bss_prop {
> WPAS_DBUS_NEW_IFACE_INTERFACE ".P2PDevice"
>
> #define WPAS_DBUS_NEW_IFACE_MESH WPAS_DBUS_NEW_IFACE_INTERFACE ".Mesh"
> +#ifdef CONFIG_DPP
> +#define WPAS_DBUS_NEW_IFACE_DPPDEVICE \
> + WPAS_DBUS_NEW_IFACE_INTERFACE ".DPPDevice"
> +#endif /* CONFIG_DPP */
No need for adding #ifdef CONFIG_DPP in header files around this type of
defines since they do not result in any difference in the compiled
binary.
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
> @@ -32,6 +32,10 @@
> #include "ap/hostapd.h"
> #include "ap/sta_info.h"
> #endif /* CONFIG_MESH */
> +#ifdef CONFIG_DPP
> +#include "../ctrl_iface.h"
> +#include "../dpp_supplicant.h"
> +#endif
No #ifdef CONFIG_DPP here either (that CONFIG_MESH example above is not
a good example to follow..).
Please do not include ../ctrl_iface.h into dbus/*. Instead, the needed
functionality from ctrl_iface.h/c should be moved into other files.
Though, in this particular case, I'm not sure why this would use the
control interface functionality in the first place.
> +DBusMessage * wpas_dbus_handler_dpp_configurator_params(DBusMessage *message,
> + struct wpa_supplicant *wpa_s){
'{' on its own line in functions.
> + DBusMessage *reply = NULL;
> + const char* cmd;
"char *cmd"
> + char cmd_buf[256];
> + char *res = NULL;
> + size_t resp_len;
> +
> + dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &cmd,
> + DBUS_TYPE_INVALID);
> + snprintf(cmd_buf, sizeof(cmd_buf), "SET dpp_configurator_params %s", cmd);
> + res = wpa_supplicant_ctrl_iface_process(wpa_s, cmd_buf, &resp_len);
This looks pretty inconvenient. Please do not use
wpa_supplicant_ctrl_iface_process(). Instead, update
wpa_s->dpp_configurator_params here just like
wpa_supplicant_ctrl_iface_set does. That is actually less code than
what's here now..
> + res[resp_len] = '\0';
> + reply = dbus_message_new_method_return(message);
> +
> + if ((resp_len != 3) || (os_strncmp(res, "OK\n", 3) != 0))
> + reply = wpas_dbus_error_iface_unknown(message);
Wouldn't that leak resources by overriding the previously assigned reply
value? And should this error case really continue to the next line?
> + if (!dbus_message_append_args(reply,DBUS_TYPE_STRING,&res,DBUS_TYPE_INVALID)){
What is the point of returning the control interface response string
("OK\n" or "FAIL\n") over D-Bus?
> +DBusMessage * wpas_dbus_handler_dpp_bootstrap_gen(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + DBusMessage *reply = NULL;
> + const char *type;
> + int id;
> +
> + dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &type,
> + DBUS_TYPE_INVALID);
> +
> + id = wpas_dpp_bootstrap_gen(wpa_s, type);
> + reply = dbus_message_new_method_return(message);
> + if (!reply)
> + return wpas_dbus_error_no_memory(message);
> +
> + if (id == -1)
> + return wpas_dbus_error_iface_unknown(message);
Wouldn't this leak memory (reply assigned above)?
> +DBusMessage * wpas_dbus_handler_dpp_bootstrap_remove(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + DBusMessage *reply = NULL;
> + const char *id;
> + int result;
> +
> + dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &id,
> + DBUS_TYPE_INVALID);
> +
> + result = wpas_dpp_bootstrap_remove(wpa_s, id);
> + reply = dbus_message_new_method_return(message);
> + if (!reply)
> + return wpas_dbus_error_no_memory(message);
> +
> + if (result == -1)
> + return wpas_dbus_error_iface_unknown(message);
Same here.
> +DBusMessage * wpas_dbus_handler_dpp_pkex_add(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + DBusMessage *reply = NULL;
> + const char *cmd;
> + int result;
> +
> + dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &cmd,
> + DBUS_TYPE_INVALID);
> +
> + result = wpas_dpp_pkex_add(wpa_s, cmd);
> + reply = dbus_message_new_method_return(message);
> + if (!reply)
> + return wpas_dbus_error_no_memory(message);
> +
> + if (result == -1)
> + return wpas_dbus_error_iface_unknown(message);
And here.
> +DBusMessage * wpas_dbus_handler_dpp_pkex_remove(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + DBusMessage *reply = NULL;
> + const char *id;
> + int result;
> +
> + dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &id,
> + DBUS_TYPE_INVALID);
> +
> + result = wpas_dpp_pkex_remove(wpa_s, id);
> + reply = dbus_message_new_method_return(message);
> + if (!reply)
> + return wpas_dbus_error_no_memory(message);
> +
> + if (result == -1)
> + return wpas_dbus_error_iface_unknown(message);
And here.
> +DBusMessage * wpas_dbus_handler_dpp_listen(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + DBusMessage *reply = NULL;
> + const char* cmd;
> + int result;
> +
> + dbus_message_get_args(message, NULL, DBUS_TYPE_STRING, &cmd,
> + DBUS_TYPE_INVALID);
> +
> + result = wpas_dpp_listen(wpa_s, cmd);
> + reply = dbus_message_new_method_return(message);
> + if (!reply)
> + return wpas_dbus_error_no_memory(message);
> +
> + if (result == -1)
> + return wpas_dbus_error_iface_unknown(message);
And here.
> +DBusMessage * wpas_dbus_handler_dpp_listen_stop(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + wpas_dpp_stop(wpa_s);
> + wpas_dpp_listen_stop(wpa_s);
> +
> + return NULL;
Is it appropriate to return NULL on success?
> +DBusMessage * wpas_dbus_handler_dpp_configurator_add(DBusMessage *message,
> + struct wpa_supplicant *wpa_s)
> +{
> + DBusMessage *reply = NULL;
> + const char *cmd = "";
> + int result = -1;
> +
> + result = wpas_dpp_configurator_add(wpa_s, cmd);
> + reply = dbus_message_new_method_return(message);
> + if (!reply)
> + return wpas_dbus_error_no_memory(message);
> +
> + if (result == -1)
> + return wpas_dbus_error_iface_unknown(message);
Resource leak (reply)..
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.h b/wpa_supplicant/dbus/dbus_new_handlers.h
> +#ifdef CONFIG_DPP
No need for #ifdef CONFIG_DPP for these.
> +DBusMessage * wpas_dbus_handler_dpp_configurator_params(
> + DBusMessage *message,struct wpa_supplicant *wpa_s);
Space after comma.
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
