| Message ID | 7ee141a4-8983-4da6-9c0f-f070fa6e5611@MAIL2.uni-rostock.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Johannes Berg |
| Headers | show |
On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote: > Make the dump operation aware of changes on radio list and corresponding > inconsistent dumps. Change the dump iteration to be independent from > increasing radio indices on radio list. Looks like this should use nl_dump_check_consistent()? johannes
Am 11.12.2017 um 13:14 schrieb Johannes Berg: > On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote: >> Make the dump operation aware of changes on radio list and corresponding >> inconsistent dumps. Change the dump iteration to be independent from >> increasing radio indices on radio list. > Looks like this should use nl_dump_check_consistent()? > > johannes > It is called in mac80211_hwsim_get_radio, I didn't changed that.
On Mon, 2017-12-11 at 13:37 +0100, Benjamin Beichler wrote: > Am 11.12.2017 um 13:14 schrieb Johannes Berg: > > On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote: > > > Make the dump operation aware of changes on radio list and corresponding > > > inconsistent dumps. Change the dump iteration to be independent from > > > increasing radio indices on radio list. > > > > Looks like this should use nl_dump_check_consistent()? > > > > johannes > > > > It is called in mac80211_hwsim_get_radio, I didn't changed that. But you added this: + /* list changed */ + if (cb->prev_seq && cb->seq != cb->prev_seq) + goto cleanup; which is mostly just a copy of the inline. johannes
Am 11.12.2017 um 13:49 schrieb Johannes Berg: > On Mon, 2017-12-11 at 13:37 +0100, Benjamin Beichler wrote: >> Am 11.12.2017 um 13:14 schrieb Johannes Berg: >>> On Tue, 2017-11-21 at 13:17 +0100, Benjamin Beichler wrote: >>>> Make the dump operation aware of changes on radio list and corresponding >>>> inconsistent dumps. Change the dump iteration to be independent from >>>> increasing radio indices on radio list. >>> Looks like this should use nl_dump_check_consistent()? >>> >>> johannes >>> >> It is called in mac80211_hwsim_get_radio, I didn't changed that. > But you added this: > > + /* list changed */ > + if (cb->prev_seq && cb->seq != cb->prev_seq) > + goto cleanup; > > which is mostly just a copy of the inline. > > johannes Year you are right, but for nl_dump_check_consistent() I also need a header struct to write the flag to it and I thought a ghost header only to this function is also misleading. But if you think this is better, I can do that. Or we introduce a function, which really only check consistency and not also set the flag. I also thought the line is readable at it's own, because it's simply inconsistent if the sequence numbers are not equal.
On Mon, 2017-12-11 at 14:02 +0100, Benjamin Beichler wrote: > > > But you added this: > > > > + /* list changed */ > > + if (cb->prev_seq && cb->seq != cb->prev_seq) > > + goto cleanup; > > > > which is mostly just a copy of the inline. > > > > johannes > > Year you are right, but for nl_dump_check_consistent() I also need a > header struct to write the flag to it and I thought a ghost header only > to this function is also misleading. But if you think this is better, I > can do that. Or we introduce a function, which really only check > consistency and not also set the flag. I also thought the line is > readable at it's own, because it's simply inconsistent if the sequence > numbers are not equal. It's readable, but there should be an indication to userspace in this case, no? johannes
Am 11.12.2017 um 14:07 schrieb Johannes Berg: > On Mon, 2017-12-11 at 14:02 +0100, Benjamin Beichler wrote: >>> But you added this: >>> >>> + /* list changed */ >>> + if (cb->prev_seq && cb->seq != cb->prev_seq) >>> + goto cleanup; >>> >>> which is mostly just a copy of the inline. >>> >>> johannes >> Year you are right, but for nl_dump_check_consistent() I also need a >> header struct to write the flag to it and I thought a ghost header only >> to this function is also misleading. But if you think this is better, I >> can do that. Or we introduce a function, which really only check >> consistency and not also set the flag. I also thought the line is >> readable at it's own, because it's simply inconsistent if the sequence >> numbers are not equal. > It's readable, but there should be an indication to userspace in this > case, no? > > johannes > Mhh, OK you are totally right. I think we need to send something like an empty message, containing the flag. Because there exist the corner case, that while a dump is interrupted the complete list is deleted. Currently this could not happen because of non-parallel netlink callbacks, but maybe in the future parallel callbacks are enabled. Would it break things, if I simply create an header with HWSIM_CMD_GET_RADIO, but put no other information in it? Or how could it be done correctly?
On Mon, 2017-12-11 at 14:29 +0100, Benjamin Beichler wrote: > I think we need to send something like an empty message, containing the > flag. Because there exist the corner case, that while a dump is > interrupted the complete list is deleted. Currently this could not > happen because of non-parallel netlink callbacks, but maybe in the > future parallel callbacks are enabled. I don't know if we really want parallel, but I guess it's possible eventually. > Would it break things, if I simply create an header with > HWSIM_CMD_GET_RADIO, but put no other information in it? Or how could it > be done correctly? I think that's probably OK. johannes
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index 48b9efed725e..fc8d9664cbfa 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -494,6 +494,7 @@ static struct rhashtable hwsim_radios_rht; static spinlock_t hwsim_delete_lock; static LIST_HEAD(delete_radios); static int hwsim_radio_idx; +static int hwsim_radios_generation = 1; static struct platform_driver mac80211_hwsim_driver = { .driver = { @@ -2754,6 +2755,7 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, } list_add_tail(&data->list, &hwsim_radios); + hwsim_radios_generation++; spin_unlock_bh(&hwsim_radio_lock); if (idx > 0) @@ -3202,6 +3204,7 @@ static int hwsim_del_radio_nl(struct sk_buff *msg, struct genl_info *info) list_del(&data->list); rhashtable_remove_fast(&hwsim_radios_rht, &data->rht, hwsim_rht_params); + hwsim_radios_generation++; spin_unlock_bh(&hwsim_radio_lock); mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), info); @@ -3258,19 +3261,25 @@ static int hwsim_get_radio_nl(struct sk_buff *msg, struct genl_info *info) static int hwsim_dump_radio_nl(struct sk_buff *skb, struct netlink_callback *cb) { - int idx = cb->args[0]; - struct mac80211_hwsim_data *data = NULL; + struct mac80211_hwsim_data *data = + (struct mac80211_hwsim_data *)cb->args[0]; int res; spin_lock_bh(&hwsim_radio_lock); + cb->seq = hwsim_radios_generation; - if (idx == hwsim_radio_idx) - goto done; + /* list changed */ + if (cb->prev_seq && cb->seq != cb->prev_seq) + goto cleanup; - list_for_each_entry(data, &hwsim_radios, list) { - if (data->idx < idx) - continue; + /* iterator is at head again, finish*/ + if (data && &data->list == &hwsim_radios) + goto cleanup; + /* data will NULL or valid since we quit, if list changed */ + data = list_prepare_entry(data, &hwsim_radios, list); + + list_for_each_entry_continue(data, &hwsim_radios, list) { if (!net_eq(wiphy_net(data->hw->wiphy), sock_net(skb->sk))) continue; @@ -3280,13 +3289,11 @@ static int hwsim_dump_radio_nl(struct sk_buff *skb, NLM_F_MULTI); if (res < 0) break; - - idx = data->idx + 1; } - cb->args[0] = idx; + cb->args[0] = (long)data; -done: +cleanup: spin_unlock_bh(&hwsim_radio_lock); return skb->len; } @@ -3348,6 +3355,7 @@ static void destroy_radio(struct work_struct *work) spin_lock_bh(&hwsim_delete_lock); list_del(&data->list); + hwsim_radios_generation++; spin_unlock_bh(&hwsim_delete_lock); mac80211_hwsim_del_radio(data, wiphy_name(data->hw->wiphy), NULL);
Make the dump operation aware of changes on radio list and corresponding inconsistent dumps. Change the dump iteration to be independent from increasing radio indices on radio list. Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de> --- drivers/net/wireless/mac80211_hwsim.c | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-)