[PATCH] CHROMIUM: dbus: Add virtual interface create/remove logic to be inline with ctrl_iface

[Jouni Malinen]

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