On 17.12.2018 12:49, Thomas Monjalon wrote: > 16/12/2018 13:02, Ophir Munk: >> Hi Ilya, >> Please find comments inline. >> >> From: Ilya Maximets <[email protected]> >>> >>> Not a full review. Just one comment inline. >>> >>> Best regards, Ilya Maximets. >>> >>> On 12.12.2018 2:34, Ophir Munk wrote: >>>> Dpdk port representors were introduced in dpdk 18.xx. >>> >> ..... >>>> >>>> - rte_eth_dev_close(port_id); >>>> + rte_dev = rte_eth_devices[port_id].device; >>> >>> >>> We should not use 'rte_eth_devices' directly because it's internal to DPDK. >>> See the discussion here: >>> http://mails.dpdk.org/archives/dev/2018-November/119198.html >> >> The discussions mentioned in the link raised pros and cons for using a >> direct access to rte_eth_devices[] but was not conclusive. >> I am in favor of keeping the direct access to rte_eth_devices array for the >> following reasons: >> 1. Using rte_eth_dev_info_get() API uses the same pointer to the internal >> rte_eth_devices[] array. So actually it is as dangerous as accessing the >> array directly. >> 2. Theoretically there is logic in using the API as it may have a safer >> implementation in the future. However, I talked with Thomans Monjalon - the >> maintainer of this API - and he recommended using the direct array access. >> Thomas mentioned that this API will be dropped. >> >> Thomas - can you please share your view on this topic? > > I agree with the idea of avoiding direct access to this structure. > That's why I wrote a new function specifically for this use case: > https://patches.dpdk.org/patch/48429/ > > About workarounding the direct access with rte_eth_dev_info_get(), > I am not sure it is a great idea. > In my opinion, it is best to keep the direct access to the array > and add a comment about the need to replace it with a proper API.
OK. Thank you, Thomas. That sounds reasonable. Ophir, please, add a "FIXME" comment that describes the situation. Best regards, Ilya Maximets. _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
