[ovs-dev] [PATCH ovn 1/2] controller: Avoid attempt to free NULL pointer.

[Dumitru Ceara]

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 <fnordahl at 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