Re: [PATCH v3 5/5] batman-adv: Don't keep redundant TT change events

[Sven Eckelmann <sven@narfation.org>]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]

On Thursday, 21 November 2024 10:23:33 CET Sven Eckelmann wrote:
> On Thursday, 21 November 2024 10:13:15 CET Remi Pommarel wrote:
> > Looks right to me, I would even simplify that more for readability with:
> [...]
> > -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;
> > 
> 
> The "discard" is unused. But the rest looks good.

No, it doesn't. You've accidental removed "entry->change.flags = flags;". So 
it should look more like this:

--- a/net/batman-adv/translation-table.c
+++ b/net/batman-adv/translation-table.c
@@ -484,34 +484,25 @@ 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
-		 * add): update them
-		 */
-		if (del_op_requested == del_op_entry) {
+		if (del_op_requested != del_op_entry) {
+			/* 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
+			 */
+			list_del(&entry->list);
+			kmem_cache_free(batadv_tt_change_cache, entry);
+			bat_priv->tt.local_changes--;
+		} else {
+			/* 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
+			 */
 			entry->change.flags = flags;
-			goto discard;
 		}
 
-		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;
 	}

Kind regards,
	Sven

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 228 bytes --]