On Monday, 28 October 2024 14:25:36 CET Antonio Quartulli wrote:
[...]
> > +/**
> > + * batadv_dat_get_softif() - get the soft interface from a netlink callback
> > + * @cb: callback structure containing arguments
> > + *
> > + * Return: The soft interface on success or an error pointer otherwise.
> > + */
> > +static struct net_device *batadv_dat_get_softif(struct netlink_callback
> > *cb)
> > +{
> > + struct net *net = sock_net(cb->skb->sk);
> > + struct net_device *soft_iface;
> > + int ifindex;
> > +
> > + ifindex = batadv_netlink_get_ifindex(cb->nlh,
> > + BATADV_ATTR_MESH_IFINDEX);
> > + if (!ifindex)
> > + return ERR_PTR(-EINVAL);
> > +
> > + soft_iface = dev_get_by_index(net, ifindex);
> > + if (!soft_iface)
> > + return ERR_PTR(-ENODEV);
> > +
> > + if (!batadv_softif_is_valid(soft_iface)) {
> > + dev_put(soft_iface);
> > + return ERR_PTR(-ENODEV);
> > + }
> > +
> > + return soft_iface;
> > +}
>
> I don't think this function is DAT specific at all.
> Moreover, the very same code (which I think you are re-using here)
> appears in batadv_netlink_dump_hardif().
>
> I think it'd make more sense to factor it out and create a helper out of
> it (place it in netlink.c?). This way we avoid code duplication.
>
> [I might be wrong but 90% of the work already is in
> batadv_get_softif_from_info()]
Looks like this was never answered and also not handled in the v6 version of
the patch.
[...]
> > --- a/net/batman-adv/types.h
> > +++ b/net/batman-adv/types.h
> > @@ -1231,8 +1231,11 @@ struct batadv_priv_dat {
> > /** @addr: node DAT address */
> > batadv_dat_addr_t addr;
> >
> > - /** @hash: hashtable representing the local ARP cache */
> > - struct batadv_hashtable *hash;
> > + /** @cache_hash: hashtable representing the local ARP cache */
> > + struct batadv_hashtable *cache_hash;
> > +
> > + /** @dht_hash: hashtable representing the local DAT DHT */
> > + struct batadv_hashtable *dht_hash;
> >
> > /** @work: work queue callback item for cache purging */
> > struct delayed_work work;
>
> I can see that most of the code in this patch is about handling two
> tables (in a generic fashion) instead of one.
>
> One functional change I am seeing is also that before this patch
> batman-adv would store/cache any ARP information coming from the mesh.
> While now this happens only for the DHT PUT. Am I right?
>
> If that's the case, it means we may issue more DHT_GETs (and possibly
> ARP requests) because we lost a chance to cache a bit more that what the
> DHT stores. Does it make sense?
Looks like this was never answered.
Kind regards,
Sven
signature.asc
Description: This is a digitally signed message part.
