On Thu, Nov 21, 2024 at 09:43:01AM +0100, Sven Eckelmann wrote:
On Wednesday, 20 November 2024 18:47:18 CET Remi Pommarel wrote:
/* this is a second add in the same originator interval. It* means that flags have been changed: update them!
/* this is a second add or del in the same originator interval.* It could mean that flags have been changed (e.g. double* add): update them */
if (!del_op_requested && !del_op_entry)
if (del_op_requested == del_op_entry) { entry->change.flags = flags;goto discard;} continue;From logic perspective, the check would be irrelevant - and the continue never happens.
if (!del_op_requested && del_op_entry) goto del; if (del_op_requested && !del_op_entry) goto del;Implies that del_op_requested == del_op_entry. So the check wouldn't make too much sense. Maybe we should clean up this logic further:
diff --git a/net/batman-adv/translation-table.c b/net/batman-adv/translation-table.c index 437c4edd..b349851b 100644 --- a/net/batman-adv/translation-table.c +++ b/net/batman-adv/translation-table.c @@ -484,18 +484,7 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, if (!batadv_compare_eth(entry->change.addr, common->addr)) continue;
/* DEL+ADD in the same orig interval have no effect and can be* removed to avoid silly behaviour on the receiver side. The* other way around (ADD+DEL) can happen in case of roaming of* a client still in the NEW state. Roaming of NEW clients is* now possible due to automatically recognition of "temporary"* clients*/del_op_entry = entry->change.flags & BATADV_TT_CLIENT_DEL;
if (!del_op_requested && del_op_entry)goto del;if (del_op_requested && !del_op_entry)goto del;/* this is a second add or del in the same originator interval.
- It could mean that flags have been changed (e.g. double
@@ -506,8 +495,13 @@ static void batadv_tt_local_event(struct batadv_priv *bat_priv, goto discard; }
continue;-del:
/* DEL+ADD in the same orig interval have no effect and can be* removed to avoid silly behaviour on the receiver side. The* other way around (ADD+DEL) can happen in case of roaming of* a client still in the NEW state. Roaming of NEW clients is* now possible due to automatically recognition of "temporary"* clients*/
Looks right to me, I would even simplify that more for readability with:
* now possible due to automatically recognition of "temporary" * clients */ - del_op_entry = entry->change.flags & BATADV_TT_CLIENT_DEL; - if (!del_op_requested && del_op_entry) - goto del; - if (del_op_requested && !del_op_entry) - goto del; - - /* this is a second add or del in the same originator interval. - * It could mean that flags have been changed (e.g. double - * add): update them - */ - if (del_op_requested == del_op_entry) { - entry->change.flags = flags; - goto discard; + if (del_op_requested != del_op_entry) { + list_del(&entry->list); + kmem_cache_free(batadv_tt_change_cache, entry); + bat_priv->tt.local_changes--; } - - continue; -del: - list_del(&entry->list); - kmem_cache_free(batadv_tt_change_cache, entry); - bat_priv->tt.local_changes--; discard: kmem_cache_free(batadv_tt_change_cache, tt_change_node); goto unlock;