On Thu, Nov 03, 2022 at 01:03:36AM +0000, Jintao Lin wrote:
> diff --git a/doc/dbus.doxygen b/doc/dbus.doxygen
> + <tr><td>Create</td><td>b</td><td>Whether to create a new interface in the kernel</td><td>No</td>
> + <tr><td>Type</td><td>s</td><td>Interface type to create</td><td>No</td>
Should this list the possible values for the interface type (sta and ap
in the current patch)?
> diff --git a/wpa_supplicant/dbus/dbus_new_handlers.c b/wpa_supplicant/dbus/dbus_new_handlers.c
> @@ -755,6 +755,10 @@ DBusMessage * wpas_dbus_handler_create_interface(DBusMessage *message,
> + char *type = NULL;
> + bool create_iface = false;
> + enum wpa_driver_if_type if_type = WPA_IF_STATION;
> + u8 mac_addr[ETH_ALEN];
Maintaining type and if_type separately seems unnecessarily complex for
this. Wouldn't if_type alone be sufficient to get rid of the dynamic
allocation?
> @@ -791,6 +795,17 @@ DBusMessage * wpas_dbus_handler_create_interface(DBusMessage *message,
> + } else if (os_strcmp(entry.key, "Type") == 0 &&
> + entry.type == DBUS_TYPE_STRING) {
> + os_free(type);
> + type = os_strdup(entry.str_value);
In other words, this part there could do the string to enum
wpa_driver_if_type mapping without storing a copy of the string.
> + if (create_iface) {
> + if (os_strcmp(type, "sta") == 0) {
> + if_type = WPA_IF_STATION;
> + } else if (os_strcmp(type, "ap") == 0) {
> + if_type = WPA_IF_AP_BSS;
> + } else {
> + goto error;
> + }
So this could be moved above.
> reply = dbus_message_new_error(
> message, WPAS_DBUS_ERROR_IFACE_EXISTS,
> "wpa_supplicant already controls this interface.");
> + goto fail;
So if the interface is already present, this would jump to the fail
label.
> @@ -845,6 +884,10 @@ error:
> oom:
> reply = wpas_dbus_error_no_memory(message);
> goto out;
> +fail:
> + if (create_iface)
> + wpa_drv_if_remove(global->ifaces, WPA_IF_STATION, ifname);
> + goto out;
And that would remove the interface that was already in use. That does
not sound correct. Furthermore, that use of the hardcoded WPA_IF_STATION
would not cover the AP case.
> @@ -865,19 +908,35 @@ DBusMessage * wpas_dbus_handler_remove_interface(DBusMessage *message,
> + if (delete_iface) {
> + wpa_printf(MSG_DEBUG, "%s[dbus]: deleting the interface '%s'",
> + __func__, wpa_s->ifname);
> + if (wpa_drv_if_remove(global->ifaces, WPA_IF_STATION, wpa_s->ifname)) {
And same for this hardcoded WPA_IF_STATION case.. This needs to match
the interface type to cover the WPA_IF_AP_BSS specific operations in
wpa_driver_nl80211_if_remove().
--
Jouni Malinen PGP id EFC895FA
_______________________________________________
Hostap mailing list
Hostap@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/hostap
