Re: [PATCH 06/11] staging: wilc1000: refactor mgmt_tx to fix line over 80 chars
From: Dan Carpenter <hidden>
Date: 2018-03-21 07:40:26
This would have been easier for me if it were split up slightly different again. This patch is fine. I have a couple comments for future patches which you are free to ignore if you want because it's mostly just my personal taste. On Tue, Mar 20, 2018 at 10:25:39PM +0530, Ajay Singh wrote:
+ if (subtype != P2P_INV_REQ && subtype != P2P_INV_RSP) {
+ int vendor_spec_len = sizeof(p2p_vendor_spec);I'm not a huge fan of making shorter names for sizeof()s just to get around the 80 character rule. The 80 character rule is ultimately supposed to make the code more readable, and this is making less readable so it's maybe better to just ignore the rule.
quoted hunk ↗ jump to hunk
+ + memcpy(&mgmt_tx->buff[len], p2p_vendor_spec, + vendor_spec_len); + mgmt_tx->buff[len + vendor_spec_len] = p2p_local_random; + mgmt_tx->size = buf_len; + } + } +} + static int mgmt_tx(struct wiphy *wiphy, struct wireless_dev *wdev, struct cfg80211_mgmt_tx_params *params,@@ -1568,9 +1620,9 @@ static int mgmt_tx(struct wiphy *wiphy, struct p2p_mgmt_data *mgmt_tx; struct wilc_priv *priv; struct host_if_drv *wfi_drv; - u32 i; struct wilc_vif *vif; u32 buf_len = len + sizeof(p2p_vendor_spec) + sizeof(p2p_local_random); + int ret = 0; vif = netdev_priv(wdev->netdev); priv = wiphy_priv(wiphy);@@ -1580,92 +1632,75 @@ static int mgmt_tx(struct wiphy *wiphy, priv->tx_cookie = *cookie; mgmt = (const struct ieee80211_mgmt *)buf; - if (ieee80211_is_mgmt(mgmt->frame_control)) { - mgmt_tx = kmalloc(sizeof(struct p2p_mgmt_data), GFP_KERNEL); - if (!mgmt_tx) - return -EFAULT; + if (!ieee80211_is_mgmt(mgmt->frame_control)) + goto out;
I hate this "goto out;". My first question when I see it is "what does goto out; do?" It's a kind of vague label name. So I have to scroll down to the bottom of the function. And then I'm like "Oh, it doesn't do anything. Well that was a waste of time." And then it occurs to me, "Huh, what is 'ret' set to?" So now I have to scroll all the way to the very start of the function... All this scrolling could be avoided if we just did a direct "return 0;" regards, dan carpenter