[PATCH v2 1/2] hwsim: allow concurrent radio creations

[PATCH v2 1/2] hwsim: allow concurrent radio creations

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 9549 bytes --]

Currently CreateRadio only allows a single outstanding DBus message
until the radio is fully created. 99% of the time this is just fine
but in order to test dual phy cards there needs to be support for
phy's appearing at the same time.

This required storing the pending DBus message inside the radio object
rather than a single static variable.

To handle things a bit cleaner the radio info is now created when
CreateRadio is called. The name is now a required arugment which allows
the NEW_RADIO event to lookup. This removes the duplicate code in the
NEW_RADIO ack callback and leaves only error handling.
---
 tools/hwsim.c | 147 +++++++++++++++++++++++++-------------------------
 1 file changed, 73 insertions(+), 74 deletions(-)

v2:
 * Removed creating the radio in multiple places. Instead only create
   in the NEW_RADIO event.

diff --git a/tools/hwsim.c b/tools/hwsim.c
index f74dd82e..a4edf008 100644
--- a/tools/hwsim.c
+++ b/tools/hwsim.c
@@ -35,6 +35,7 @@
 #include <net/if_arp.h>
 
 #include <ell/ell.h>
+#include "ell/useful.h"
 
 #include "linux/nl80211.h"
 
@@ -415,6 +416,8 @@ struct radio_info_rec {
 	uint8_t addrs[2][ETH_ALEN];
 	char *name;
 	bool ap_only;
+	struct l_dbus_message *pending;
+	uint32_t cmd_id;
 };
 
 struct interface_info_rec {
@@ -428,13 +431,16 @@ struct interface_info_rec {
 static struct l_queue *radio_info;
 static struct l_queue *interface_info;
 
-static struct l_dbus_message *pending_create_msg;
-static uint32_t pending_create_radio_id;
-
 static void radio_free(void *user_data)
 {
 	struct radio_info_rec *rec = user_data;
 
+	if (rec->cmd_id)
+		l_genl_family_cancel(nl80211, rec->cmd_id);
+
+	if (rec->pending)
+		l_dbus_message_unref(rec->pending);
+
 	l_free(rec->name);
 	l_free(rec);
 }
@@ -455,6 +461,14 @@ static void hwsim_radio_cache_cleanup(void)
 	interface_info = NULL;
 }
 
+static bool radio_info_match_name(const void *a, const void *b)
+{
+	const struct radio_info_rec *rec = a;
+	const char *name = b;
+
+	return !strcmp(rec->name, name);
+}
+
 static bool radio_info_match_id(const void *a, const void *b)
 {
 	const struct radio_info_rec *rec = a;
@@ -512,12 +526,6 @@ static const char *interface_get_path(const struct interface_info_rec *rec)
 	return path;
 }
 
-static struct l_dbus_message *dbus_error_busy(struct l_dbus_message *msg)
-{
-	return l_dbus_message_new_error(msg, HWSIM_SERVICE ".InProgress",
-					"Operation already in progress");
-}
-
 static struct l_dbus_message *dbus_error_failed(struct l_dbus_message *msg)
 {
 	return l_dbus_message_new_error(msg, HWSIM_SERVICE ".Failed",
@@ -603,17 +611,18 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
 	struct l_genl_attr attr;
 	uint16_t type, len;
 	const void *data;
-	const char *name = NULL;
+	_auto_(l_free) char *name = NULL;
 	const uint32_t *id = NULL;
 	size_t name_len = 0;
 	struct radio_info_rec *rec;
 	uint8_t file_buffer[128];
 	int bytes, consumed;
 	unsigned int uintval;
-	bool old;
+	bool old = false;
 	struct radio_info_rec prev_rec;
 	bool name_change = false;
 	const char *path;
+	struct l_dbus_message *reply;
 
 	if (!l_genl_attr_init(&attr, msg))
 		return;
@@ -628,8 +637,7 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
 			break;
 
 		case HWSIM_ATTR_RADIO_NAME:
-			name = data;
-			name_len = len;
+			name = l_strndup(data, len);
 			break;
 		}
 	}
@@ -637,8 +645,13 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
 	if (!id || !name)
 		return;
 
-	rec = l_queue_find(radio_info, radio_info_match_id, L_UINT_TO_PTR(*id));
-	if (rec) {
+	/*
+	 * If the radio info exists without a pending dbus message this is a
+	 * name change. Otherwise if the radio is not found something external
+	 * created it, not hwsim.
+	 */
+	rec = l_queue_find(radio_info, radio_info_match_name, name);
+	if (rec && !rec->pending) {
 		old = true;
 		memcpy(&prev_rec, rec, sizeof(prev_rec));
 
@@ -647,13 +660,9 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
 			name_change = true;
 
 		l_free(rec->name);
-	} else {
-		old = false;
-		rec = l_new(struct radio_info_rec, 1);
-		rec->id = *id;
-	}
-
-	rec->name = l_strndup(name, name_len);
+		rec->name = l_strndup(name, name_len);
+	} else if (!rec)
+		return;
 
 	l_genl_attr_init(&attr, msg);
 
@@ -688,6 +697,8 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
 		}
 	}
 
+	rec->id = *id;
+
 	/*
 	 * Assuming that the radio name is the wiphy name read the wiphy index
 	 * associated with the radio and the wiphy's hardware addresses from
@@ -758,23 +769,24 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
 	}
 
 	/* Send pending CreateRadio reply */
-	if (pending_create_msg && pending_create_radio_id == rec->id) {
-		struct l_dbus_message *reply =
-			l_dbus_message_new_method_return(pending_create_msg);
+	if (rec->pending) {
+		reply = l_dbus_message_new_method_return(rec->pending);
 
 		l_dbus_message_set_arguments(reply, "o", path);
-		dbus_pending_reply(&pending_create_msg, reply);
+		dbus_pending_reply(&rec->pending, reply);
 	}
 
 	return;
 
 err_free_radio:
-	if (!old)
-		radio_free(rec);
+	if (rec->pending)
+		dbus_pending_reply(&rec->pending,
+					dbus_error_failed(rec->pending));
 
-	if (pending_create_msg && pending_create_radio_id == *id)
-		dbus_pending_reply(&pending_create_msg,
-					dbus_error_failed(pending_create_msg));
+	if (!old) {
+		l_queue_remove(radio_info, rec);
+		radio_free(rec);
+	}
 }
 
 static bool radio_ap_only(struct radio_info_rec *rec)
@@ -1642,44 +1654,27 @@ static void unicast_handler(struct l_genl_msg *msg, void *user_data)
 static void radio_manager_create_callback(struct l_genl_msg *msg,
 						void *user_data)
 {
+	struct radio_info_rec *radio = user_data;
 	struct l_dbus_message *reply;
-	struct l_genl_attr attr;
-	struct radio_info_rec *radio;
-	int err;
 
-	/*
-	 * Note that the radio id is returned in the error field of
-	 * the returned message.
-	 */
-	if (l_genl_attr_init(&attr, msg))
-		goto error;
+	radio->cmd_id = 0;
 
-	err = l_genl_msg_get_error(msg);
-	if (err < 0)
-		goto error;
-
-	pending_create_radio_id = err;
+	if (l_genl_msg_get_error(msg) >= 0)
+		return;
 
 	/*
-	 * If the NEW_RADIO event has been received we'll have added the
-	 * radio to radio_info already but we can send the method return
-	 * only now that we know the ID returned by our command.
+	 * In theory pending should always be set. This is to handle the
+	 * NEW_RADIO event coming prior to this callback and this callback
+	 * also having an error. It doesn't seem possible for this to happen,
+	 * but who knows.
 	 */
-	radio = l_queue_find(radio_info, radio_info_match_id,
-				L_UINT_TO_PTR(pending_create_radio_id));
-	if (radio && pending_create_msg) {
-		const char *path = radio_get_path(radio);
-
-		reply = l_dbus_message_new_method_return(pending_create_msg);
-		l_dbus_message_set_arguments(reply, "o", path);
-		dbus_pending_reply(&pending_create_msg, reply);
+	if (radio->pending) {
+		reply = dbus_error_failed(radio->pending);
+		dbus_pending_reply(&radio->pending, reply);
 	}
 
-	return;
-
-error:
-	reply = dbus_error_failed(pending_create_msg);
-	dbus_pending_reply(&pending_create_msg, reply);
+	l_queue_remove(radio_info, radio);
+	radio_free(radio);
 }
 
 static struct l_dbus_message *radio_manager_create(struct l_dbus *dbus,
@@ -1694,9 +1689,7 @@ static struct l_dbus_message *radio_manager_create(struct l_dbus *dbus,
 	bool p2p = false;
 	const char *disabled_iftypes = NULL;
 	const char *disabled_ciphers = NULL;
-
-	if (pending_create_msg)
-		return dbus_error_busy(message);
+	struct radio_info_rec *radio;
 
 	if (!l_dbus_message_get_arguments(message, "a{sv}", &dict))
 		goto invalid;
@@ -1720,13 +1713,15 @@ static struct l_dbus_message *radio_manager_create(struct l_dbus *dbus,
 			goto invalid;
 	}
 
+	if (!name)
+		goto invalid;
+
 	new_msg = l_genl_msg_new(HWSIM_CMD_NEW_RADIO);
 	l_genl_msg_append_attr(new_msg, HWSIM_ATTR_DESTROY_RADIO_ON_CLOSE,
 				0, NULL);
 
-	if (name)
-		l_genl_msg_append_attr(new_msg, HWSIM_ATTR_RADIO_NAME,
-					strlen(name) + 1, name);
+	l_genl_msg_append_attr(new_msg, HWSIM_ATTR_RADIO_NAME,
+				strlen(name) + 1, name);
 
 	if (p2p)
 		l_genl_msg_append_attr(new_msg, HWSIM_ATTR_SUPPORT_P2P_DEVICE,
@@ -1752,11 +1747,18 @@ static struct l_dbus_message *radio_manager_create(struct l_dbus *dbus,
 
 	}
 
-	l_genl_family_send(hwsim, new_msg, radio_manager_create_callback,
-				pending_create_msg, NULL);
+	radio = l_new(struct radio_info_rec, 1);
+	radio->pending = l_dbus_message_ref(message);
+	radio->name = l_strdup(name);
 
-	pending_create_msg = l_dbus_message_ref(message);
-	pending_create_radio_id = 0;
+	if (!radio_info)
+		radio_info = l_queue_new();
+
+	l_queue_push_tail(radio_info, radio);
+
+	radio->cmd_id = l_genl_family_send(hwsim, new_msg,
+						radio_manager_create_callback,
+						radio, NULL);
 
 	return NULL;
 
@@ -3088,9 +3090,6 @@ int main(int argc, char *argv[])
 	l_genl_family_free(nl80211);
 	l_genl_unref(genl);
 
-	if (pending_create_msg)
-		l_dbus_message_unref(pending_create_msg);
-
 	l_dbus_destroy(dbus);
 	hwsim_radio_cache_cleanup();
 	l_queue_destroy(rules, l_free);
-- 
2.34.1

Re: [PATCH v2 1/2] hwsim: allow concurrent radio creations

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 3526 bytes --]

Hi James,

On 2/15/22 10:52, James Prestwood wrote:
> Currently CreateRadio only allows a single outstanding DBus message
> until the radio is fully created. 99% of the time this is just fine
> but in order to test dual phy cards there needs to be support for
> phy's appearing at the same time.
> 
> This required storing the pending DBus message inside the radio object
> rather than a single static variable.
> 
> To handle things a bit cleaner the radio info is now created when
> CreateRadio is called. The name is now a required arugment which allows

arugment->argument

> the NEW_RADIO event to lookup. This removes the duplicate code in the
> NEW_RADIO ack callback and leaves only error handling.
> ---
>   tools/hwsim.c | 147 +++++++++++++++++++++++++-------------------------
>   1 file changed, 73 insertions(+), 74 deletions(-)
> 

Yes, this looks much better now.

> v2:
>   * Removed creating the radio in multiple places. Instead only create
>     in the NEW_RADIO event.
> 

<snip>

> @@ -603,17 +611,18 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   	struct l_genl_attr attr;
>   	uint16_t type, len;
>   	const void *data;
> -	const char *name = NULL;
> +	_auto_(l_free) char *name = NULL;
>   	const uint32_t *id = NULL;
>   	size_t name_len = 0;
>   	struct radio_info_rec *rec;
>   	uint8_t file_buffer[128];
>   	int bytes, consumed;
>   	unsigned int uintval;
> -	bool old;
> +	bool old = false;
>   	struct radio_info_rec prev_rec;
>   	bool name_change = false;
>   	const char *path;
> +	struct l_dbus_message *reply;
>   
>   	if (!l_genl_attr_init(&attr, msg))
>   		return;
> @@ -628,8 +637,7 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   			break;
>   
>   		case HWSIM_ATTR_RADIO_NAME:
> -			name = data;
> -			name_len = len;
> +			name = l_strndup(data, len);

So why the strndup here?

>   			break;
>   		}
>   	}
> @@ -637,8 +645,13 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   	if (!id || !name)
>   		return;
>   
> -	rec = l_queue_find(radio_info, radio_info_match_id, L_UINT_TO_PTR(*id));
> -	if (rec) {
> +	/*
> +	 * If the radio info exists without a pending dbus message this is a
> +	 * name change. Otherwise if the radio is not found something external
> +	 * created it, not hwsim.
> +	 */
> +	rec = l_queue_find(radio_info, radio_info_match_name, name);

If this is a name change, then how would this work?  I think you do need to 
match by id or by name.  This may be easier done with l_queue_get_entries style 
for loop.

For a radio that is 'in flight', we may want to set the id to something 
non-sensical like -1.

> +	if (rec && !rec->pending) {
>   		old = true;
>   		memcpy(&prev_rec, rec, sizeof(prev_rec));
>   
> @@ -647,13 +660,9 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   			name_change = true;
>   
>   		l_free(rec->name);
> -	} else {
> -		old = false;
> -		rec = l_new(struct radio_info_rec, 1);
> -		rec->id = *id;
> -	}
> -
> -	rec->name = l_strndup(name, name_len);
> +		rec->name = l_strndup(name, name_len);

so either l_strndup _only_ here or l_stealptr if you already did that above. 
Assuming name isn't being used below.

> +	} else if (!rec)
> +		return;

This part seems suspicious.  What if the radio is being created outside hwsim?

>   
>   	l_genl_attr_init(&attr, msg);
>   

Regards,
-Denis

Re: [PATCH v2 1/2] hwsim: allow concurrent radio creations

[James Prestwood]

[-- Attachment #1: Type: text/plain, Size: 705 bytes --]

Hi Denis,

> 
> > +       } else if (!rec)
> > +               return;
> 
> This part seems suspicious.  What if the radio is being created
> outside hwsim?

I guess this is changing behavior... Currently we would still create
the radio info object if the radio was created externally. Keeping this
behavior will end up requiring creating the radio here and in
CreateRadio(), nearly the same as what we had before.

I guess its up to you. Either we don't support external radio creation
for cleaner code, or we go back to the way it was (or some variant of
it).

> 
> >   
> >         l_genl_attr_init(&attr, msg);
> >   
> 
> Regards,
> -Denis

Re: [PATCH v2 1/2] hwsim: allow concurrent radio creations

[Denis Kenzior]

[-- Attachment #1: Type: text/plain, Size: 988 bytes --]

Hi James,

On 2/15/22 13:16, James Prestwood wrote:
> Hi Denis,
> 
>>
>>> +       } else if (!rec)
>>> +               return;
>>
>> This part seems suspicious.  What if the radio is being created
>> outside hwsim?
> 
> I guess this is changing behavior... Currently we would still create
> the radio info object if the radio was created externally. Keeping this
> behavior will end up requiring creating the radio here and in
> CreateRadio(), nearly the same as what we had before.
> 

CreateRadio really only creates a stub.  What is there to duplicate besides 
l_new and setting the name/id?

> I guess its up to you. Either we don't support external radio creation
> for cleaner code, or we go back to the way it was (or some variant of
> it).
> 

Clearly we want to support this since hwsim might start with radios already 
created and this path is being used by the initial radio dump (started in 
hwsim_ready).

Regards,
-Denis