Re: [B.A.T.M.A.N.] [PATCHv3 1/9] batman-adv: make struct batadv_neigh_node algorithm agnostic

Marek Lindner
2013-08-16T09:27:51Z

On Friday, August 16, 2013 14:27:29 Antonio Quartulli wrote:
> On Fri, Aug 16, 2013 at 11:46:41AM +0800, Marek Lindner wrote:
> > On Tuesday, August 13, 2013 14:43:44 Antonio Quartulli wrote:
> > > @@ -1011,13 +1011,13 @@ batadv_iv_ogm_update_seqnos(const struct ethhdr
> > > *ethhdr, }
> > >
> > > /* if the window moved, set the update flag. */
> > >
> > > - need_update |= batadv_bit_get_packet(bat_priv,
> > > - tmp_neigh_node->real_bits,
> > > + bitmap = tmp_neigh_node->bat_iv.real_bits;
> > > + need_update |= batadv_bit_get_packet(bat_priv, bitmap,
> > >
> > > seq_diff, set_mark);
> > >
> > > - packet_count = bitmap_weight(tmp_neigh_node->real_bits,
> > > + packet_count = bitmap_weight(tmp_neigh_node->bat_iv.real_bits,
> > >
> > > BATADV_TQ_LOCAL_WINDOW_SIZE);
> > >
> > > - tmp_neigh_node->real_packet_count = packet_count;
> > > + tmp_neigh_node->bat_iv.real_packet_count = packet_count;
> > >
> > > }
> >
> > You can't make assumptions about the size of bitmap because it is
> > platform specific which is why the variable declaration is a macro in
> > the first place.
>
> where do I make such assumption? Variable bitmap is a just storing the
> address of the bitmap.

If it is just the address it probably is ok - you are right.

> > > - struct batadv_priv *bat_priv = netdev_priv(hard_iface->soft_iface);
> > >
> > > struct batadv_neigh_node *neigh_node;
> > >
> > > neigh_node = kzalloc(sizeof(*neigh_node), GFP_ATOMIC);
> > > if (!neigh_node)
> > >
> > > goto out;
> > >
> > > - INIT_HLIST_NODE(&neigh_node->list);
> > > -
> > >
> > > memcpy(neigh_node->addr, neigh_addr, ETH_ALEN);
> > >
> > > - spin_lock_init(&neigh_node->lq_update_lock);
> > > + neigh_node->if_incoming = hard_iface;
> > > + neigh_node->orig_node = orig_node;
> > > +
> > > + spin_lock_bh(&orig_node->neigh_list_lock);
> > > + hlist_add_head_rcu(&neigh_node->list, &orig_node->neigh_list);
> > > + spin_unlock_bh(&orig_node->neigh_list_lock);
> > > +
> > > + INIT_LIST_HEAD(&neigh_node->bonding_list);
> > >
> > > /* extra reference for return */
> > > atomic_set(&neigh_node->refcount, 2);
> > >
> > > - batadv_dbg(BATADV_DBG_BATMAN, bat_priv,
> > > - "Creating new neighbor %pM on interface %s\n", neigh_addr,
> > > - hard_iface->net_dev->name);
> > > -
> > >
> > > out:
> > > return neigh_node;
> > >
> > > }
> >
> > All variables should be initialized before we add the new entry to the
> > any list.
>
> object is allocated with kzalloc: everything is zero. I thought we use
> kzalloc for this purpose: avoid initialisation of every field..no?

What is "INIT_LIST_HEAD(&neigh_node->bonding_list);" ? Looks like an
initialization if I am not mistaken. Moreover, batadv_iv_ogm_neigh_new() inits
even more after this function returns ...

Cheers,
Marek