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

[Denis Kenzior <denkenz at gmail.com>]

[-- 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