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

* Re: [PATCH 2/3] hwsim: allow concurrent radio creations
@ 2022-02-14 22:19 Denis Kenzior
  0 siblings, 0 replies; 2+ messages in thread
From: Denis Kenzior @ 2022-02-14 22:19 UTC (permalink / raw)
  To: iwd

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

Hi James,

On 2/14/22 14:34, 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.
> ---
>   tools/hwsim.c | 77 +++++++++++++++++++++++++++------------------------
>   1 file changed, 41 insertions(+), 36 deletions(-)
> 
> diff --git a/tools/hwsim.c b/tools/hwsim.c
> index f74dd82e..87e5db57 100644
> --- a/tools/hwsim.c
> +++ b/tools/hwsim.c
> @@ -415,6 +415,7 @@ struct radio_info_rec {
>   	uint8_t addrs[2][ETH_ALEN];
>   	char *name;
>   	bool ap_only;
> +	struct l_dbus_message *pending;
>   };
>   
>   struct interface_info_rec {
> @@ -428,13 +429,13 @@ 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->pending)
> +		l_dbus_message_unref(rec->pending);
> +
>   	l_free(rec->name);
>   	l_free(rec);
>   }
> @@ -512,12 +513,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",
> @@ -610,7 +605,7 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   	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;
> @@ -637,8 +632,15 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   	if (!id || !name)
>   		return;
>   
> +	/*
> +	 * If the radio info exists without a pending dbus message this is a
> +	 * name change. If the radio info exists with a pending message a
> +	 * minimal radio was created in order to save the DBus message. This
> +	 * can be handled nearly identical to if 'rec' is NULL by initializing
> +	 * the object now.
> +	 */
>   	rec = l_queue_find(radio_info, radio_info_match_id, L_UINT_TO_PTR(*id));
> -	if (rec) {
> +	if (rec && !rec->pending) {
>   		old = true;
>   		memcpy(&prev_rec, rec, sizeof(prev_rec));
>   
> @@ -647,8 +649,7 @@ static void get_radio_callback(struct l_genl_msg *msg, void *user_data)
>   			name_change = true;
>   
>   		l_free(rec->name);
> -	} else {
> -		old = false;
> +	} else if (!rec) {
>   		rec = l_new(struct radio_info_rec, 1);
>   		rec->id = *id;
>   	}
> @@ -758,12 +759,12 @@ 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) {
> +	if (rec->pending) {
>   		struct l_dbus_message *reply =
> -			l_dbus_message_new_method_return(pending_create_msg);
> +			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;
> @@ -772,9 +773,9 @@ err_free_radio:
>   	if (!old)
>   		radio_free(rec);
>   
> -	if (pending_create_msg && pending_create_radio_id == *id)
> -		dbus_pending_reply(&pending_create_msg,
> -					dbus_error_failed(pending_create_msg));
> +	if (rec->pending)
> +		dbus_pending_reply(&rec->pending,
> +					dbus_error_failed(rec->pending));
>   }
>   
>   static bool radio_ap_only(struct radio_info_rec *rec)
> @@ -1642,6 +1643,7 @@ 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 l_dbus_message *message = user_data;
>   	struct l_dbus_message *reply;
>   	struct l_genl_attr attr;
>   	struct radio_info_rec *radio;
> @@ -1658,28 +1660,38 @@ static void radio_manager_create_callback(struct l_genl_msg *msg,
>   	if (err < 0)
>   		goto error;
>   
> -	pending_create_radio_id = err;
> -
>   	/*
>   	 * 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.
>   	 */
>   	radio = l_queue_find(radio_info, radio_info_match_id,
> -				L_UINT_TO_PTR(pending_create_radio_id));
> -	if (radio && pending_create_msg) {
> +				L_UINT_TO_PTR(err));
> +	if (radio) {
>   		const char *path = radio_get_path(radio);
>   
> -		reply = l_dbus_message_new_method_return(pending_create_msg);
> +		reply = l_dbus_message_new_method_return(message);
>   		l_dbus_message_set_arguments(reply, "o", path);
> -		dbus_pending_reply(&pending_create_msg, reply);
> +		dbus_pending_reply(&message, reply);
> +	} else {
> +		/*
> +		 * NEW_RADIO has not happened but in order to allow concurrent
> +		 * radio creations we must save away the message now. This means
> +		 * creating this minimal radio object in order to look up in
> +		 * the NEW_RADIO callback and reply.
> +		 */
> +		radio = l_new(struct radio_info_rec, 1);
> +		radio->id = err;
> +		radio->pending = message;
> +
> +		l_queue_push_tail(radio_info, radio);
>   	}
>   
>   	return;
>   
>   error:
> -	reply = dbus_error_failed(pending_create_msg);
> -	dbus_pending_reply(&pending_create_msg, reply);
> +	reply = dbus_error_failed(message);
> +	dbus_pending_reply(&message, reply);

This all seems even messier than before.  Maybe we should just always create the 
radio info structure in radio_manager_create and just set the 
pending_genl_cmd_id and dbus_message accordingly?  That way you're not creating 
the structure all over the place.

>   }
>   
>   static struct l_dbus_message *radio_manager_create(struct l_dbus *dbus,

<snip>

> @@ -1753,10 +1762,9 @@ 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);
> +				message, NULL);

Nit: You probably should be using the destroy callback here.  Otherwise the 
messages won't be cleaned up properly.

>   
> -	pending_create_msg = l_dbus_message_ref(message);
> -	pending_create_radio_id = 0;
> +	l_dbus_message_ref(message);
>   
>   	return NULL;
>   

<snip>

Regards,
-Denis

^ permalink raw reply	[flat|nested] 2+ messages in thread