Wang, hope it's clear that any new development is welcomed. One step before integration is to clearly explain why your code is needed. That's why a nack vote may help to discuss and decide.
Comments below 2015-06-02 13:15, Wang, Liang-min: > >2015-05-29 15:26, Liang-Min Larry Wang: > >> adding a new library based upon ethdev APIs to provide API's that bear > >> the same functionality as ethtool_ops (linux/ethtool.h) and > >> net_device_ops (linux/netdevice.h). > >> > >> Signed-off-by: Liang-Min Larry Wang <liang-min.wang at intel.com> > >> --- > >> MAINTAINERS | 4 + > >> config/common_linuxapp | 5 + > >> lib/Makefile | 1 + > >> lib/librte_ethtool/Makefile | 56 +++++++ > >> lib/librte_ethtool/rte_ethtool.c | 155 +++++++++++++++++ > >> lib/librte_ethtool/rte_ethtool.h | 257 > >> +++++++++++++++++++++++++++++ > >> lib/librte_ethtool/rte_ethtool_version.map | 18 ++ > >> mk/rte.app.mk | 1 + > > > >NACK for several reasons: > >- It's unclear what benefits this ethdev wrapper is bringing > > Since ethtool is provided to assist users migrating from kernel > ethtool/net_device_op based design to user-space DPDK device management. The > ethtool API's are created to closely maintain its original interface, > therefore this library depends on <linux/ethool.h>. To avoid pollute the > existing ethdev interface, a new library is created. To minimize code > replication and maintain closely 1:1 API definition with kernel space API, > this interface is designed based upon available ethdev APIs and add > additional dev_ops if it's necessary. > > >- There is no obvious interest (how is it supposed to be used?) > There are already two acknowledge on this release. Earlier comment on this > patch has that " ... The API's for ethtool like things are valuable ..." Stephen had some doubts about the real need and 2 people from Cisco (who never contributed before) give their ack without justification. Saying it's "valuable" or "very useful" is not enough. A new library needs to demonstrate in which scenario the added-value is. Sorry but you have to prove that it deserves to be maintained inside the dpdk project. > >- There is no update in the doc/ directory > Need more guidance on that. You probably have to add a new chapter in the programmer's guide. > >Other comments: > >- the patches are not versioned > > There is version file. Not sure what do you mean "the patches are not > versioned" I mean there is no v2/v3 in the Subject. Please read http://dpdk.org/dev#send > >- the copyright starts in 2010 > > Will fix that. > > >I'm curious to understand how renaming rte_eth_dev_set_mtu() to > >rte_ethtool_net_change_mtu() will help anyone. > > As described, this interface is designed to provide API closely to kernel > space ethtool ops and net_device_op. But the application still needs to adapt the code to call rte_* functions. So changing to rte_ethtool_net_change_mtu is equivalent to change to the existing rte_eth_dev_set_mtu. I don't see the benefit.