> From: Robin Jarry [mailto:rja...@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!
