On Thu, Feb 26, 2026 at 3:54 PM Dumitru Ceara <[email protected]> wrote:
> On 2/26/26 3:48 PM, Ales Musil wrote: > > On Thu, Feb 26, 2026 at 3:26 PM Dumitru Ceara <[email protected]> wrote: > > > >> On 2/25/26 9:39 AM, Ales Musil via dev wrote: > >>> The LSP port security was parsed just to be used as check if there > >>> is any valid port security. The full parsing isn't necessary we can > >>> stop the parsing when we find at least one valid entry as a > >>> indication for the rest of the processing. This also ensure that we > >>> don't produce log when there is "VRRPv3" specified as it was just > >>> noise and didn't really affect the functionality. > >>> > >>> Fixes: d9e2393e9840 ("controller: Add option to make port security > >> compliant with RFC 9568.") > >>> Reported-at: https://issues.redhat.com/browse/FDP-3245 > >>> Signed-off-by: Ales Musil <[email protected]> > >>> --- > >> > >> Hi Ales, > >> > >> Thanks for the patch! > >> > > > > Hi Dumitru, > > > > thank you for the review. > > > > > >> > >>> northd/northd.c | 28 ++++++++-------------------- > >>> northd/northd.h | 4 ++-- > >>> tests/ovn.at | 2 ++ > >>> 3 files changed, 12 insertions(+), 22 deletions(-) > >>> > >>> diff --git a/northd/northd.c b/northd/northd.c > >>> index 983975dac..9401f41ab 100644 > >>> --- a/northd/northd.c > >>> +++ b/northd/northd.c > >>> @@ -1116,13 +1116,6 @@ ovn_port_cleanup(struct ovn_port *port) > >>> port->peer->peer = NULL; > >>> } > >>> > >>> - for (int i = 0; i < port->n_ps_addrs; i++) { > >>> - destroy_lport_addresses(&port->ps_addrs[i]); > >>> - } > >>> - free(port->ps_addrs); > >>> - port->ps_addrs = NULL; > >>> - port->n_ps_addrs = 0; > >>> - > >> > >> Now you use port->lsp_has_port_sec but it's not cleared here. There are > >> cases when ovn_port_cleanup(port) is called and then 'port' is > >> reinitialized, e.g., in ls_port_reinit(). There we call > >> ls_port_init(port) after cleanup. That calls parse_lsp_addrs() where we > >> only at most set port->lsp_has_port_sec = true. > >> > >> But if it used to be "true" and now it should be "false", we'd have an > >> issue. In theory. In practice that can't happen because if addresses > >> change lsp_can_be_inc_processed(port) returns false. > >> > >>> destroy_lport_addresses(&port->lrp_networks); > >>> destroy_lport_addresses(&port->proxy_arp_addrs); > >>> } > >>> @@ -1512,19 +1505,14 @@ parse_lsp_addrs(struct ovn_port *op) > >> > >> So, I think I'd set port->lsp_has_port_sec = false in the beginning of > >> parse_lsp_addrs(), somewhere above this point. > >> > >> What do you think? > >> > > > > Yeah that makes sense. We can probably do that above the for loop. > > > > OK, if that's the only change in this patch I think there's no need for > v2. I'm OK if you just amend your current commit with it and: > > Acked-by: Dumitru Ceara <[email protected]> > > Thanks, > Dumitru > > Thank you Dumitru, I have decided to go with setting false in the cleanup function in the end, but it should have the same effect. So with that I went ahead, merged this into main and backported all the way down to 24.03. Regards, Ales _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
