3:52 a.m.
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
3:52 a.m.
New subject: [PATCH v2 2/2] auto-t: hwsim.py: support async radio creation
Adds a new wait argument which, if false, will call the DBus method
and return immediately. This allows the caller to create multiple
radios very quickly, simulating (as close as we can) a wifi card
with dual phy's which appear in the kernel simultaneously.
The name argument was also changed to be mandatory, which is now
required by hwsim.
---
autotests/util/hwsim.py | 19 ++++++++++++++-----
1 file changed, 14 insertions(+), 5 deletions(-)
diff --git a/autotests/util/hwsim.py b/autotests/util/hwsim.py
index 34859910..ce85f43d 100755
--- a/autotests/util/hwsim.py
+++ b/autotests/util/hwsim.py
@@ -309,26 +309,35 @@ class RadioList(collections.Mapping):
def _interfaces_removed_handler(self, path, interfaces):
del _dict[path]
- def create(self, name=None, p2p_device=False, iftype_disable=None,
- cipher_disable=None):
+ def create(self, name, p2p_device=False, iftype_disable=None,
+ cipher_disable=None, wait=True):
args = dbus.Dictionary({
+ 'Name': name,
'P2P': p2p_device,
}, signature='sv')
- if name:
- args['Name'] = name
-
if iftype_disable:
args['InterfaceTypeDisable'] = iftype_disable
if cipher_disable:
args['CipherTypeDisable'] = cipher_disable
+ if not wait:
+ self._radio_manager.CreateRadio(args, reply_handler=self._success,
+ error_handler=self._failure)
+ return None
+
path = self._radio_manager.CreateRadio(args)
obj = Radio(path)
self._dict[path] = obj
return obj
+ def _success(self, bla):
+ pass
+
+ def _failure(self, ex):
+ pass
+
class Hwsim(iwd.AsyncOpAbstract):
_instances = WeakValueDictionary()
--
2.34.1
4:53 a.m.
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
6:16 a.m.
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
6:40 a.m.
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
215
days inactive
215
days old
4 comments
2 participants
participants (2)
-
Denis Kenzior -
James Prestwood