Hi Lucas, thanks for the new patch!
I feel like I wasn't as clear as I should have been in my last review so
sorry about that, and hopefully this makes more sense.
I feel like it would make sense to remove nullable_string_is_equal()
entirely if you're removing all calls to it in OVN. It doesn't make
sense to have two almost identical functions between OVS and OVN IMO.
This patch removes calls from OVN, but the function and a bunch of calls
are still present in OVS. And so if nullable_string_is_equal() is still
available in OVN via OVS, it feels like duplicate code, and we might as
well have one function both projects for simplicity's sake.
So I see your options as:
1. Remove nullable_string_is_equal() from OVS. Add nullable_strcmp() to
OVS util.h. Replace all calls in OVS + OVN.
2. Leave nullable_string_is_equal() as is, and just use
nullable_strcmp() as you did in v1. Justify the duplicate code.
As I said above, I lean towards the 1st option.
Do you agree or have any thoughts?
See one other comment below.
On 10/5/26 2:47 PM, Lucas Vargas Dias wrote:
nullable_strcmp() was added to lib/ovn-util.h together with the ECMP
id ordering. Use it in the remaining callers of the OVS helper so that
OVN has a single way of comparing strings that may be NULL. Two NULLs
still compare equal, so there is no functional change.
Suggested-by: Rosemarie O'Riorden <[email protected]>
Signed-off-by: Lucas Vargas Dias <[email protected]>
---
v2:
- New patch, suggested in the v1 review.
northd/lb.c | 4 ++--
northd/lflow-mgr.c | 2 +-
northd/northd.c | 3 +--
3 files changed, 4 insertions(+), 5 deletions(-)
diff --git a/northd/lb.c b/northd/lb.c
index 5ff9d1fad..eeeb90b5c 100644
--- a/northd/lb.c
+++ b/northd/lb.c
@@ -345,8 +345,8 @@ ovn_northd_lb_init(struct ovn_northd_lb *lb,
const struct nbrec_load_balancer *nbrec_lb)
{
bool template = smap_get_bool(&nbrec_lb->options, "template", false);
- bool is_udp = nullable_string_is_equal(nbrec_lb->protocol, "udp");
- bool is_sctp = nullable_string_is_equal(nbrec_lb->protocol, "sctp");
+ bool is_udp = !nullable_strcmp(nbrec_lb->protocol, "udp");
+ bool is_sctp = !nullable_strcmp(nbrec_lb->protocol, "sctp");
OVN's coding style guide states the following:
"Also, don’t assume that a conversion to bool or _Bool follows C99
semantics, i.e. use (bool) (some_value != 0) rather than (bool)
some_value. The latter might produce unexpected results on non-C99
environments."
Thanks,
Rosemarie O'Riorden
int address_family = !strcmp(smap_get_def(&nbrec_lb->options,
"address-family", "ipv4"),
"ipv4")
diff --git a/northd/lflow-mgr.c b/northd/lflow-mgr.c
index deceadb74..ffe6e2e45 100644
--- a/northd/lflow-mgr.c
+++ b/northd/lflow-mgr.c
@@ -1131,7 +1131,7 @@ ovn_lflow_equal(const struct ovn_lflow *a, const struct
ovn_stage *stage,
&& a->priority == priority
&& !strcmp(a->match, match)
&& !strcmp(a->actions, actions)
- && nullable_string_is_equal(a->ctrl_meter, ctrl_meter)
+ && !nullable_strcmp(a->ctrl_meter, ctrl_meter)
&& a->acl_ct_translation == acl_ct_translation);
}
diff --git a/northd/northd.c b/northd/northd.c
index f37040b57..3fb75a423 100644
--- a/northd/northd.c
+++ b/northd/northd.c
@@ -12596,8 +12596,7 @@ parsed_route_lookup(struct hmap *routes, size_t hash,
continue;
}
- if (!nullable_string_is_equal(pr->lrp_addr_s,
- new_pr->lrp_addr_s)) {
+ if (nullable_strcmp(pr->lrp_addr_s, new_pr->lrp_addr_s)) {
continue;
}
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
