| Message ID | 20180122170437.14213-3-benjamin.beichler@uni-rostock.de (mailing list archive) |
|---|---|
| State | Accepted |
| Delegated to: | Johannes Berg |
| Headers | show |
On Mon, 2018-01-22 at 18:04 +0100, Benjamin Beichler wrote: > > + if(!info) > + pr_debug("mac80211_hwsim: radio index %d already present\n", > + idx); Can this really trigger? I think I'll edit it out - we do pass info==NULL in the case of module init, but in that case nothing else can interfere and we can't even really fail. johannes
Am 30.01.2018 um 08:56 schrieb Johannes Berg: > On Mon, 2018-01-22 at 18:04 +0100, Benjamin Beichler wrote: >> + if(!info) >> + pr_debug("mac80211_hwsim: radio index %d already present\n", >> + idx); > Can this really trigger? I think I'll edit it out - we do pass > info==NULL in the case of module init, but in that case nothing else > can interfere and we can't even really fail. It should not be triggered because of already existing macs, I added it for somehow completeness, but you can leave it out. After having again a look on it, there could be an -ENOMEM on rhashtable_insert. Maybe it's better to explicitly catch only -EEXIST and print for the rest an generic error. Do you agree ? > johannes >
On Tue, 2018-01-30 at 10:47 +0100, Benjamin Beichler wrote: > It should not be triggered because of already existing macs, I added it > for somehow completeness, but you can leave it out. Sure. > After having again a look on it, there could be an -ENOMEM on > rhashtable_insert. Maybe it's better to explicitly catch only -EEXIST > and print for the rest an generic error. Do you agree ? Nah, that's unlikely enough ... and you get -ENOMEM back in userspace too. That said, good thing I looked at this - you forgot a set of braces :-) johannes
Am 31.01.2018 um 13:05 schrieb Johannes Berg: > On Tue, 2018-01-30 at 10:47 +0100, Benjamin Beichler wrote: > >> It should not be triggered because of already existing macs, I added it >> for somehow completeness, but you can leave it out. > Sure. > >> After having again a look on it, there could be an -ENOMEM on >> rhashtable_insert. Maybe it's better to explicitly catch only -EEXIST >> and print for the rest an generic error. Do you agree ? > Nah, that's unlikely enough ... and you get -ENOMEM back in userspace > too. > > That said, good thing I looked at this - you forgot a set of braces :-) Damn ... but fortunately you saw it. As you already accepted it, I don't need to send new patches :-D > johannes > thanks for your efforts! Benjamin
diff --git a/drivers/net/wireless/mac80211_hwsim.c b/drivers/net/wireless/mac80211_hwsim.c index e9c92f70b97d..8baf61dea961 100644 --- a/drivers/net/wireless/mac80211_hwsim.c +++ b/drivers/net/wireless/mac80211_hwsim.c @@ -2796,8 +2796,13 @@ static int mac80211_hwsim_new_radio(struct genl_info *info, err = rhashtable_insert_fast(&hwsim_radios_rht, &data->rht, hwsim_rht_params); if (err < 0) { - pr_debug("mac80211_hwsim: radio index %d already present\n", - idx); + if(!info) + pr_debug("mac80211_hwsim: radio index %d already present\n", + idx); + else + GENL_SET_ERR_MSG(info,"perm addr already present"); + NL_SET_BAD_ATTR(info->extack, + info->attrs[HWSIM_ATTR_PERM_ADDR]); spin_unlock_bh(&hwsim_radio_lock); goto failed_final_insert; }
Add a NL_ERR_MSG in case of creating a radio by a netlink message to give clear output to the creating process instead of creating only a debug message in kernel log. The same function is used for the creation while module load, so keep the old message, although it should never be thrown while load, because the module controls all mac addresses. Signed-off-by: Benjamin Beichler <benjamin.beichler@uni-rostock.de> --- drivers/net/wireless/mac80211_hwsim.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)