Hi Lucas,
Yes, that third option you suggested sounds good to me. It will be the
simplest one anyways. And then we can avoid the polarity flip too.
The only kind of clunky part will be handling the return value cast as I
mentioned before:
> i.e. use (bool) (some_value != 0) rather than (bool) some_value
but overall I think that's a fair tradeoff.
Take care,
Rosemarie O'Riorden
On 10/8/26 2:33 PM, Lucas Vargas Dias wrote:
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] <mailto:[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]
<mailto:[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
