On 9/16/20 8:36 PM, Han Zhou wrote: > > > On Wed, Sep 16, 2020 at 10:51 AM Dumitru Ceara <dce...@redhat.com > <mailto:dce...@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! _______________________________________________ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
