On 9/17/25 6:08 PM, Frode Nordahl wrote:
> 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 <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.
>
> 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.
>
No worries!
>> 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!
>
Regards,
Dumitru
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
