On Thu, Jun 4, 2026 at 8:47 PM Ilya Maximets <[email protected]> wrote:

> On 5/13/26 9:13 AM, Ales Musil via dev wrote:
> > Add support for unicast compose_nd_ns that allows the caller to
> > specify if the packet should be created as multicast or unicast
> > to specified address.
> >
> > Signed-off-by: Ales Musil <[email protected]>
> > ---
> > Note: This is planned to be used by OVN for MAC binding refresh,
> > as that it would be nice if this could be backported.
> > ---
> >  lib/packets.c                | 16 ++++++++++------
> >  lib/packets.h                |  1 +
> >  ofproto/ofproto-dpif-xlate.c |  2 +-
> >  3 files changed, 12 insertions(+), 7 deletions(-)
> >
> > diff --git a/lib/packets.c b/lib/packets.c
> > index 998a73afe..fc4bc6c7f 100644
> > --- a/lib/packets.c
> > +++ b/lib/packets.c
> > @@ -1869,19 +1869,23 @@ compose_ipv6(struct dp_packet *packet, uint8_t
> proto,
> >  /* Compose an IPv6 Neighbor Discovery Neighbor Solicitation message. */
> >  void
> >  compose_nd_ns(struct dp_packet *b, const struct eth_addr eth_src,
> > +              const struct eth_addr eth_dst, bool multicast,
>
> I'd suggest to move the boolean to the end of a list or put it after the
> dp_packet arg.
>
> Also, I see that the eth_src was constant, but there is no point for these
> eth arguments to be const, they are passed down by value.  Removing the
> const can save one extra variable below.
>
> >                const struct in6_addr *ipv6_src, const struct in6_addr
> *ipv6_dst)
> >  {
> > -    struct in6_addr sn_addr;
>
> I'd keep this one.  It seems better to keep the 'sn' naming.
>
> > -    struct eth_addr eth_dst;
> >      struct ovs_nd_msg *ns;
> >      struct ovs_nd_lla_opt *lla_opt;
> >      uint32_t icmp_csum;
> >
> > -    in6_addr_solicited_node(&sn_addr, ipv6_dst);
> > -    ipv6_multicast_to_ethernet(&eth_dst, &sn_addr);
> > +    struct in6_addr ipv6_dst_ = *ipv6_dst;
>
> If we keep the sn_addr declared at the top, this copy can be moved
> to an 'else' branch of the multicast.
>
> > +    struct eth_addr eth_dst_ = eth_dst;
>
> And this is not needed if we remove the const and can just update the
> argument.
>
> Best regards, Ilya Maximets.
>
>
Thank you, Ilya, for the review, I'll address those in v2.

Regards,
Ales
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Reply via email to