RE: [PATCH dpdk v2 16/16] ipv6: add function to check ipv6 version

["Morten Brørup" <mb@smartsharesystems.com>]

> From: Robin Jarry [mailto:rjarry@redhat.com]
> Sent: Thursday, 10 October 2024 22.01
> 
> Hi Morten,
> 
> Morten Brørup, Oct 06, 2024 at 11:02:
> > Personally, I would prefer following the convention of rte_ether
> functions to return boolean (as int)...
> >
> > static inline int
> rte_is_<zero/unicast/multicast/broadcast/universal/local_admin/valid_as
> signed>_ether_addr(const struct rte_ether_addr *ea)
> 
> Sorry, I haven't followed your recommendation in v3, but I have a good
> reason :)
> 
> I find that functions following that naming scheme are impossible to
> find since they all start with the same "rte_is_" prefix regardless of
> the DPDK module in which they are defined. It it is particularly
> annoying when using code completion with clangd or similar tools.
> 
> rte_is_power_of_2                   <rte_bitops.h>
> rte_is_aligned                      <rte_common.h>
> rte_is_same_ether_addr              <rte_ether.h>
> rte_is_zero_ether_addr              <rte_ether.h>
> rte_is_unicast_ether_addr           <rte_ether.h>
> rte_is_multicast_ether_addr         <rte_ether.h>
> rte_is_broadcast_ether_addr         <rte_ether.h>
> rte_is_universal_ether_addr         <rte_ether.h>
> rte_is_local_admin_ether_addr       <rte_ether.h>
> rte_is_valid_assigned_ether_addr    <rte_ether.h>
> 
> The ethernet address functions are even more extreme as they end all
> with the same suffix:
> 
>     rte_<action>_<namespace>_<object>
> 
> All other public API seems to follow the inverse logic:
> 
>     rte_<namespace>_<object>_<action>
> 
> It does not follow the natural English language but more of an inverse
> polish notation. However, it feels more user friendly and better
> discoverable.
> 
> rte_ipv6_addr_eq
> rte_ipv6_addr_eq_prefix
> rte_ipv6_addr_is_linklocal
> rte_ipv6_addr_is_loopback
> rte_ipv6_addr_is_mcast
> rte_ipv6_addr_is_sitelocal
> rte_ipv6_addr_is_unspec
> rte_ipv6_addr_is_v4compat
> rte_ipv6_addr_is_v4mapped
> rte_ipv6_addr_mask
> rte_ipv6_llocal_from_ethernet
> rte_ipv6_mask_depth
> rte_ipv6_mc_scope
> rte_ipv6_solnode_from_addr
> 
> I hope I'm making sense :)

Excellent explanation.
You have convinced me, so I now agree with your decision.

Then, in some other future patch, the ether_addr functions should be renamed to follow this convention too. And for backwards compatibility, their old names can be preserved as macros/functions calling the new names.

> 
> > static inline int rte_is_ipv6_version(const struct rte_ipv6_hdr *ip)
> > {
> > 	return ip->vtc_flow & RTE_IPV6_VTC_FLOW_VERSION_MASK ==
> RTE_IPV6_VTC_FLOW_VERSION;
> > }
> >
> > Or, at your preference, an optimized variant:
> > static inline int rte_is_version_ipv6(const struct rte_ipv6_hdr *ip)
> > {
> > 	return *(const unsigned char *)ip & 0xf0 == 0x60;
> > }
> >
> > The first nibble is also used for version in IPv4, so an IPv4 variant
> would look similar:
> > static inline int rte_is_version_ipv4(const struct rte_ip_hdr *ip)
> > {
> > 	return *(const unsigned char *)ip & 0xf0 == 0x40;
> > }
> 
> I had missed this in ipv4. I could rework it for v4 if there are other
> remarks.
> 
> Thanks for the review!