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 <fnord...@ubuntu.com>
> ---
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.
I'll review patch 2/2 next, no need to post v2 for it, I see it applies
just fine on its own.
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
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
