On 9/17/25 10:24, Dumitru Ceara wrote:
On 9/16/25 8:25 PM, Frode Nordahl wrote:
While recent C standards dictate that free() should take no action
if it gets a NULL pointer, I'd argue it is bad practice to rely on
this.
The commit in the fixes tag made use of this to effectively use an
smap for sset when convenient, while this may be acceptable, let's
use an empty string instead of NULL.
Fixes: d7d886eca553 ("controller: Support learning routes per iface.")
Signed-off-by: Frode Nordahl <[email protected]>
---
Hi Frode,
Thanks for the patch!
However, we actually recommend the opposite when it comes to not
avoiding passing NULL to free(). From our coding-style document:
https://github.com/ovn-org/ovn/blob/main/Documentation/internals/contributing/coding-style.rst#functions
"Functions that destroy an instance of a dynamically-allocated type
should accept and ignore a null pointer argument. Code that calls such a
function (including the C standard library function free()) should omit
a null-pointer check. We find that this usually makes code easier to read."
And we already do that in numerous places in the code base.
I hope you don't mind but I'll archive this patch. I think it's fine to
keep the code as it currently is.
That is fair, knowing you likely reviewed the commit in the fixes tag, I
should have checked the original review.
Both options were in my mind and I went with my gut feeling, sorry for
the noise.
I'll review patch 2/2 next, no need to post v2 for it, I see it applies
just fine on its own.
Thank you so much for your pragmatic approach to this, much appreciated!
--
Frode Nordahl
Regards,
Dumitru
controller/route-exchange.c | 4 ++--
controller/route.c | 3 +--
2 files changed, 3 insertions(+), 4 deletions(-)
diff --git a/controller/route-exchange.c b/controller/route-exchange.c
index 161385aa4..6ad8ceb2e 100644
--- a/controller/route-exchange.c
+++ b/controller/route-exchange.c
@@ -171,8 +171,8 @@ sb_sync_learned_routes(const struct vector *learned_routes,
SMAP_FOR_EACH (port_node, bound_ports) {
/* The user specified an ifname, but we learned it on a different
* port. */
- if (port_node->value && strcmp(port_node->value,
- learned_route->ifname)) {
+ if (port_node->value && *port_node->value != '\0'
+ && strcmp(port_node->value, learned_route->ifname)) {
continue;
}
diff --git a/controller/route.c b/controller/route.c
index 841370ab2..7afa2578d 100644
--- a/controller/route.c
+++ b/controller/route.c
@@ -207,8 +207,7 @@ route_run(struct route_ctx_in *r_ctx_in,
"dynamic-routing-port-name");
if (!port_name) {
/* No port-name set, so we learn routes from all ports. */
- smap_add_nocopy(&ad->bound_ports,
- xstrdup(local_peer->logical_port), NULL);
+ smap_add(&ad->bound_ports, local_peer->logical_port, "");
} else {
/* If a port_name is set the we filter for the name as set in
* the port-mapping or the interface name of the local
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
