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
> + */
> list_del(&entry->list);
> kmem_cache_free(batadv_tt_change_cache, entry);
> bat_priv->tt.local_changes--;
>
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;
--
Remi
