On 30/08/17 20:11, Arend van Spriel wrote:
>> Whilst this if () statement is technically correct, it lacks clarity.
>
> I don't see the unclarity here. In my opinion people reading the code
> should have a good level in C language and a decent level of curiosity
> when they come across a function/macro like skb_queue_walk().
I thought it to be part of the general codingstyle for the kernel that
multi-line ifs and elses should be in braces, although I accept that
technically the if-clause is a single block-level statement.
Having said that, *this* specific example falls into a grey area in the
codingstyle, which covers multi-statement, multi-line if() clauses and
single-statement, single-line ones. It does not cover single-statement,
multi-line examples such as the one here.
Whilst I can't therefore definitively justify my position, I can show,
for example, line 999 in net/mac80211/iface.c where a for() statement
uses braces around the skb_queue_walk()
for (i = 0; i < IEEE80211_MAX_QUEUES; i++) {
skb_queue_walk_safe(&local->pending[i], skb, tmp) {
struct ieee80211_tx_info *info = IEEE80211_SKB_CB(skb);
if (info->control.vif == &sdata->vif) {
__skb_unlink(skb, &local->pending[i]);
ieee80211_free_txskb(&local->hw, skb);
}
}
}
And the following in ath9k_htc_tx_cleanup_queue()
if (process) {
skb_queue_walk_safe(&queue, skb, tmp) {
__skb_unlink(skb, &queue);
ath9k_htc_tx_process(priv, skb, NULL);
}
}
So I feel that we should do the same.
-Ian
