Hi Rosemarie Thanks for your review. Ok, I understand it. I agree, I'll send a patch to OVS.
I think we have a third option. Could we change the function nullable_string_is_equal to make it closer to nullable_strcmp implementation? What do you think? Regards, Lucas Em qui., 8 de out. de 2026 às 13:52, Rosemarie O'Riorden < [email protected]> escreveu: > 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; > > } > > > > -- _‘Esta mensagem é direcionada apenas para os endereços constantes no cabeçalho inicial. Se você não está listado nos endereços constantes no cabeçalho, pedimos-lhe que desconsidere completamente o conteúdo dessa mensagem e cuja cópia, encaminhamento e/ou execução das ações citadas estão imediatamente anuladas e proibidas’._ * **‘Apesar do Magazine Luiza tomar todas as precauções razoáveis para assegurar que nenhum vírus esteja presente nesse e-mail, a empresa não poderá aceitar a responsabilidade por quaisquer perdas ou danos causados por esse e-mail ou por seus anexos’.* _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
