[ovs-dev] [PATCH ovn v2 2/2] lflow.c: Release ref_lflow_node as soon as it is not needed.

Dumitru Ceara dceara at redhat.com
Wed Sep 16 18:59:01 UTC 2020


On 9/16/20 8:36 PM, Han Zhou wrote:
> 
> 
> On Wed, Sep 16, 2020 at 10:51 AM Dumitru Ceara <dceara at redhat.com
> <mailto:dceara at redhat.com>> wrote:
>>
>> On 9/16/20 2:18 PM, Dumitru Ceara wrote:
>>
>> [...]
>>
>> >
>> >> +            hmap_remove(&lfrr->ref_lflow_table, &lrln->rlfn->node);
>> >> +            ref_lflow_node_destroy(lrln->rlfn);
>> >
>> > As mentioned on v1 (at least for me) it's really hard to keep track of
>> > what's going on due to the variable/field names, e.g., "lrln->rlfn". I'd
>> > really like it if we had somewhat more explicit names.
>> >
>>
>> This can be done as a follow up patch. I guess the goal is to fix the
>> crash as soon as possible. Refactoring should come afterwards.
>>
> Agree. For the names, what do you suggest? Here rlfn means
> ref_lflow_node, lrln means lflow_ref_list_node. It is common to use
> short abbreviations for variables with local scope, but maybe the rules
> are not clear enough. Maybe lrln can be lfrln? Or maybe the name
> ref_lflow_node itself is confusing? 'ref' represents a resource which is
> identified by ref_type + ref_name. Let me know your suggestion.
> 

I'm not sure to be honest. I understand the names but I still have a
problem thinking about what they refer to when I look at statements
using the field names. I know this is very subjective but I think it's
because of all the common letters between "lrln" and "lfrln", for example.

> For the comment, I hope my explains in 1/2 helped understanding it. The
> reason why the check was needed is basically what the code is telling:
> when the lflow_uuids is empty (no lflows referencing this resource).
> 

Right that's clear now, thanks!



More information about the dev mailing list