Thank you again for the review, Ilya. I will be posting an updated patch (series, this time) addressing your comments shortly.
On Tue, 24 Feb 2026 at 22:07, Ilya Maximets <[email protected]> wrote: > On 2/18/26 6:48 PM, Matteo Perin via dev wrote: > > The --disable-system-route option was not fully preventing system routing > > rules from being cached at startup. When route_table_reset() was called, > > it would query all kernel routing rules via RTM_GETRULE and cache them > > with user=false, regardless of the use_system_routing_table flag. > > > > This also caused some unit tests to fail if non-standard system routing > > rules were present in the system and would appear in the cache. > > > > An internal ovs_router_rule_add__() function that unconditionally > > adds routing rules was added, following the same pattern used > > by ovs_router_insert(). > > > > ovs_router_rule_add() was modified to filter system rules based on > > configuration and table type: > > - Always allows user-configured rules (user=true) > > - Always allows standard table rules (local/main/default) even when > > system routing is disabled > > - Only allows non-standard system rules when use_system_routing_table > > is true > > > > Updated internal callers (init_standard_rules() and > > ovs_router_rule_add_cmd()) to use ovs_router_rule_add__() directly, > > ensuring standard rules and user-configured rules bypass the filter. > > > > This needs a 'Fixes' tag pointing to the commit that introduced the issue. I will make sure to add the 'Fixes' tag to all relevant commits, I apologize for failing to do so. On Tue, 24 Feb 2026 at 22:07, Ilya Maximets <[email protected]> wrote: > > +void > > +ovs_router_rule_add(uint32_t prio, bool invert, bool user, uint8_t > src_len, > > + const struct in6_addr *from, uint32_t lookup_table, > > + bool ipv4) > > + OVS_REQUIRES(mutex) > > Hmm, I don't think the caller of this function takes this lock, doesn't it? > We should probably take it in this fucntion around the > ovs_router_rule_add__() > call. May need to be a separate patch though. That's true, I failed to see it but it really does seem that we are missing the locking mechanism in the caller. I will add it where you suggested, as a separate patch in the series. On Tue, 24 Feb 2026 at 22:07, Ilya Maximets <[email protected]> wrote: > > +{ > > + /* Always add user-configured rules. > > + * For system (non-user) rules, only add if disable-system-route is > false > > + * or it is a standard table rule (local, main, default). > > + */ > > + if (user || use_system_routing_table || > is_standard_table(lookup_table)) { > > This function is never called with user==true, why do we need this argument > here at all? > > Also, why the exception for the standard tables? Is it because the flush > removes them? I'd say the flush removing them is the problem then and > we should add them back right after the flush in case it's not the > flush_all. > E.g. by calling the init function inside the ovs_router_rules_flush(). The user=true argument was me being too pedantic (and also helping myself follow the code a little better while working on it). But I see no reasons keeping it, as it is clear regardless. As you correctly pointed out, the exception for the standard tables was because of ovs_router_rules_flush() removing them in every instance, making the tests fail with the fixed option. I will change the flush function to add them back if flush_all=false and drop the additional condition. I am inclined to make the flush mechanism fix a separate patch for better readability, but I am also ok squashing it into the main --disable-system-route option behavior fix, if you prefer. Best Regards, Matteo _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
