On Mon, Dec 02, 2024 at 10:43:50AM +0100, Dumitru Ceara wrote:
> On 11/26/24 3:37 PM, Felix Huettner via dev wrote:
> > We already parse the networks of a port to the ovn_port struct, so there
> > is no need to reference northbound.
> > This is a prerequisite for later patches that use derived router ports.
> >
> > Acked-by: Lorenzo Bianconi <[email protected]>
> > Signed-off-by: Felix Huettner <[email protected]>
> > ---
>
> Hi Felix,
Hi Dumitru,
thanks for the review.
Smaller nits will be fixed in the next version.
>
> > northd/northd.c | 21 ++++++++++++++++++---
> > 1 file changed, 18 insertions(+), 3 deletions(-)
> >
> > diff --git a/northd/northd.c b/northd/northd.c
> > index 0fe15ac59..32a7c8509 100644
> > --- a/northd/northd.c
> > +++ b/northd/northd.c
> > @@ -3139,10 +3139,25 @@ ovn_port_update_sbrec(struct ovsdb_idl_txn
> > *ovnsb_txn,
> > sbrec_port_binding_set_parent_port(op->sb, NULL);
> > sbrec_port_binding_set_tag(op->sb, NULL, 0);
> >
> > + const struct lport_addresses *networks;
> > + if (op->primary_port) {
> > + networks = &op->primary_port->lrp_networks;
> > + } else {
> > + networks = &op->lrp_networks;
> > + }
> > struct ds s = DS_EMPTY_INITIALIZER;
> > - ds_put_cstr(&s, op->nbrp->mac);
> > - for (int i = 0; i < op->nbrp->n_networks; ++i) {
> > - ds_put_format(&s, " %s", op->nbrp->networks[i]);
> > + ds_put_cstr(&s, networks->ea_s);
> > + for (int i = 0; i < networks->n_ipv4_addrs; ++i) {
>
> Nit: size_t
> Nit: ++i is not really needed
>
> > + struct ipv4_netaddr addr = networks->ipv4_addrs[i];
> > + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen);
> > + }
> > + /* We do not need the ipv6 LLA. Since it is last in the list we
> > just
> > + * skip it. */
>
> Should we instead use
> in6_is_lla(&op->lrp_networks.ipv6_addrs[j].addr)
> ?
>
> If this has the potential of breaking things (e.g., when users
> explicitly set a LLA into the LRP.networks - although, is that even
> supported and OK?) we could add a helper to ovn-util.[hc]:
>
> lrp_networks_to_string(const struct lport_addresses *, struct ds *)
>
> Like that we don't make assumptions in northd.c about how addresses are
> stored by the ovn-util module.
I took a look into why there is even the need to ignore the generated
ipv6 address in the first place. It would seem to me more logical if the
address list contained all addresses a given port_binding has, instead
of ignoring one.
However i saw multiple places (e.g. bfd_monitor_run in pinctrl.c) that
check if there is any ipv6 address and if yes use the first one.
So for a previous ipv4 only port adding now this generated v6 address
would cause a behaviour change.
I am extremely uncertain if this would be breaking in any way.
I will therefor add the helpers to ovn-util as you suggested.
Thanks a lot
Felix
>
> > + if (networks->n_ipv6_addrs > 1) {
> > + for (int i = 0; i < networks->n_ipv6_addrs - 1; ++i) {
>
> Same nits about size_t and ++i.
>
> > + struct ipv6_netaddr addr = networks->ipv6_addrs[i];
> > + ds_put_format(&s, " %s/%d", addr.addr_s, addr.plen);
> > + }
> > }
> > const char *addresses = ds_cstr(&s);
> > sbrec_port_binding_set_mac(op->sb, &addresses, 1);
>
> Regards,
> Dumitru
>
> _______________________________________________
> dev mailing list
> [email protected]
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
