On Wed, Dec 4, 2019 at 12:26 PM Dumitru Ceara <[email protected]> wrote: > > On Tue, Dec 3, 2019 at 9:28 AM Han Zhou <[email protected]> wrote: > > > > > >
<snip> > > > > Thanks Dumitru. The approach looks valid. However, I think it could be > > improved a little, to avoid introducing the internal_data and make it > > easier to ensure there is no direct access to the node's data without > > validation. > > I think we can expose the engine_get_data() interface and force it to be > > used before accessing the data. The rules can be simply: always use this > > engine_get_data() to get engine node's data before accessing, except for > > accessing its own data in an engine node's handler (because it needs to be > > computed to become valid/updated). > > We can remove the ed_xxx variables such as: "struct ed_type_flow_output > > ed_flow_output;" so that we can avoid accessing data directly such as > > "&ed_flow_output.flow_table". Otherwise, it is hard to ensure the > > correctness if someone just update the code to bypass the if (...data) > > check. Code review would help, but it is still hard to guarantee. > > To remove the ed_xxx variable, we just need to move the data memory > > allocation in each node's init() function (and free it in cleanup()). I > > think I proposed this in an earlier version but I totally understand it is > > easy to get lost because of too much information exchanged. > > I really appreciate your effort for the fixes and improvements. I think I > > have no problem of merging this last patch with current version, but it > > could be even better. Please let me know if you are ok with the above > > suggestion, or I can merge it and let's address it with a separate patch. > > Thanks Han for the reviews and for applying the previous patches in the > series. > > You're right, it's cleaner to remove direct access to internal node > data. I thought the resulting code would be less verbose if we add the > "internal_data" field but it does add the need for extra care when > reviewing new code. > > I'll rework the last patch and send a v8. > > Thanks, > Dumitru > > > > > Thanks, > > Han > > Hi Han, I sent a v8 with the changes we discussed: https://patchwork.ozlabs.org/patch/1204267/ I also replaced all engine_node->data accesses outside inc-proc-eng.[ch] with API calls so that we don't make things confusing due to accessing engine_node->data in run/change handlers. Thanks, Dumitru _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
